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

[Paulo Alcantara <pc@manguebit.org>]

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.