From e8eeca0bf4466ee1b196346d3a247535990cf44d Mon Sep 17 00:00:00 2001 From: Steve French Date: Mon, 19 Jun 2023 22:32:38 -0500 Subject: smb3: do not reserve too many oplock credits There were cases reported where servers will sometimes return more credits than requested on oplock break responses, which can lead to most of the credits being allocated for oplock breaks (instead of for normal operations like read and write) if number of SMB3 requests in flight always stays above 0 (the oplock and echo credits are rebalanced when in flight requests goes down to zero). If oplock credits gets unexpectedly large (e.g. three is more than it would ever be expected to be) and in flight requests are greater than zero, then rebalance the oplock credits and regular credits (go back to reserving just one oplock credit). Signed-off-by: Steve French --- fs/smb/client/smb2ops.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'fs/smb/client/smb2ops.c') diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c index a8bb9d00d33a..1dc2143ae924 100644 --- a/fs/smb/client/smb2ops.c +++ b/fs/smb/client/smb2ops.c @@ -109,7 +109,11 @@ smb2_add_credits(struct TCP_Server_Info *server, server->credits--; server->oplock_credits++; } - } + } else if ((server->in_flight > 0) && (server->oplock_credits > 3) && + ((optype & CIFS_OP_MASK) == CIFS_OBREAK_OP)) + /* if now have too many oplock credits, rebalance so don't starve normal ops */ + change_conf(server); + scredits = *val; in_flight = server->in_flight; spin_unlock(&server->req_lock); -- cgit v1.2.3 From 326a8d04f147e2bf393f6f9cdb74126ee6900607 Mon Sep 17 00:00:00 2001 From: Shyam Prasad N Date: Thu, 22 Jun 2023 18:16:04 +0000 Subject: cifs: do all necessary checks for credits within or before locking All the server credits and in-flight info is protected by req_lock. Once the req_lock is held, and we've determined that we have enough credits to continue, this lock cannot be dropped till we've made the changes to credits and in-flight count. However, we used to drop the lock in order to avoid deadlock with the recent srv_lock. This could cause the checks already made to be invalidated. Fixed it by moving the server status check to before locking req_lock. Fixes: d7d7a66aacd6 ("cifs: avoid use of global locks for high contention data") Signed-off-by: Shyam Prasad N Signed-off-by: Steve French --- fs/smb/client/smb2ops.c | 19 ++++++++++--------- fs/smb/client/transport.c | 20 ++++++++++---------- 2 files changed, 20 insertions(+), 19 deletions(-) (limited to 'fs/smb/client/smb2ops.c') diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c index 1dc2143ae924..3696d4ce0df3 100644 --- a/fs/smb/client/smb2ops.c +++ b/fs/smb/client/smb2ops.c @@ -215,6 +215,16 @@ smb2_wait_mtu_credits(struct TCP_Server_Info *server, unsigned int size, spin_lock(&server->req_lock); while (1) { + spin_unlock(&server->req_lock); + + spin_lock(&server->srv_lock); + if (server->tcpStatus == CifsExiting) { + spin_unlock(&server->srv_lock); + return -ENOENT; + } + spin_unlock(&server->srv_lock); + + spin_lock(&server->req_lock); if (server->credits <= 0) { spin_unlock(&server->req_lock); cifs_num_waiters_inc(server); @@ -225,15 +235,6 @@ smb2_wait_mtu_credits(struct TCP_Server_Info *server, unsigned int size, return rc; spin_lock(&server->req_lock); } else { - spin_unlock(&server->req_lock); - spin_lock(&server->srv_lock); - if (server->tcpStatus == CifsExiting) { - spin_unlock(&server->srv_lock); - return -ENOENT; - } - spin_unlock(&server->srv_lock); - - spin_lock(&server->req_lock); scredits = server->credits; /* can deadlock with reopen */ if (scredits <= 8) { diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c index 0474d0bba0a2..f280502a2aee 100644 --- a/fs/smb/client/transport.c +++ b/fs/smb/client/transport.c @@ -522,6 +522,16 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits, } while (1) { + spin_unlock(&server->req_lock); + + spin_lock(&server->srv_lock); + if (server->tcpStatus == CifsExiting) { + spin_unlock(&server->srv_lock); + return -ENOENT; + } + spin_unlock(&server->srv_lock); + + spin_lock(&server->req_lock); if (*credits < num_credits) { scredits = *credits; spin_unlock(&server->req_lock); @@ -547,15 +557,6 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits, return -ERESTARTSYS; spin_lock(&server->req_lock); } else { - spin_unlock(&server->req_lock); - - spin_lock(&server->srv_lock); - if (server->tcpStatus == CifsExiting) { - spin_unlock(&server->srv_lock); - return -ENOENT; - } - spin_unlock(&server->srv_lock); - /* * For normal commands, reserve the last MAX_COMPOUND * credits to compound requests. @@ -569,7 +570,6 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits, * for servers that are slow to hand out credits on * new sessions. */ - spin_lock(&server->req_lock); if (!optype && num_credits == 1 && server->in_flight > 2 * MAX_COMPOUND && *credits <= MAX_COMPOUND) { -- cgit v1.2.3 From ac615db03ba508d42d240612262f21f2e5836b67 Mon Sep 17 00:00:00 2001 From: Shyam Prasad N Date: Tue, 20 Jun 2023 02:56:06 +0000 Subject: cifs: log session id when a matching ses is not found We do not log the session id in crypt_setup when a matching session is not found. Printing the session id helps debugging here. This change does just that. This change also changes this log to FYI, since it is normal to see then during a reconnect. Doing the same for a similar log in case of signed connections. The plan is to have a tracepoint for this event, so that we will be able to see this event if need be. That will be done as another change. Signed-off-by: Shyam Prasad N Signed-off-by: Steve French --- fs/smb/client/smb2ops.c | 4 ++-- fs/smb/client/smb2transport.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'fs/smb/client/smb2ops.c') diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c index 3696d4ce0df3..7f8e07c42d4c 100644 --- a/fs/smb/client/smb2ops.c +++ b/fs/smb/client/smb2ops.c @@ -4444,8 +4444,8 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst, rc = smb2_get_enc_key(server, le64_to_cpu(tr_hdr->SessionId), enc, key); if (rc) { - cifs_server_dbg(VFS, "%s: Could not get %scryption key\n", __func__, - enc ? "en" : "de"); + cifs_server_dbg(FYI, "%s: Could not get %scryption key. sid: 0x%llx\n", __func__, + enc ? "en" : "de", le64_to_cpu(tr_hdr->SessionId)); return rc; } diff --git a/fs/smb/client/smb2transport.c b/fs/smb/client/smb2transport.c index 22954a9c7a6c..c3e9cb5c7be5 100644 --- a/fs/smb/client/smb2transport.c +++ b/fs/smb/client/smb2transport.c @@ -92,7 +92,7 @@ int smb2_get_sign_key(__u64 ses_id, struct TCP_Server_Info *server, u8 *key) if (ses->Suid == ses_id) goto found; } - cifs_server_dbg(VFS, "%s: Could not find session 0x%llx\n", + cifs_server_dbg(FYI, "%s: Could not find session 0x%llx\n", __func__, ses_id); rc = -ENOENT; goto out; @@ -564,7 +564,7 @@ smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server, rc = smb2_get_sign_key(le64_to_cpu(shdr->SessionId), server, key); if (unlikely(rc)) { - cifs_server_dbg(VFS, "%s: Could not get signing key\n", __func__); + cifs_server_dbg(FYI, "%s: Could not get signing key\n", __func__); return rc; } -- cgit v1.2.3 From 61986a58bc6abbb1aea26e52bd269f49e5bacf19 Mon Sep 17 00:00:00 2001 From: Shyam Prasad N Date: Tue, 27 Jun 2023 06:22:20 +0000 Subject: cifs: new dynamic tracepoint to track ses not found errors It is perfectly valid to not find session not found errors when a reconnect of a session happens when requests for the same session are happening in parallel. We had these log messages as VFS logs. My last change dumped these logs as FYI logs. This change just creates a new dynamic tracepoint to capture events of this type, just in case it is useful while debugging issues in the future. Signed-off-by: Shyam Prasad N Signed-off-by: Steve French --- fs/smb/client/smb2ops.c | 2 ++ fs/smb/client/smb2transport.c | 1 + fs/smb/client/trace.h | 20 ++++++++++++++++++++ 3 files changed, 23 insertions(+) (limited to 'fs/smb/client/smb2ops.c') diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c index 7f8e07c42d4c..eb1340b9125e 100644 --- a/fs/smb/client/smb2ops.c +++ b/fs/smb/client/smb2ops.c @@ -4414,6 +4414,8 @@ smb2_get_enc_key(struct TCP_Server_Info *server, __u64 ses_id, int enc, u8 *key) } spin_unlock(&cifs_tcp_ses_lock); + trace_smb3_ses_not_found(ses_id); + return -EAGAIN; } /* diff --git a/fs/smb/client/smb2transport.c b/fs/smb/client/smb2transport.c index c3e9cb5c7be5..c6db898dab7c 100644 --- a/fs/smb/client/smb2transport.c +++ b/fs/smb/client/smb2transport.c @@ -92,6 +92,7 @@ int smb2_get_sign_key(__u64 ses_id, struct TCP_Server_Info *server, u8 *key) if (ses->Suid == ses_id) goto found; } + trace_smb3_ses_not_found(ses_id); cifs_server_dbg(FYI, "%s: Could not find session 0x%llx\n", __func__, ses_id); rc = -ENOENT; diff --git a/fs/smb/client/trace.h b/fs/smb/client/trace.h index d3053bd8ae73..e671bd16f00c 100644 --- a/fs/smb/client/trace.h +++ b/fs/smb/client/trace.h @@ -1003,6 +1003,26 @@ DEFINE_EVENT(smb3_reconnect_class, smb3_##name, \ DEFINE_SMB3_RECONNECT_EVENT(reconnect); DEFINE_SMB3_RECONNECT_EVENT(partial_send_reconnect); +DECLARE_EVENT_CLASS(smb3_ses_class, + TP_PROTO(__u64 sesid), + TP_ARGS(sesid), + TP_STRUCT__entry( + __field(__u64, sesid) + ), + TP_fast_assign( + __entry->sesid = sesid; + ), + TP_printk("sid=0x%llx", + __entry->sesid) +) + +#define DEFINE_SMB3_SES_EVENT(name) \ +DEFINE_EVENT(smb3_ses_class, smb3_##name, \ + TP_PROTO(__u64 sesid), \ + TP_ARGS(sesid)) + +DEFINE_SMB3_SES_EVENT(ses_not_found); + DECLARE_EVENT_CLASS(smb3_credit_class, TP_PROTO(__u64 currmid, __u64 conn_id, -- cgit v1.2.3