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:

> On 10/06, Paulo Alcantara wrote:
>>Enzo Matsumiya <ematsumiya@suse.de> writes:
>>>
>>> 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.

Yes, I know.

Consider an userspace application that reads and writes to a file and
retries on -EAGAIN without any backoff mechanism.  If we return a
retryable error when the server goes offline and it never comes back,
the userspace application will hang.

If the user _really_ wants to make sure that every read and write call
goes to the server -- even when it goes offline for a while -- then
'hard' mount option is the best option.

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

I disagree.  I'd rather keep it simple to just care about
re-establishing TCP connection to the server.  Handle the retries
somewhere else.  See below.

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

Well, I assumed that the new @reconnecting field you introduced would
help with that.

Why wouldn't checking CifsNeedReconnect and
TCP_Server_Info::reconnecting be enough to know that reconnect actually
started but then failed?

About the retries, we already have 'retrans=' mount option.  Why
couldn't we fix that and then handle such retries gracefully?  If
someone doesn't care about retries and wants it to fail as quickly as
possible, 

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

Yes, I get it.  Let me know what you think on having 'retrans=' fixed
for soft mounts.

> I'll send v4 addressing the solid concerns you mentioned.

Awesome, thanks!

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

Absolutely.  Unfortunately it was designed to handle reconnects to a
single target server, but then we started changing it to handle more
complex scenarios like DFS failover, multichannel, etc.