linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thiago Rafael Becker <trbecker@gmail.com>
To: "Aurélien Aptel" <aaptel@suse.com>
Cc: ronnie sahlberg <ronniesahlberg@gmail.com>,
	linux-cifs <linux-cifs@vger.kernel.org>,
	Steve French <sfrench@samba.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] cifs: retry lookup and readdir when EAGAIN is returned.
Date: Wed, 16 Jun 2021 12:40:26 -0300	[thread overview]
Message-ID: <YMobapLxHv/BPzFP@nyarly.rlyeh.local> (raw)
In-Reply-To: <87zgvqqg09.fsf@suse.com>

Thanks for the comments, I'm working on them.

On Wed, Jun 16, 2021 at 12:07:50PM +0200, Aurélien Aptel wrote:

> I guess it looks ok as a quick fix for the issue at hand but:
> - should we check for more error codes (use is_retryable_error()?)
> - all syscalls will need something similar, the session can be closed at
>   any point
>   * either we add a loop everywhere
>   * or we change cifs_send_receive to retry (most?) calls
>   * might be worth looking at what nfs does here

Some syscall can return EAGAIN, so we would need to check which
operation is retryable and if the error is retryable. I don't know if
all Linux syscalls mappable to smb2 operations in the wire, but it may
be worth mapping.

NFS requeues the calls, and fails after a configurable timeout
for soft mounts, or issues an error message and requeues the request
for hard mounts (retrans and timeo mount options).

> - Should this be done for both soft (default) and hard mounts? I guess
>   for hard we would retry indefinitely

Good point, but the correct option is to retry on hard mounts until the
operation succeeds.

NFS hard mounts create issues like being unable to do a clean shutdown on
a server failure, because oustanding I/O blocks it. The NFS has atempted
to fix this by adding options to kill all outstanding I/O, but I'm not
current on the efforts/issues in this front.

So this would create the same issue with outstanding mounts on cifs 
shares, so a similar solution to NFS may be adapted/created in the future.

One thing that I forgot to mention in the patch is that this uncovered a
bug in compuond requests, where wait_for_compund_request will block the
readdir request with EDEADLK, if the share needs reconnect. I'm looking
into this patch, and will submmit it separatelly, as this looks like a
corner case uncovered by the patch and specific conditions in the tests.

> Cheers,
Best regards,
Thiago

  reply	other threads:[~2021-06-16 15:47 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-15 16:42 Thiago Rafael Becker
2021-06-16  6:09 ` ronnie sahlberg
2021-06-16 10:07   ` Aurélien Aptel
2021-06-16 15:40     ` Thiago Rafael Becker [this message]
2021-06-16 17:09   ` Steve French

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YMobapLxHv/BPzFP@nyarly.rlyeh.local \
    --to=trbecker@gmail.com \
    --cc=aaptel@suse.com \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ronniesahlberg@gmail.com \
    --cc=sfrench@samba.org \
    --subject='Re: [PATCH] cifs: retry lookup and readdir when EAGAIN is returned.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).