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

[PATCH v3 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 detected,
so any sends/receives fails immediately (and don't wait for socket to
timeout).

Add TCP_Server_Info::need_sock_shutdown to ensure socket is shutdown
only once.

kernel_sock_shutdown() will be called from whichever task detects it
first;  cifsd (cifs_tcp_ses_needs_reconnect()), or sending tasks,
through cifs_call_async() or compound_send_recv().

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
v2 -> v3 (patch refactor, change strategy but keep concept):
- remove TCP_Server_Info::mutex_owner (covers sashiko report)
- shutdown socket directly from sender functions (when need to reconnect)
- bail out from smb_send_kvec() when need to reconnect only if rc <= 0
  (from sasiko report)

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  |  1 +
 fs/smb/client/connect.c   | 22 +++++++++++++++++++---
 fs/smb/client/transport.c | 30 ++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 79e4e84f8985..e49140d5c4a4 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -802,6 +802,7 @@ struct TCP_Server_Info {
 	bool	posix_ext_supported;
 	struct delayed_work reconnect; /* reconnect workqueue job */
 	struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
+	bool need_sock_shutdown; /* true when CifsNeedReconnect was first set by non-cifsd task */
 	unsigned long echo_interval;
 
 	/*
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 28e1ddeb6182..733d50b042fd 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -126,12 +126,14 @@ void smb2_query_server_interfaces(struct work_struct *work)
 	queue_delayed_work(cifsiod_wq, &tcon->query_interfaces,
 			   (SMB_INTERFACE_POLL_INTERVAL * HZ));
 }
-
 #define set_need_reco(server) \
 do { \
 	spin_lock(&server->srv_lock); \
-	if (server->tcpStatus != CifsExiting) \
+	if (server->tcpStatus != CifsExiting) { \
+		if (server->tcpStatus != CifsNeedReconnect) \
+			server->need_sock_shutdown = true; \
 		server->tcpStatus = CifsNeedReconnect; \
+	} \
 	spin_unlock(&server->srv_lock); \
 } while (0)
 
@@ -353,6 +355,8 @@ cifs_abort_connection(struct TCP_Server_Info *server)
 
 static bool cifs_tcp_ses_needs_reconnect(struct TCP_Server_Info *server, int num_targets)
 {
+	bool shutdown;
+
 	spin_lock(&server->srv_lock);
 	server->nr_targets = num_targets;
 	if (server->tcpStatus == CifsExiting) {
@@ -365,9 +369,21 @@ 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;
 
+	/* Cover cases where sender tasks didn't manage to shutdown the socket for some reason */
+	shutdown = (server->tcpStatus != CifsNeedReconnect || server->need_sock_shutdown);
+	server->need_sock_shutdown = false;
+	server->tcpStatus = CifsNeedReconnect;
 	spin_unlock(&server->srv_lock);
+
+	if (shutdown) {
+		cifs_server_lock(server);
+		if (server->ssocket)
+			/* Don't release it here/yet! */
+			kernel_sock_shutdown(server->ssocket, SHUT_RDWR);
+		cifs_server_unlock(server);
+	}
+
 	return true;
 }
 
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index 6e21b5f8754a..a1ae8548cf16 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -179,7 +179,18 @@ smb_send_kvec(struct TCP_Server_Info *server, struct msghdr *smb_msg,
 		 * to avoid unnecessary reconnects.
 		 */
 		rc = sock_sendmsg(ssocket, smb_msg);
+
+		/*
+		 * If need to reconnect, blame it for any non-interrupt error and bail out early,
+		 * even if -EAGAIN;  reconnect will never happen in parallel as cifs_reconnect()
+		 * needs server mutex (which we're already holding), so it's pointless to retry.
+		 */
+		if (unlikely(rc <= 0 && server->tcpStatus == CifsNeedReconnect &&
+			     !is_interrupt_error(rc)))
+			return -ECONNRESET;
+
 		if (rc == -EAGAIN || unlikely(rc == -EINTR && task_work_pending(current))) {
+
 			retries++;
 			if (retries >= 14 ||
 			    (!server->noblocksnd && (retries > 2))) {
@@ -654,6 +665,23 @@ int wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 	return 0;
 }
 
+static inline void cond_sock_shutdown(struct TCP_Server_Info *server)
+{
+	bool shutdown;
+
+	lockdep_assert_held(&server->_srv_mutex);
+
+	spin_lock(&server->srv_lock);
+	/* No need to check status, need_sock_shutdown is only set if CifsNeedReconnect */
+	shutdown = (server->need_sock_shutdown && server->ssocket);
+	server->need_sock_shutdown = false;
+	spin_unlock(&server->srv_lock);
+
+	if (shutdown)
+		/* Don't release it here/yet! */
+		kernel_sock_shutdown(server->ssocket, SHUT_RDWR);
+}
+
 /*
  * Send a SMB request and set the callback function in the mid to handle
  * the result. Caller is responsible for dealing with timeouts.
@@ -724,6 +752,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
 		revert_current_mid(server, mid->credits);
 		server->sequence_number -= 2;
 		delete_mid(server, mid);
+		cond_sock_shutdown(server);
 	}
 
 	cifs_server_unlock(server);
@@ -982,6 +1011,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 			delete_mid(server, mid[i]);
 			cancelled_mid[i] = true;
 		}
+		cond_sock_shutdown(server);
 	}
 
 	cifs_server_unlock(server);
-- 
2.55.0

[PATCH v3 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
or netfs, 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>
---
v2 -> v3:
- handle CifsExiting status on hard mounts (from sashiko report)

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     | 94 ++++++++++++++++++++++++++++------------
 3 files changed, 72 insertions(+), 27 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index e49140d5c4a4..61191a167b89 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -803,6 +803,7 @@ struct TCP_Server_Info {
 	struct delayed_work reconnect; /* reconnect workqueue job */
 	struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
 	bool need_sock_shutdown; /* true when CifsNeedReconnect was first set by non-cifsd task */
+	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 733d50b042fd..8ea60965ae06 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -374,6 +374,8 @@ static bool cifs_tcp_ses_needs_reconnect(struct TCP_Server_Info *server, int num
 	shutdown = (server->tcpStatus != CifsNeedReconnect || server->need_sock_shutdown);
 	server->need_sock_shutdown = false;
 	server->tcpStatus = CifsNeedReconnect;
+	server->reconnecting = true;
+	wake_up(&server->response_q);
 	spin_unlock(&server->srv_lock);
 
 	if (shutdown) {
@@ -457,6 +459,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);
@@ -599,6 +602,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..3fa4e36aac32 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -1077,44 +1077,84 @@ 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;
 
 	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;
+	}
+
+	/* Hard mounts keep waiting until process is killed or server comes back on-line. */
+	if (retry) {
+		spin_unlock(&server->srv_lock);
+
+		return wait_event_interruptible(server->response_q,
+						(server->tcpStatus != CifsNeedReconnect));
 	}
-	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);
 
-		/* are we still trying to reconnect? */
-		spin_lock(&server->srv_lock);
-		if (server->tcpStatus != CifsNeedReconnect) {
-			spin_unlock(&server->srv_lock);
-			return 0;
+	/* 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);
+
+	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 v3 1/2] smb: client: fast fail sends if need to reconnect

[Paulo Alcantara]

Enzo Matsumiya <ematsumiya@suse.de> writes:

> 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 detected,
> so any sends/receives fails immediately (and don't wait for socket to
> timeout).
>
> Add TCP_Server_Info::need_sock_shutdown to ensure socket is shutdown
> only once.
>
> kernel_sock_shutdown() will be called from whichever task detects it
> first;  cifsd (cifs_tcp_ses_needs_reconnect()), or sending tasks,
> through cifs_call_async() or compound_send_recv().
>
> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> ---
> v2 -> v3 (patch refactor, change strategy but keep concept):
> - remove TCP_Server_Info::mutex_owner (covers sashiko report)
> - shutdown socket directly from sender functions (when need to reconnect)
> - bail out from smb_send_kvec() when need to reconnect only if rc <= 0
>   (from sasiko report)
>
> 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  |  1 +
>  fs/smb/client/connect.c   | 22 +++++++++++++++++++---
>  fs/smb/client/transport.c | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index 79e4e84f8985..e49140d5c4a4 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -802,6 +802,7 @@ struct TCP_Server_Info {
>  	bool	posix_ext_supported;
>  	struct delayed_work reconnect; /* reconnect workqueue job */
>  	struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
> +	bool need_sock_shutdown; /* true when CifsNeedReconnect was first set by non-cifsd task */
>  	unsigned long echo_interval;
>  
>  	/*
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 28e1ddeb6182..733d50b042fd 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -126,12 +126,14 @@ void smb2_query_server_interfaces(struct work_struct *work)
>  	queue_delayed_work(cifsiod_wq, &tcon->query_interfaces,
>  			   (SMB_INTERFACE_POLL_INTERVAL * HZ));
>  }
> -

Why are you removing the blank line?

>  #define set_need_reco(server) \
>  do { \
>  	spin_lock(&server->srv_lock); \
> -	if (server->tcpStatus != CifsExiting) \
> +	if (server->tcpStatus != CifsExiting) { \
> +		if (server->tcpStatus != CifsNeedReconnect) \
> +			server->need_sock_shutdown = true; \
>  		server->tcpStatus = CifsNeedReconnect; \
> +	} \
>  	spin_unlock(&server->srv_lock); \
>  } while (0)
>  
> @@ -353,6 +355,8 @@ cifs_abort_connection(struct TCP_Server_Info *server)
>  
>  static bool cifs_tcp_ses_needs_reconnect(struct TCP_Server_Info *server, int num_targets)
>  {
> +	bool shutdown;
> +
>  	spin_lock(&server->srv_lock);
>  	server->nr_targets = num_targets;
>  	if (server->tcpStatus == CifsExiting) {
> @@ -365,9 +369,21 @@ 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;
>  
> +	/* Cover cases where sender tasks didn't manage to shutdown the socket for some reason */
> +	shutdown = (server->tcpStatus != CifsNeedReconnect || server->need_sock_shutdown);
> +	server->need_sock_shutdown = false;
> +	server->tcpStatus = CifsNeedReconnect;
>  	spin_unlock(&server->srv_lock);
> +
> +	if (shutdown) {
> +		cifs_server_lock(server);
> +		if (server->ssocket)
> +			/* Don't release it here/yet! */
> +			kernel_sock_shutdown(server->ssocket, SHUT_RDWR);
> +		cifs_server_unlock(server);
> +	}
> +

The new TCP_Server_Info::need_sock_shutdown field seems unnecessary.

You could simply call kernel_sock_shutdown() if @server->ssocket != NULL
after releasing ->srv_sock, as the socket can be shut down only once.

>  }
>  
> diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
> index 6e21b5f8754a..a1ae8548cf16 100644
> --- a/fs/smb/client/transport.c
> +++ b/fs/smb/client/transport.c
> @@ -179,7 +179,18 @@ smb_send_kvec(struct TCP_Server_Info *server, struct msghdr *smb_msg,
>  		 * to avoid unnecessary reconnects.
>  		 */
>  		rc = sock_sendmsg(ssocket, smb_msg);
> +
> +		/*
> +		 * If need to reconnect, blame it for any non-interrupt error and bail out early,
> +		 * even if -EAGAIN;  reconnect will never happen in parallel as cifs_reconnect()
> +		 * needs server mutex (which we're already holding), so it's pointless to retry.
> +		 */

Long lines.  Keep them at 80 columns max.

Consider doing the same for the rest of the series.

> +		if (unlikely(rc <= 0 && server->tcpStatus == CifsNeedReconnect &&
> +			     !is_interrupt_error(rc)))
> +			return -ECONNRESET;

You are reading tcpStatus without holding ->srv_lock.  Is that OK?  If
so, you should use READ_ONCE() here.

> +
>  		if (rc == -EAGAIN || unlikely(rc == -EINTR && task_work_pending(current))) {
> +

Remove this new blank line.

>  			retries++;
>  			if (retries >= 14 ||
>  			    (!server->noblocksnd && (retries > 2))) {
> @@ -654,6 +665,23 @@ int wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *mid)
>  	return 0;
>  }
>  
> +static inline void cond_sock_shutdown(struct TCP_Server_Info *server)

inline is unnecessary.  This isn't even hot path.

> +{
> +	bool shutdown;
> +
> +	lockdep_assert_held(&server->_srv_mutex);
> +
> +	spin_lock(&server->srv_lock);
> +	/* No need to check status, need_sock_shutdown is only set if CifsNeedReconnect */
> +	shutdown = (server->need_sock_shutdown && server->ssocket);

Again, TCP_Server_info::need_sock_shutdown doesn't seem to be required
as you could have done

        shutdown = server->tcpStatus == CifsNeedReconnect && server->ssocket;

Besides, the parentheses are unnecessary, so remove them.

Otherwise, looks good.

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

[Enzo Matsumiya]

On 10/06, Paulo Alcantara wrote:
>Enzo Matsumiya <ematsumiya@suse.de> writes:
>
>> 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 detected,
>> so any sends/receives fails immediately (and don't wait for socket to
>> timeout).
>>
>> Add TCP_Server_Info::need_sock_shutdown to ensure socket is shutdown
>> only once.
>>
>> kernel_sock_shutdown() will be called from whichever task detects it
>> first;  cifsd (cifs_tcp_ses_needs_reconnect()), or sending tasks,
>> through cifs_call_async() or compound_send_recv().
>>
>> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
>> ---
>> v2 -> v3 (patch refactor, change strategy but keep concept):
>> - remove TCP_Server_Info::mutex_owner (covers sashiko report)
>> - shutdown socket directly from sender functions (when need to reconnect)
>> - bail out from smb_send_kvec() when need to reconnect only if rc <= 0
>>   (from sasiko report)
>>
>> 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  |  1 +
>>  fs/smb/client/connect.c   | 22 +++++++++++++++++++---
>>  fs/smb/client/transport.c | 30 ++++++++++++++++++++++++++++++
>>  3 files changed, 50 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
>> index 79e4e84f8985..e49140d5c4a4 100644
>> --- a/fs/smb/client/cifsglob.h
>> +++ b/fs/smb/client/cifsglob.h
>> @@ -802,6 +802,7 @@ struct TCP_Server_Info {
>>  	bool	posix_ext_supported;
>>  	struct delayed_work reconnect; /* reconnect workqueue job */
>>  	struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
>> +	bool need_sock_shutdown; /* true when CifsNeedReconnect was first set by non-cifsd task */
>>  	unsigned long echo_interval;
>>
>>  	/*
>> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
>> index 28e1ddeb6182..733d50b042fd 100644
>> --- a/fs/smb/client/connect.c
>> +++ b/fs/smb/client/connect.c
>> @@ -126,12 +126,14 @@ void smb2_query_server_interfaces(struct work_struct *work)
>>  	queue_delayed_work(cifsiod_wq, &tcon->query_interfaces,
>>  			   (SMB_INTERFACE_POLL_INTERVAL * HZ));
>>  }
>> -
>
>Why are you removing the blank line?

Probably just because I saw it.
Not sure why it should stay there, but I'll drop it.

>>  #define set_need_reco(server) \
>>  do { \
>>  	spin_lock(&server->srv_lock); \
>> -	if (server->tcpStatus != CifsExiting) \
>> +	if (server->tcpStatus != CifsExiting) { \
>> +		if (server->tcpStatus != CifsNeedReconnect) \
>> +			server->need_sock_shutdown = true; \
>>  		server->tcpStatus = CifsNeedReconnect; \
>> +	} \
>>  	spin_unlock(&server->srv_lock); \
>>  } while (0)
>>
>> @@ -353,6 +355,8 @@ cifs_abort_connection(struct TCP_Server_Info *server)
>>
>>  static bool cifs_tcp_ses_needs_reconnect(struct TCP_Server_Info *server, int num_targets)
>>  {
>> +	bool shutdown;
>> +
>>  	spin_lock(&server->srv_lock);
>>  	server->nr_targets = num_targets;
>>  	if (server->tcpStatus == CifsExiting) {
>> @@ -365,9 +369,21 @@ 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;
>>
>> +	/* Cover cases where sender tasks didn't manage to shutdown the socket for some reason */
>> +	shutdown = (server->tcpStatus != CifsNeedReconnect || server->need_sock_shutdown);
>> +	server->need_sock_shutdown = false;
>> +	server->tcpStatus = CifsNeedReconnect;
>>  	spin_unlock(&server->srv_lock);
>> +
>> +	if (shutdown) {
>> +		cifs_server_lock(server);
>> +		if (server->ssocket)
>> +			/* Don't release it here/yet! */
>> +			kernel_sock_shutdown(server->ssocket, SHUT_RDWR);
>> +		cifs_server_unlock(server);
>> +	}
>> +
>
>The new TCP_Server_Info::need_sock_shutdown field seems unnecessary.
>
>You could simply call kernel_sock_shutdown() if @server->ssocket != NULL
>after releasing ->srv_sock, as the socket can be shut down only once.

The idea is to ensure the socket is shutdown only once from cifs POV,
not TCP/net -- e.g. there could be simultaneous threads reaching the
same code and shutdown the socket right after cifs_reconnect()
successfully connected.

>>  }
>>
>> diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
>> index 6e21b5f8754a..a1ae8548cf16 100644
>> --- a/fs/smb/client/transport.c
>> +++ b/fs/smb/client/transport.c
>> @@ -179,7 +179,18 @@ smb_send_kvec(struct TCP_Server_Info *server, struct msghdr *smb_msg,
>>  		 * to avoid unnecessary reconnects.
>>  		 */
>>  		rc = sock_sendmsg(ssocket, smb_msg);
>> +
>> +		/*
>> +		 * If need to reconnect, blame it for any non-interrupt error and bail out early,
>> +		 * even if -EAGAIN;  reconnect will never happen in parallel as cifs_reconnect()
>> +		 * needs server mutex (which we're already holding), so it's pointless to retry.
>> +		 */
>
>Long lines.  Keep them at 80 columns max.
>
>Consider doing the same for the rest of the series.

checkpatch.pl has a default max_line_length = 100.  I was (have been)
just following that, but sure I can change it.

>
>> +		if (unlikely(rc <= 0 && server->tcpStatus == CifsNeedReconnect &&
>> +			     !is_interrupt_error(rc)))
>> +			return -ECONNRESET;
>
>You are reading tcpStatus without holding ->srv_lock.  Is that OK?  If
>so, you should use READ_ONCE() here.

Ack, I'll add READ_ONCE().
Unlocked read should be ok; it's a very specific condition, and even if
it changes e.g. after returning, it means it reconnected, and we need to
return some error anyway.

>> +
>>  		if (rc == -EAGAIN || unlikely(rc == -EINTR && task_work_pending(current))) {
>> +
>
>Remove this new blank line.

Wait, I thought we were keeping blank lines?! lol I'll remove it.

>
>>  			retries++;
>>  			if (retries >= 14 ||
>>  			    (!server->noblocksnd && (retries > 2))) {
>> @@ -654,6 +665,23 @@ int wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *mid)
>>  	return 0;
>>  }
>>
>> +static inline void cond_sock_shutdown(struct TCP_Server_Info *server)
>
>inline is unnecessary.  This isn't even hot path.

Ack.

>> +{
>> +	bool shutdown;
>> +
>> +	lockdep_assert_held(&server->_srv_mutex);
>> +
>> +	spin_lock(&server->srv_lock);
>> +	/* No need to check status, need_sock_shutdown is only set if CifsNeedReconnect */
>> +	shutdown = (server->need_sock_shutdown && server->ssocket);
>
>Again, TCP_Server_info::need_sock_shutdown doesn't seem to be required
>as you could have done
>
>        shutdown = server->tcpStatus == CifsNeedReconnect && server->ssocket;

cf. comment above -- it's specially true for this case, as
cond_sock_shutdown() is called from cifs_call_async() and
compound_send_recv(), which increases the chances of them shutting
down the socket right after it's connected.

(because they're reachable from userspace, whereas the above shutdown is
done by cifsd, which we control)

>Besides, the parentheses are unnecessary, so remove them.

Ack.

>Otherwise, looks good.


Thanks!  I'll send v4 addressing the issues you pointed out.


Enzo

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

[Paulo Alcantara]

Enzo Matsumiya <ematsumiya@suse.de> writes:

> 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
> or netfs, 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>
> ---
> v2 -> v3:
> - handle CifsExiting status on hard mounts (from sashiko report)
>
> 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     | 94 ++++++++++++++++++++++++++++------------
>  3 files changed, 72 insertions(+), 27 deletions(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index e49140d5c4a4..61191a167b89 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -803,6 +803,7 @@ struct TCP_Server_Info {
>  	struct delayed_work reconnect; /* reconnect workqueue job */
>  	struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
>  	bool need_sock_shutdown; /* true when CifsNeedReconnect was first set by non-cifsd task */
> +	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 733d50b042fd..8ea60965ae06 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -374,6 +374,8 @@ static bool cifs_tcp_ses_needs_reconnect(struct TCP_Server_Info *server, int num
>  	shutdown = (server->tcpStatus != CifsNeedReconnect || server->need_sock_shutdown);
>  	server->need_sock_shutdown = false;
>  	server->tcpStatus = CifsNeedReconnect;
> +	server->reconnecting = true;
> +	wake_up(&server->response_q);
>  	spin_unlock(&server->srv_lock);
>  
>  	if (shutdown) {
> @@ -457,6 +459,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);
> @@ -599,6 +602,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..3fa4e36aac32 100644
> --- a/fs/smb/client/misc.c
> +++ b/fs/smb/client/misc.c
> @@ -1077,44 +1077,84 @@ 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;
>  
>  	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;
> +	}
> +
> +	/* Hard mounts keep waiting until process is killed or server comes back on-line. */
> +	if (retry) {
> +		spin_unlock(&server->srv_lock);
> +
> +		return wait_event_interruptible(server->response_q,
> +						(server->tcpStatus != CifsNeedReconnect));

Hrm - given the above comment, makes me think that we should be calling
wait_event_killable() instead on hard mounts.  But that would change
current behavior, so it's probably better to handle it separately.

What do you think?

>  	}
> -	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);

I know you've intentionally increased the timeout, but my worry is that
I/O will now fail with -ECONNRESET/-EHOSTDOWN after 180-240s instead of
10s on soft mounts with a single target server?

Are you sure that the best option is to rely on the echo_interval value?

The rest looks good, thanks for fixing this issue.

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

[Enzo Matsumiya]

On 10/06, Paulo Alcantara wrote:
>Enzo Matsumiya <ematsumiya@suse.de> writes:
>
>> 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
>> or netfs, 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>
>> ---
>> v2 -> v3:
>> - handle CifsExiting status on hard mounts (from sashiko report)
>>
>> 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     | 94 ++++++++++++++++++++++++++++------------
>>  3 files changed, 72 insertions(+), 27 deletions(-)
>>
>> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
>> index e49140d5c4a4..61191a167b89 100644
>> --- a/fs/smb/client/cifsglob.h
>> +++ b/fs/smb/client/cifsglob.h
>> @@ -803,6 +803,7 @@ struct TCP_Server_Info {
>>  	struct delayed_work reconnect; /* reconnect workqueue job */
>>  	struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
>>  	bool need_sock_shutdown; /* true when CifsNeedReconnect was first set by non-cifsd task */
>> +	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 733d50b042fd..8ea60965ae06 100644
>> --- a/fs/smb/client/connect.c
>> +++ b/fs/smb/client/connect.c
>> @@ -374,6 +374,8 @@ static bool cifs_tcp_ses_needs_reconnect(struct TCP_Server_Info *server, int num
>>  	shutdown = (server->tcpStatus != CifsNeedReconnect || server->need_sock_shutdown);
>>  	server->need_sock_shutdown = false;
>>  	server->tcpStatus = CifsNeedReconnect;
>> +	server->reconnecting = true;
>> +	wake_up(&server->response_q);
>>  	spin_unlock(&server->srv_lock);
>>
>>  	if (shutdown) {
>> @@ -457,6 +459,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);
>> @@ -599,6 +602,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..3fa4e36aac32 100644
>> --- a/fs/smb/client/misc.c
>> +++ b/fs/smb/client/misc.c
>> @@ -1077,44 +1077,84 @@ 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;
>>
>>  	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;
>> +	}
>> +
>> +	/* Hard mounts keep waiting until process is killed or server comes back on-line. */
>> +	if (retry) {
>> +		spin_unlock(&server->srv_lock);
>> +
>> +		return wait_event_interruptible(server->response_q,
>> +						(server->tcpStatus != CifsNeedReconnect));
>
>Hrm - given the above comment, makes me think that we should be calling
>wait_event_killable() instead on hard mounts.  But that would change
>current behavior, so it's probably better to handle it separately.
>
>What do you think?

Well, I don't see any real change in behavior, but can you elaborate
further please?  I don't understand what would be the advantage or
necessity of this change.

>>  	}
>> -	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);
>
>I know you've intentionally increased the timeout, but my worry is that
>I/O will now fail with -ECONNRESET/-EHOSTDOWN after 180-240s instead of
>10s on soft mounts with a single target server?

Note that max timeout is capped at 60s.
But yes, without patch 1, it's entirely possible to wait that much.

(I'm assuming we're both considering the server to be down for >60s)

I just increased the timeout as a preventive measure, because now we
can afford to wait longer (because of patch 1 "fail fast").

The core idea is to have more requests retried after a reconnect,
instead of just missing most of them (-EHOSTDOWN, not retryable).

>Are you sure that the best option is to rely on the echo_interval value?

 From the comment:

>> +	 * Compute timeout based on echo_interval, to match user expectations wrt. time to recover
>> +	 * from network interruptions.

Since we compute a time based on echo_interval to consider a server
as unresponsive on the receiving side, I thought it'd be fair to use
the same base on the sending side.

Also, it's a way to have it:
- (somewhat) user-configurable
- (somewhat) standardized, e.g. "why are my requests failing in 10s if
   my server has been down for only 30s, and echo_interval is 60s?"

>The rest looks good, thanks for fixing this issue.

Thanks for reviewing!

Since you mentioned "180-240s" timeout, I assume you considered
3*echo_interval (like e.g. server_unresponsive()), which sure looks
excessive, but I used echo_interval (1x) only.

But again, echo_interval is just a base, maximum is 60s.
Let me know if I should still change that.


Cheers,

Enzo


============
Extra topics:

cifs_wait_for_server_reconnect() is just waiting for server to
reconnect.  When it times out, it means reconnect took too long.
i.e it has no ability whatsoever to determine whether the server is
really down, other than tcpStatus still begin CifsNeedReconnect.

- shouldn't it always return some retryable error because of that?

cifs_reconnect() keeps running indefinitely, even on soft mounts, even
if the server is never coming back again.

- shouldn't it have limited retries (for soft mounts)?  it could be
   re-triggered e.g. on the next send attempt

This would also allow cifs_wait_for_server_reconnect() to return a
retryable error safely, as I/O would never get into some infinite retry
loop.

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

[Paulo Alcantara]

Enzo Matsumiya <ematsumiya@suse.de> writes:

> On 10/06, Paulo Alcantara wrote:
>>> diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
>>> index 05168284f205..3fa4e36aac32 100644
>>> --- a/fs/smb/client/misc.c
>>> +++ b/fs/smb/client/misc.c
>>> @@ -1077,44 +1077,84 @@ 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;
>>>
>>>  	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;
>>> +	}
>>> +
>>> +	/* Hard mounts keep waiting until process is killed or server comes back on-line. */
>>> +	if (retry) {
>>> +		spin_unlock(&server->srv_lock);
>>> +
>>> +		return wait_event_interruptible(server->response_q,
>>> +						(server->tcpStatus != CifsNeedReconnect));
>>
>>Hrm - given the above comment, makes me think that we should be calling
>>wait_event_killable() instead on hard mounts.  But that would change
>>current behavior, so it's probably better to handle it separately.
>>
>>What do you think?
>
> Well, I don't see any real change in behavior, but can you elaborate
> further please?  I don't understand what would be the advantage or
> necessity of this change.

What I mean is that calling wait_event_interruptible() doesn't exactly match

        /* Hard mounts keep waiting until process is killed or server comes back on-line. */

or 'hard' mount description in mount.cifs(8).

SIGINT could interrupt that.  That's why I suggested calling
wait_event_killable() instead.  This isn't a problem with your patch, so
don't need to worry about that.

>>>  	}
>>> -	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);
>>
>>I know you've intentionally increased the timeout, but my worry is that
>>I/O will now fail with -ECONNRESET/-EHOSTDOWN after 180-240s instead of
>>10s on soft mounts with a single target server?
>
> Note that max timeout is capped at 60s.

Yes, but you're changing it from 10s to 60s with default mount options.
That's a big difference.  I recall people complaining about having to
wait 10s on soft mounts.  There was even an early patch sent to the ML
getting rid of that default 10s.

> But yes, without patch 1, it's entirely possible to wait that much.
>
> (I'm assuming we're both considering the server to be down for >60s)
>
> I just increased the timeout as a preventive measure, because now we
> can afford to wait longer (because of patch 1 "fail fast").
>
> The core idea is to have more requests retried after a reconnect,
> instead of just missing most of them (-EHOSTDOWN, not retryable).

I understand.  But with soft mounts, we ought to fail it as quickly as
possible and then let caller decide whether it will retry the request or
not.  Having them now to wait for ~60s to either get either -EHOSTDOWN
or -EAGAIN doesn't look good to me.

>>Are you sure that the best option is to rely on the echo_interval value?
>
>  From the comment:
>
>>> +	 * Compute timeout based on echo_interval, to match user expectations wrt. time to recover
>>> +	 * from network interruptions.
>
> Since we compute a time based on echo_interval to consider a server
> as unresponsive on the receiving side, I thought it'd be fair to use
> the same base on the sending side.
>
> Also, it's a way to have it:
> - (somewhat) user-configurable
> - (somewhat) standardized, e.g. "why are my requests failing in 10s if
>    my server has been down for only 30s, and echo_interval is 60s?"
>
>>The rest looks good, thanks for fixing this issue.
>
> Thanks for reviewing!
>
> Since you mentioned "180-240s" timeout, I assume you considered
> 3*echo_interval (like e.g. server_unresponsive()), which sure looks
> excessive, but I used echo_interval (1x) only.
>
> But again, echo_interval is just a base, maximum is 60s.
> Let me know if I should still change that.

It's fine, as long as we don't hang too long on soft mounts.

> cifs_wait_for_server_reconnect() is just waiting for server to
> reconnect.  When it times out, it means reconnect took too long.
> i.e it has no ability whatsoever to determine whether the server is
> really down, other than tcpStatus still begin CifsNeedReconnect.
>
> - shouldn't it always return some retryable error because of that?

No.  It is up to the caller to decide whether it will return a retryable
error.  Returning a retryable error when the server is down might not
make sense.  smb2_reconnect() does return a retryable error from certain
SMB commands, but at that point the TCP connection and SMB session were
re-established.

> cifs_reconnect() keeps running indefinitely, even on soft mounts, even
> if the server is never coming back again.

Do you mean in cifsd?  If so, I don't see any problem with that.  Better
than keeping it idle.

That could even help the next I/O to not waste any time trying to
reconnect to the server, as cifsd already did that.
smb2_reconnect_server() worker also helps with trying to re-establish a
session with the server.

> - shouldn't it have limited retries (for soft mounts)?  it could be
>    re-triggered e.g. on the next send attempt
>
> This would also allow cifs_wait_for_server_reconnect() to return a
> retryable error safely, as I/O would never get into some infinite retry
> loop.

Note that there's currently no backoff mechanism in either CIFS or
netfslib for writeback, meaning that the write requests will be retried
forever if the server goes offline.  You'll end up with tasks hanging
forever on close or fsync.  Trying to fix that in the reconnect path
doesn't seem the right way to do it, IMO.

In the non-writepack path, we definitely want to return as quickly as
possible on soft mounts and then let userspace decide whether it will
retry the request or not.

Please let me know what I'm missing as you could have more information
on the issues that your customers reported.

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

[Enzo Matsumiya]

On 10/06, Paulo Alcantara wrote:
>Enzo Matsumiya <ematsumiya@suse.de> writes:
>
>> On 10/06, Paulo Alcantara wrote:
>>>> diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
>>>> index 05168284f205..3fa4e36aac32 100644
>>>> --- a/fs/smb/client/misc.c
>>>> +++ b/fs/smb/client/misc.c
>>>> @@ -1077,44 +1077,84 @@ 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;
>>>>
>>>>  	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;
>>>> +	}
>>>> +
>>>> +	/* Hard mounts keep waiting until process is killed or server comes back on-line. */
>>>> +	if (retry) {
>>>> +		spin_unlock(&server->srv_lock);
>>>> +
>>>> +		return wait_event_interruptible(server->response_q,
>>>> +						(server->tcpStatus != CifsNeedReconnect));
>>>
>>>Hrm - given the above comment, makes me think that we should be calling
>>>wait_event_killable() instead on hard mounts.  But that would change
>>>current behavior, so it's probably better to handle it separately.
>>>
>>>What do you think?
>>
>> Well, I don't see any real change in behavior, but can you elaborate
>> further please?  I don't understand what would be the advantage or
>> necessity of this change.
>
>What I mean is that calling wait_event_interruptible() doesn't exactly match
>
>        /* Hard mounts keep waiting until process is killed or server comes back on-line. */
>
>or 'hard' mount description in mount.cifs(8).
>
>SIGINT could interrupt that.  That's why I suggested calling
>wait_event_killable() instead.  This isn't a problem with your patch, so
>don't need to worry about that.

Got it.  Your suggestion indeed makes sense.

>>>>  	}
>>>> -	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);
>>>
>>>I know you've intentionally increased the timeout, but my worry is that
>>>I/O will now fail with -ECONNRESET/-EHOSTDOWN after 180-240s instead of
>>>10s on soft mounts with a single target server?
>>
>> Note that max timeout is capped at 60s.
>
>Yes, but you're changing it from 10s to 60s with default mount options.
>That's a big difference.  I recall people complaining about having to
>wait 10s on soft mounts.  There was even an early patch sent to the ML
>getting rid of that default 10s.

Ack.

>> But yes, without patch 1, it's entirely possible to wait that much.
>>
>> (I'm assuming we're both considering the server to be down for >60s)
>>
>> I just increased the timeout as a preventive measure, because now we
>> can afford to wait longer (because of patch 1 "fail fast").
>>
>> The core idea is to have more requests retried after a reconnect,
>> instead of just missing most of them (-EHOSTDOWN, not retryable).
>
>I understand.  But with soft mounts, we ought to fail it as quickly as
>possible and then let caller decide whether it will retry the request or
>not.  Having them now to wait for ~60s to either get either -EHOSTDOWN
>or -EAGAIN doesn't look good to me.

I kind of agree with that, it's just that increasing the timeout was the
smoothest way to handle the scenario I mentioned.

Just a small note here regarding "let caller decide to retry":
-EHOSTDOWN is not considered, or aliased to, a retryable error
anywhere in cifs, and usually neither by most userspace applications.

i.e. it always bubbles up to userspace and applications are terminated.

>>>Are you sure that the best option is to rely on the echo_interval value?
>>
>>  From the comment:
>>
>>>> +	 * Compute timeout based on echo_interval, to match user expectations wrt. time to recover
>>>> +	 * from network interruptions.
>>
>> Since we compute a time based on echo_interval to consider a server
>> as unresponsive on the receiving side, I thought it'd be fair to use
>> the same base on the sending side.
>>
>> Also, it's a way to have it:
>> - (somewhat) user-configurable
>> - (somewhat) standardized, e.g. "why are my requests failing in 10s if
>>    my server has been down for only 30s, and echo_interval is 60s?"
>>
>>>The rest looks good, thanks for fixing this issue.
>>
>> Thanks for reviewing!
>>
>> Since you mentioned "180-240s" timeout, I assume you considered
>> 3*echo_interval (like e.g. server_unresponsive()), which sure looks
>> excessive, but I used echo_interval (1x) only.
>>
>> But again, echo_interval is just a base, maximum is 60s.
>> Let me know if I should still change that.
>
>It's fine, as long as we don't hang too long on soft mounts.
>
>> cifs_wait_for_server_reconnect() is just waiting for server to
>> reconnect.  When it times out, it means reconnect took too long.
>> i.e it has no ability whatsoever to determine whether the server is
>> really down, other than tcpStatus still begin CifsNeedReconnect.
>>
>> - shouldn't it always return some retryable error because of that?
>
>No.  It is up to the caller to decide whether it will return a retryable
>error.  Returning a retryable error when the server is down might not
>make sense.

My point was that cifs_wait_for_server_reconnect() has no way to
know/determine whether the server is really down -- especially in a 10s
window, and even more especially when it waited 10s but
cifs_reconnect() didn't even start yet.

cifs_reconnect() is the only one that could possibly determine that
in this case;  it can count the number of failed attempts, act based
on error code, check socket state, etc.

Ideally, IMHO, that could be identifiable by yet another tcpStatus,
one that represents something like "we failed to reconnect too many
times to this server, but it's not supposed to go away yet", e.g.
CifsReconnectFailed, or CifsServerDown, or whatever.

Then set it after e.g. 3 reconnect failures in cifs_reconnect(),
but keep retrying regardless.

This way, cifs_wait_for_server_reconnect() could interpret it as:
- CifsNeedReconnect -> cifs_reconnect() might be running, not many
   failures -> can wait a little longer, return -EAGAIN
- CifsServerDown -> cifs_reconnect() already tried and failed too
   many times -> don't wait, return -EHOSTDOWN

Thoughts on that?

> smb2_reconnect() does return a retryable error from certain
>SMB commands, but at that point the TCP connection and SMB session were
>re-established.
>
>> cifs_reconnect() keeps running indefinitely, even on soft mounts, even
>> if the server is never coming back again.
>
>Do you mean in cifsd?  If so, I don't see any problem with that.  Better
>than keeping it idle.
>
>That could even help the next I/O to not waste any time trying to
>reconnect to the server, as cifsd already did that.
>smb2_reconnect_server() worker also helps with trying to re-establish a
>session with the server.

Ack.

>> - shouldn't it have limited retries (for soft mounts)?  it could be
>>    re-triggered e.g. on the next send attempt
>>
>> This would also allow cifs_wait_for_server_reconnect() to return a
>> retryable error safely, as I/O would never get into some infinite retry
>> loop.
>
>Note that there's currently no backoff mechanism in either CIFS or
>netfslib for writeback, meaning that the write requests will be retried
>forever if the server goes offline.  You'll end up with tasks hanging
>forever on close or fsync.

Yes, that was actually the main bug I wanted to fix with patch 1.

I don't know if you said "forever" as an exaggeration or literally, but
from my tests, I did experience an application taking like 2h to terminate
after dropping the network.

I could observe the number of writer tasks decreasing, slowly, but I
never got an indication that they'd be hanging forever (literally).

Can you try patch 1 with your reproducer please?  And let me know how it
works for you.

This patch 2 should not be necessary for that case.

>Trying to fix that in the reconnect path
>doesn't seem the right way to do it, IMO.
>
>In the non-writepack path, we definitely want to return as quickly as
>possible on soft mounts and then let userspace decide whether it will
>retry the request or not.
>
>Please let me know what I'm missing as you could have more information
>on the issues that your customers reported.

Not really.  Your observation about the writeback path is spot on -- it
was precisely what the reports were about.

Handling in cifs reconnect path was just easier given that we have
kernels that are pre- or mid-netfs migration.

I'll send v4 addressing the solid concerns you mentioned.

However, I'd like very much to keep discussing the other concerns.
I think reconnect in cifs needs a lot more attention, e.g. I've had
(non-customer) issues with failover in DFS, multichannel, and
Windows clusters.


Cheers,

Enzo

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

[Enzo Matsumiya]

On 10/06, Paulo Alcantara wrote:
>Note that there's currently no backoff mechanism in either CIFS or
>netfslib for writeback, meaning that the write requests will be retried
>forever if the server goes offline.  You'll end up with tasks hanging
>forever on close or fsync.  Trying to fix that in the reconnect path
>doesn't seem the right way to do it, IMO.

Btw, I forgot perhaps the most important bit here -- whenever there's
a reconnect during a write, the file gets corrupted.

That's regardless of how long the downtime was.  As soon as the first
task times out in cifs_wait_for_server_reconnect(), returning
-EHOSTDOWN, that request is lost, and data is corrupted.

Note that the writes (operations) _are_ resumed, but what was lost,
stays lost.

Again, I agree that retrying indefinitely might be wrong, but so is
corrupting data due to a short network outage.

I did try to relieve this situation with this patch, and TBH, after
reassessing it, I fail to see how/where this could be handled in netfs
or cifs I/O path.

The closest I got to a PoC was by loosening the restrictions to set
NETFS_SREQ_NEED_RETRY on smb2_writev_callback() and smb2_async_writev(),
and then we're obviously back to the retry-forever-but-no-corruption
dilemma.


Cheers,

Enzo

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

[Paulo Alcantara]

Enzo Matsumiya <ematsumiya@suse.de> writes:

> On 10/06, Paulo Alcantara wrote:
>>Enzo Matsumiya <ematsumiya@suse.de> writes:
>>>
>>> But yes, without patch 1, it's entirely possible to wait that much.
>>>
>>> (I'm assuming we're both considering the server to be down for >60s)
>>>
>>> I just increased the timeout as a preventive measure, because now we
>>> can afford to wait longer (because of patch 1 "fail fast").
>>>
>>> The core idea is to have more requests retried after a reconnect,
>>> instead of just missing most of them (-EHOSTDOWN, not retryable).
>>
>>I understand.  But with soft mounts, we ought to fail it as quickly as
>>possible and then let caller decide whether it will retry the request or
>>not.  Having them now to wait for ~60s to either get either -EHOSTDOWN
>>or -EAGAIN doesn't look good to me.
>
> I kind of agree with that, it's just that increasing the timeout was the
> smoothest way to handle the scenario I mentioned.
>
> Just a small note here regarding "let caller decide to retry":
> -EHOSTDOWN is not considered, or aliased to, a retryable error
> anywhere in cifs, and usually neither by most userspace applications.

Yes, I know.

Consider an userspace application that reads and writes to a file and
retries on -EAGAIN without any backoff mechanism.  If we return a
retryable error when the server goes offline and it never comes back,
the userspace application will hang.

If the user _really_ wants to make sure that every read and write call
goes to the server -- even when it goes offline for a while -- then
'hard' mount option is the best option.

>>> cifs_wait_for_server_reconnect() is just waiting for server to
>>> reconnect.  When it times out, it means reconnect took too long.
>>> i.e it has no ability whatsoever to determine whether the server is
>>> really down, other than tcpStatus still begin CifsNeedReconnect.
>>>
>>> - shouldn't it always return some retryable error because of that?
>>
>>No.  It is up to the caller to decide whether it will return a retryable
>>error.  Returning a retryable error when the server is down might not
>>make sense.
>
> My point was that cifs_wait_for_server_reconnect() has no way to
> know/determine whether the server is really down -- especially in a 10s
> window, and even more especially when it waited 10s but
> cifs_reconnect() didn't even start yet.
>
> cifs_reconnect() is the only one that could possibly determine that
> in this case;  it can count the number of failed attempts, act based
> on error code, check socket state, etc.

I disagree.  I'd rather keep it simple to just care about
re-establishing TCP connection to the server.  Handle the retries
somewhere else.  See below.

> Ideally, IMHO, that could be identifiable by yet another tcpStatus,
> one that represents something like "we failed to reconnect too many
> times to this server, but it's not supposed to go away yet", e.g.
> CifsReconnectFailed, or CifsServerDown, or whatever.
>
> Then set it after e.g. 3 reconnect failures in cifs_reconnect(),
> but keep retrying regardless.
>
> This way, cifs_wait_for_server_reconnect() could interpret it as:
> - CifsNeedReconnect -> cifs_reconnect() might be running, not many
>    failures -> can wait a little longer, return -EAGAIN
> - CifsServerDown -> cifs_reconnect() already tried and failed too
>    many times -> don't wait, return -EHOSTDOWN
>
> Thoughts on that?

Well, I assumed that the new @reconnecting field you introduced would
help with that.

Why wouldn't checking CifsNeedReconnect and
TCP_Server_Info::reconnecting be enough to know that reconnect actually
started but then failed?

About the retries, we already have 'retrans=' mount option.  Why
couldn't we fix that and then handle such retries gracefully?  If
someone doesn't care about retries and wants it to fail as quickly as
possible, 

>> smb2_reconnect() does return a retryable error from certain
>>SMB commands, but at that point the TCP connection and SMB session were
>>re-established.
>>
>>> cifs_reconnect() keeps running indefinitely, even on soft mounts, even
>>> if the server is never coming back again.
>>
>>Do you mean in cifsd?  If so, I don't see any problem with that.  Better
>>than keeping it idle.
>>
>>That could even help the next I/O to not waste any time trying to
>>reconnect to the server, as cifsd already did that.
>>smb2_reconnect_server() worker also helps with trying to re-establish a
>>session with the server.
>
> Ack.
>
>>> - shouldn't it have limited retries (for soft mounts)?  it could be
>>>    re-triggered e.g. on the next send attempt
>>>
>>> This would also allow cifs_wait_for_server_reconnect() to return a
>>> retryable error safely, as I/O would never get into some infinite retry
>>> loop.
>>
>>Note that there's currently no backoff mechanism in either CIFS or
>>netfslib for writeback, meaning that the write requests will be retried
>>forever if the server goes offline.  You'll end up with tasks hanging
>>forever on close or fsync.
>
> Yes, that was actually the main bug I wanted to fix with patch 1.
>
> I don't know if you said "forever" as an exaggeration or literally, but
> from my tests, I did experience an application taking like 2h to terminate
> after dropping the network.
>
> I could observe the number of writer tasks decreasing, slowly, but I
> never got an indication that they'd be hanging forever (literally).
>
> Can you try patch 1 with your reproducer please?  And let me know how it
> works for you.
>
> This patch 2 should not be necessary for that case.
>
>>Trying to fix that in the reconnect path
>>doesn't seem the right way to do it, IMO.
>>
>>In the non-writepack path, we definitely want to return as quickly as
>>possible on soft mounts and then let userspace decide whether it will
>>retry the request or not.
>>
>>Please let me know what I'm missing as you could have more information
>>on the issues that your customers reported.
>
> Not really.  Your observation about the writeback path is spot on -- it
> was precisely what the reports were about.
>
> Handling in cifs reconnect path was just easier given that we have
> kernels that are pre- or mid-netfs migration.

Yes, I get it.  Let me know what you think on having 'retrans=' fixed
for soft mounts.

> I'll send v4 addressing the solid concerns you mentioned.

Awesome, thanks!

> However, I'd like very much to keep discussing the other concerns.
> I think reconnect in cifs needs a lot more attention, e.g. I've had
> (non-customer) issues with failover in DFS, multichannel, and
> Windows clusters.

Absolutely.  Unfortunately it was designed to handle reconnects to a
single target server, but then we started changing it to handle more
complex scenarios like DFS failover, multichannel, etc.

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

[Paulo Alcantara]

Enzo Matsumiya <ematsumiya@suse.de> writes:

> On 10/06, Paulo Alcantara wrote:
>>Note that there's currently no backoff mechanism in either CIFS or
>>netfslib for writeback, meaning that the write requests will be retried
>>forever if the server goes offline.  You'll end up with tasks hanging
>>forever on close or fsync.  Trying to fix that in the reconnect path
>>doesn't seem the right way to do it, IMO.
>
> Btw, I forgot perhaps the most important bit here -- whenever there's
> a reconnect during a write, the file gets corrupted.
>
> That's regardless of how long the downtime was.  As soon as the first
> task times out in cifs_wait_for_server_reconnect(), returning
> -EHOSTDOWN, that request is lost, and data is corrupted.

Yes, because we explicitly return a non-retryable error if the server
neve came back online.

Then we're back to whether using 'hard' mount option or 'retrans=' to
handle such cases.

> Note that the writes (operations) _are_ resumed, but what was lost,
> stays lost.
>
> Again, I agree that retrying indefinitely might be wrong, but so is
> corrupting data due to a short network outage.

I agree.  Check the available mount options (hard and retrans=) to see
if they help with your case.

> I did try to relieve this situation with this patch, and TBH, after
> reassessing it, I fail to see how/where this could be handled in netfs
> or cifs I/O path.

Reconnects and retries are definitely not easy to handle.  This,
however, makes network filesystems quite interesting to work with,
though :-)

> The closest I got to a PoC was by loosening the restrictions to set
> NETFS_SREQ_NEED_RETRY on smb2_writev_callback() and smb2_async_writev(),
> and then we're obviously back to the retry-forever-but-no-corruption
> dilemma.

Yes.  Let me know what you think and let's keep talking on how to solve
these issues.

Thanks for looking into them!