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

[Enzo Matsumiya <ematsumiya@suse.de>]

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.