[PATCH v2 0/2] reconnect fixes and improvements

[PATCH v2 0/2] reconnect fixes and improvements

[Enzo Matsumiya]

Hi all,

Sending v2 to address the issues found by sashiko
https://sashiko.dev/#/patchset/20260925202003.618856-1-ematsumiya%40suse.de

Please refer to the original cover letter for rationale and reproducer
https://lore.kernel.org/linux-cifs/20260925202003.618856-1-ematsumiya@suse.de/


Cheers,

Enzo Matsumiya (2):
  smb: client: fast fail sends if need to reconnect
  smb: client: prevent premature discard of requests if reconnecting

 fs/smb/client/cifsglob.h  |  4 ++
 fs/smb/client/connect.c   | 58 ++++++++++++++++++++-----
 fs/smb/client/misc.c      | 91 +++++++++++++++++++++++++++------------
 fs/smb/client/transport.c |  8 ++++
 4 files changed, 124 insertions(+), 37 deletions(-)

-- 
2.55.0

[PATCH v2 1/2] smb: client: fast fail sends if need to reconnect

[Enzo Matsumiya]

smb_send_kvec() keeps retrying when server needs to reconnect, which
makes no sense as cifs_reconnect() will never run in parallel (because
both need server mutex).
IOW, retrying will never succeed, but only delay reconnects further.

Bail out early from smb_send_kvec() when need to reconnect.

Also shutdown socket queues when CifsNeedReconnect is first set, so any
sends/receives fails immediately (don't wait for socket to timeout).

Consequential change:
To shutdown the socket, we need to hold server mutex. Since not all
callers hold it when calling cifs_signal_cifsd_for_reconnect(), we
need to keep track of the owner task, so we can lock it as needed.

Add TCP_Server_Info::mutex_owner for that.

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
v1 -> v2 (fix issues detected by sashiko):
- move kernel_sock_shutdown() call out of cifs_tcp_ses_lock
- use server mutex to check/shutdown socket


 fs/smb/client/cifsglob.h  |  3 +++
 fs/smb/client/connect.c   | 54 +++++++++++++++++++++++++++++++--------
 fs/smb/client/transport.c |  8 ++++++
 3 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 79e4e84f8985..d279b5c92a08 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -711,6 +711,7 @@ struct TCP_Server_Info {
 	spinlock_t req_lock;  /* protect the two values above */
 	struct mutex _srv_mutex;
 	unsigned int nofs_flag;
+	struct task_struct *mutex_owner;
 	struct task_struct *tsk;
 	char server_GUID[16];
 	__u16 sec_mode;
@@ -847,12 +848,14 @@ static inline void cifs_server_lock(struct TCP_Server_Info *server)
 
 	mutex_lock(&server->_srv_mutex);
 	server->nofs_flag = nofs_flag;
+	server->mutex_owner = current;
 }
 
 static inline void cifs_server_unlock(struct TCP_Server_Info *server)
 {
 	unsigned int nofs_flag = server->nofs_flag;
 
+	server->mutex_owner = NULL;
 	mutex_unlock(&server->_srv_mutex);
 	memalloc_nofs_restore(nofs_flag);
 }
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 28e1ddeb6182..4cc433308c96 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -127,13 +127,42 @@ void smb2_query_server_interfaces(struct work_struct *work)
 			   (SMB_INTERFACE_POLL_INTERVAL * HZ));
 }
 
-#define set_need_reco(server) \
-do { \
-	spin_lock(&server->srv_lock); \
-	if (server->tcpStatus != CifsExiting) \
-		server->tcpStatus = CifsNeedReconnect; \
-	spin_unlock(&server->srv_lock); \
-} while (0)
+static inline bool __set_need_reco(struct TCP_Server_Info *server)
+{
+	bool need_reco = false;
+
+	spin_lock(&server->srv_lock);
+	if (server->tcpStatus != CifsExiting) {
+		need_reco = (server->tcpStatus != CifsNeedReconnect);
+		server->tcpStatus = CifsNeedReconnect;
+	}
+	spin_unlock(&server->srv_lock);
+
+	return need_reco;
+}
+
+static inline void kill_socket_queues(struct TCP_Server_Info *server)
+{
+	bool locked = false;
+
+	if (server->mutex_owner != current) {
+		cifs_server_lock(server);
+		locked = true;
+	}
+
+	if (server->ssocket)
+		/* Don't release it yet! */
+		kernel_sock_shutdown(server->ssocket, SHUT_RDWR);
+
+	if (locked)
+		cifs_server_unlock(server);
+}
+
+static inline void set_need_reco(struct TCP_Server_Info *server)
+{
+	if (__set_need_reco(server))
+		kill_socket_queues(server);
+}
 
 /*
  * Update the tcpStatus for the server.
@@ -151,6 +180,7 @@ cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server,
 	struct TCP_Server_Info *nserver;
 	struct cifs_ses *ses;
 	LIST_HEAD(reco);
+	bool shutdown;
 	int i;
 
 	/* if we need to signal just this channel */
@@ -162,7 +192,7 @@ cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server,
 	if (SERVER_IS_CHAN(server))
 		server = server->primary_server;
 	scoped_guard(spinlock, &cifs_tcp_ses_lock) {
-		set_need_reco(server);
+		shutdown = __set_need_reco(server);
 		list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
 			spin_lock(&ses->ses_lock);
 			if (ses->ses_status == SES_EXITING) {
@@ -184,6 +214,9 @@ cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server,
 		}
 	}
 
+	if (shutdown)
+		kill_socket_queues(server);
+
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry_safe(server, nserver, &reco, rlist) {
 		list_del_init(&server->rlist);
@@ -365,9 +398,10 @@ static bool cifs_tcp_ses_needs_reconnect(struct TCP_Server_Info *server, int num
 	cifs_dbg(FYI, "Mark tcp session as need reconnect\n");
 	trace_smb3_reconnect(server->current_mid, server->conn_id,
 			     server->hostname);
-	server->tcpStatus = CifsNeedReconnect;
-
 	spin_unlock(&server->srv_lock);
+
+	set_need_reco(server);
+
 	return true;
 }
 
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index 6e21b5f8754a..c6e0ee6d0ef4 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -180,6 +180,10 @@ smb_send_kvec(struct TCP_Server_Info *server, struct msghdr *smb_msg,
 		 */
 		rc = sock_sendmsg(ssocket, smb_msg);
 		if (rc == -EAGAIN || unlikely(rc == -EINTR && task_work_pending(current))) {
+			/* Don't retry if need to reconnect */
+			if (unlikely(server->tcpStatus == CifsNeedReconnect))
+				return -ECONNRESET;
+
 			retries++;
 			if (retries >= 14 ||
 			    (!server->noblocksnd && (retries > 2))) {
@@ -191,6 +195,10 @@ smb_send_kvec(struct TCP_Server_Info *server, struct msghdr *smb_msg,
 			continue;
 		}
 
+		/* Blame the need to reconnect for any non-interrupt error */
+		if (unlikely(server->tcpStatus == CifsNeedReconnect && !is_interrupt_error(rc)))
+			rc = -ECONNRESET;
+
 		if (rc < 0)
 			return rc;
 
-- 
2.55.0

Re: [PATCH v2 1/2] smb: client: fast fail sends if need to reconnect

[Namjae Jeon]

> @@ -184,6 +214,9 @@ cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server,
>                 }
>         }
>
> +       if (shutdown)
> +               kill_socket_queues(server);
> +
>         spin_lock(&cifs_tcp_ses_lock);
Since this loop still holds cifs_tcp_ses_lock, It can sleep while
holding a spinlock, causing a scheduling-while-atomic failure. The
previous set_need_reco macro did not acquire a mutex.

Please check sashiko's review comments.

https://sashiko.dev/#/patchset/20260928150355.850472-1-ematsumiya%40suse.de

Thanks.
>         list_for_each_entry_safe(server, nserver, &reco, rlist) {
>                 list_del_init(&server->rlist);
> @@ -365,9 +398,10 @@ static bool cifs_tcp_ses_needs_reconnect(struct TCP_Server_Info *server, int num
>         cifs_dbg(FYI, "Mark tcp session as need reconnect\n");
>         trace_smb3_reconnect(server->current_mid, server->conn_id,
>                              server->hostname);
> -       server->tcpStatus = CifsNeedReconnect;
> -
>         spin_unlock(&server->srv_lock);
> +
> +       set_need_reco(server);
> +
>         return true;

Re: [PATCH v2 1/2] smb: client: fast fail sends if need to reconnect

[Enzo Matsumiya]

On 09/29, Namjae Jeon wrote:
>> @@ -184,6 +214,9 @@ cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server,
>>                 }
>>         }
>>
>> +       if (shutdown)
>> +               kill_socket_queues(server);
>> +
>>         spin_lock(&cifs_tcp_ses_lock);
>Since this loop still holds cifs_tcp_ses_lock, It can sleep while
>holding a spinlock, causing a scheduling-while-atomic failure.

I totally missed that second loop, will handle that in v3.

AFAICS, the new set_need_reco() can be moved to after unlocking
cifs_tcp_ses_lock, as tcpStatus is already protected by srv_lock.
Any objections to that?

>The
>previous set_need_reco macro did not acquire a mutex.

Yes, the server mutex is needed to access/shutdown server->ssocket,
which is part of the core change in this patch.

>Please check sashiko's review comments.
>
>https://sashiko.dev/#/patchset/20260928150355.850472-1-ematsumiya%40suse.de

The other concerns could be ignored, but I'll address them anyway in v3.


Cheers,

Enzo

>Thanks.
>>         list_for_each_entry_safe(server, nserver, &reco, rlist) {
>>                 list_del_init(&server->rlist);
>> @@ -365,9 +398,10 @@ static bool cifs_tcp_ses_needs_reconnect(struct TCP_Server_Info *server, int num
>>         cifs_dbg(FYI, "Mark tcp session as need reconnect\n");
>>         trace_smb3_reconnect(server->current_mid, server->conn_id,
>>                              server->hostname);
>> -       server->tcpStatus = CifsNeedReconnect;
>> -
>>         spin_unlock(&server->srv_lock);
>> +
>> +       set_need_reco(server);
>> +
>>         return true;

[PATCH v2 2/2] smb: client: prevent premature discard of requests if reconnecting

[Enzo Matsumiya]

Add TCP_Server_Info::reconnecting to track cifs_reconnect() lifetime.
Wait for it to be true in cifs_wait_for_server_reconnect(), and only
then wait for tcpStatus change.

Also, return -ECONNRESET (instead of -EHOSTDOWN) if reconnect is still
ongoing at the end, so requests are not discarded unnecessarily.
-ECONNRESET is not immediately retried by intermediate layers, e.g. VFS,
but still serves as an indication to userspace that retrying the
operation might be successful.

Changes (refactor cifs_wait_for_server_reconnect()):
- remove unnecessary do/while loop; use wait_event_interruptible()
  for hard mounts
- increase timeout, and base it on echo_interval to match user
  preferences
- remove useless debug log for interrupt errors

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
v1 -> v2 (fix issues detected by sashiko):
- handle condition changes post-wait_event timeouts
- return -ECONNRESET instead of -EAGAIN when leaving still reconnecting

(not from sashiko):
- add missing wake_up() after setting server->reconnecting to true in
  cifs_tcp_ses_needs_reconnect()
- handle tcpStatus == CifsExiting in cifs_wait_for_server_reconnect()


 fs/smb/client/cifsglob.h |  1 +
 fs/smb/client/connect.c  |  4 ++
 fs/smb/client/misc.c     | 91 ++++++++++++++++++++++++++++------------
 3 files changed, 69 insertions(+), 27 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index d279b5c92a08..9623ee19312c 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -803,6 +803,7 @@ struct TCP_Server_Info {
 	bool	posix_ext_supported;
 	struct delayed_work reconnect; /* reconnect workqueue job */
 	struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
+	bool reconnecting; /* if cifs_reconnect() is indeed running */
 	unsigned long echo_interval;
 
 	/*
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 4cc433308c96..58efc5392fc4 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -398,6 +398,8 @@ static bool cifs_tcp_ses_needs_reconnect(struct TCP_Server_Info *server, int num
 	cifs_dbg(FYI, "Mark tcp session as need reconnect\n");
 	trace_smb3_reconnect(server->current_mid, server->conn_id,
 			     server->hostname);
+	server->reconnecting = true;
+	wake_up(&server->response_q);
 	spin_unlock(&server->srv_lock);
 
 	set_need_reco(server);
@@ -475,6 +477,7 @@ static int __cifs_reconnect(struct TCP_Server_Info *server,
 	spin_lock(&server->srv_lock);
 	if (server->tcpStatus == CifsNeedNegotiate)
 		mod_delayed_work(cifsiod_wq, &server->echo, 0);
+	server->reconnecting = false;
 	spin_unlock(&server->srv_lock);
 
 	wake_up(&server->response_q);
@@ -617,6 +620,7 @@ static int reconnect_dfs_server(struct TCP_Server_Info *server)
 	spin_lock(&server->srv_lock);
 	if (server->tcpStatus == CifsNeedNegotiate)
 		mod_delayed_work(cifsiod_wq, &server->echo, 0);
+	server->reconnecting = false;
 	spin_unlock(&server->srv_lock);
 
 	wake_up(&server->response_q);
diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index 05168284f205..24aa0333553a 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -1077,44 +1077,81 @@ int cifs_inval_name_dfs_link_error(const unsigned int xid,
 
 int cifs_wait_for_server_reconnect(struct TCP_Server_Info *server, bool retry)
 {
-	int timeout = 10;
-	int rc;
+	int timeout, rc = 0;
+
+	/* Hard mounts keep waiting until process is killed or server comes back on-line. */
+	if (retry)
+		return wait_event_interruptible(server->response_q,
+						(server->tcpStatus != CifsNeedReconnect));
 
 	spin_lock(&server->srv_lock);
 	if (server->tcpStatus != CifsNeedReconnect) {
+		if (unlikely(server->tcpStatus == CifsExiting))
+			rc = -ESHUTDOWN;
 		spin_unlock(&server->srv_lock);
-		return 0;
+
+		return rc;
 	}
-	timeout *= server->nr_targets;
+
+	/*
+	 * Soft mounts, wait with timeout.
+	 * Compute timeout based on echo_interval, to match user expectations wrt. time to recover
+	 * from network interruptions.
+	 *
+	 * Minimum is 17 seconds (per reconnect target):
+	 *   7s for cifs socket timeout
+	 *   3s sleep after a failed attempt
+	 *   7s spare, to cover at least a full attempt
+	 *
+	 * Maximum is 60s; might not be enough in some rare cases, but >60s is too much.
+	 */
+	timeout = max(server->echo_interval / HZ, 17) * server->nr_targets;
+	timeout = min(timeout, 60);
 	spin_unlock(&server->srv_lock);
 
 	/*
-	 * Give demultiplex thread up to 10 seconds to each target available for
-	 * reconnect -- should be greater than cifs socket timeout which is 7
-	 * seconds.
+	 * First, wait for cifs_reconnect() to start.
+	 * This is needed because tcpStatus == CifsNeedReconnect doesn't imply cifs_reconnect()
+	 * is actually running.
 	 *
-	 * On "soft" mounts we wait once. Hard mounts keep retrying until
-	 * process is killed or server comes back on-line.
+	 * Wait 3*timeout (max 180s) here, which should be more than enough.
 	 */
-	do {
-		rc = wait_event_interruptible_timeout(server->response_q,
-						      (server->tcpStatus != CifsNeedReconnect),
-						      timeout * HZ);
-		if (rc < 0) {
-			cifs_dbg(FYI, "%s: aborting reconnect due to received signal\n",
-				 __func__);
-			return -ERESTARTSYS;
-		}
+	rc = wait_event_interruptible_timeout(server->response_q,
+					      (server->reconnecting ||
+					       server->tcpStatus != CifsNeedReconnect),
+					      min(3 * timeout, 180) * HZ);
+
+	/* cifs_reconnect() never started, don't wait any longer */
+	spin_lock(&server->srv_lock);
+	if (!rc && !server->reconnecting && server->tcpStatus == CifsNeedReconnect)
+		rc = -EHOSTDOWN;
+	spin_unlock(&server->srv_lock);
 
-		/* are we still trying to reconnect? */
-		spin_lock(&server->srv_lock);
-		if (server->tcpStatus != CifsNeedReconnect) {
-			spin_unlock(&server->srv_lock);
-			return 0;
+	if (rc < 0)
+		return rc;
+
+	/* cifs_reconnect() started (maybe already succeeded), wait for status change */
+	rc = wait_event_interruptible_timeout(server->response_q,
+					      (server->tcpStatus != CifsNeedReconnect),
+					      timeout * HZ);
+	if (rc < 0)
+		return rc;
+
+	spin_lock(&server->srv_lock);
+	if (server->tcpStatus != CifsNeedReconnect) {
+		rc = 0;
+		if (server->tcpStatus == CifsExiting)
+			rc = -ESHUTDOWN;
+	} else {
+		rc = -ECONNRESET;
+
+		/* Are we still even trying to reconnect? */
+		if (!server->reconnecting) {
+			cifs_dbg(FYI, "%s: gave up waiting on reconnect\n", __func__);
+			rc = -EHOSTDOWN;
 		}
-		spin_unlock(&server->srv_lock);
-	} while (retry);
+	}
+	spin_unlock(&server->srv_lock);
 
-	cifs_dbg(FYI, "%s: gave up waiting on reconnect\n", __func__);
-	return -EHOSTDOWN;
+	return rc;
 }
-- 
2.55.0

Re: [PATCH v2 0/2] reconnect fixes and improvements

[Enzo Matsumiya]

Hi,

Any comments on these 2 patches?
We have at least a couple of customers with huge workloads impacted by
this problem.


Thanks,

Enzo

On 09/28, Enzo Matsumiya wrote:
>Hi all,
>
>Sending v2 to address the issues found by sashiko
>https://sashiko.dev/#/patchset/20260925202003.618856-1-ematsumiya%40suse.de
>
>Please refer to the original cover letter for rationale and reproducer
>https://lore.kernel.org/linux-cifs/20260925202003.618856-1-ematsumiya@suse.de/
>
>
>Cheers,
>
>Enzo Matsumiya (2):
>  smb: client: fast fail sends if need to reconnect
>  smb: client: prevent premature discard of requests if reconnecting
>
> fs/smb/client/cifsglob.h  |  4 ++
> fs/smb/client/connect.c   | 58 ++++++++++++++++++++-----
> fs/smb/client/misc.c      | 91 +++++++++++++++++++++++++++------------
> fs/smb/client/transport.c |  8 ++++
> 4 files changed, 124 insertions(+), 37 deletions(-)
>
>-- 
>2.55.0
>
>

Re: [PATCH v2 0/2] reconnect fixes and improvements

[Paulo Alcantara]

Enzo Matsumiya <ematsumiya@suse.de> writes:

> Any comments on these 2 patches?
> We have at least a couple of customers with huge workloads impacted by
> this problem.

Did you send v3 addressing sashiko[1] and Namjae reviews?

[1] https://sashiko.dev/#/patchset/20260928150355.850472-1-ematsumiya%40suse.de

Re: [PATCH v2 0/2] reconnect fixes and improvements

[Enzo Matsumiya]

On 10/06, Paulo Alcantara wrote:
>Enzo Matsumiya <ematsumiya@suse.de> writes:
>
>> Any comments on these 2 patches?
>> We have at least a couple of customers with huge workloads impacted by
>> this problem.
>
>Did you send v3 addressing sashiko[1] and Namjae reviews?
>
>[1] https://sashiko.dev/#/patchset/20260928150355.850472-1-ematsumiya%40suse.de

Yes, I did (I just noticed I replied on the v2 thread).

Patch 1:
https://lore.kernel.org/linux-cifs/20260929170358.270612-1-ematsumiya@suse.de/

Patch 2:
https://lore.kernel.org/linux-cifs/20260929170358.270612-2-ematsumiya@suse.de/


Thanks,

Enzo