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

[Enzo Matsumiya <ematsumiya@suse.de>]

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