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

[Paulo Alcantara <pc@manguebit.org>]

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.