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:
>
>> 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