From: Jeff Layton <jlayton@kernel.org>
To: Sagi Grimberg <sagi@grimberg.me>, linux-nfs@vger.kernel.org
Cc: Chuck Lever <chuck.lever@oracle.com>,
Dan Aloni <dan.aloni@vastdata.com>,
Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH rfc] nfs: propagate readlink errors in nfs_symlink_filler
Date: Tue, 21 May 2024 09:22:46 -0400 [thread overview]
Message-ID: <fa1a77fee6403454444fffce839924157778df95.camel@kernel.org> (raw)
In-Reply-To: <20240521125840.186618-1-sagi@grimberg.me>
On Tue, 2024-05-21 at 15:58 +0300, Sagi Grimberg wrote:
> There is an inherent race where a symlink file may have been overriden
> (by a different client) between lookup and readlink, resulting in a
> spurious EIO error returned to userspace. Fix this by propagating back
> ESTALE errors such that the vfs will retry the lookup/get_link (similar
> to nfs4_file_open) at least once.
>
> Cc: Dan Aloni <dan.aloni@vastdata.com>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> Note that with this change the vfs should retry once for
> ESTALE errors. However with an artificial reproducer of high
> frequency symlink overrides, nothing prevents the retry to
> also encounter ESTALE, propagating the error back to userspace.
> The man pages for openat/readlinkat do not list an ESTALE errno.
>
> An alternative attempt (implemented by Dan) was a local retry loop
> in nfs_get_link(), if this is an applicable approach, Dan can
> share his patch instead.
>
> fs/nfs/symlink.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfs/symlink.c b/fs/nfs/symlink.c
> index 0e27a2e4e68b..13818129d268 100644
> --- a/fs/nfs/symlink.c
> +++ b/fs/nfs/symlink.c
> @@ -41,7 +41,7 @@ static int nfs_symlink_filler(struct file *file, struct folio *folio)
> error:
> folio_set_error(folio);
> folio_unlock(folio);
> - return -EIO;
> + return error;
> }
>
> static const char *nfs_get_link(struct dentry *dentry,
git blame seems to indicate that we've returned -EIO here since the
beginning of the git era (and likely long before that). I see no reason
for us to cloak the real error there though, especially with something
like an ESTALE error.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
FWIW, I think we shouldn't try to do any retry looping on ESTALE beyond
what we already do.
Yes, we can sometimes trigger ESTALE errors to bubble up to userland if
we really thrash the underlying filesystem when testing, but I think
that's actually desirable:
If you have real workloads across multiple machines that are racing
with other that tightly, then you should probably be using some sort of
locking or other synchronization. If it's clever enough that it
doesn''t need that, then it should be able to deal with the occasional
ESTALE error by retrying on its own.
Cheers,
Jeff
next prev parent reply other threads:[~2024-05-21 13:22 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-21 12:58 [PATCH rfc] nfs: propagate readlink errors in nfs_symlink_filler Sagi Grimberg
2024-05-21 13:22 ` Jeff Layton [this message]
2024-05-21 14:36 ` Dan Aloni
2024-05-21 15:05 ` Sagi Grimberg
2024-05-21 15:13 ` Trond Myklebust
2024-05-21 15:24 ` Chuck Lever III
2024-05-22 4:41 ` Dan Aloni
2024-05-22 12:11 ` Trond Myklebust
2024-05-21 16:09 ` Sagi Grimberg
2024-05-22 19:40 ` Sagi Grimberg
2024-05-22 21:04 ` Trond Myklebust
2024-05-22 21:19 ` Sagi Grimberg
2024-05-30 18:08 ` Sagi Grimberg
2024-05-30 18:21 ` Trond Myklebust
2024-05-31 4:24 ` Sagi Grimberg
2024-05-22 21:20 ` Jeff Layton
2024-05-21 14:02 ` Chuck Lever
2024-05-21 14:59 ` Sagi Grimberg
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=fa1a77fee6403454444fffce839924157778df95.camel@kernel.org \
--to=jlayton@kernel.org \
--cc=chuck.lever@oracle.com \
--cc=dan.aloni@vastdata.com \
--cc=hch@lst.de \
--cc=linux-nfs@vger.kernel.org \
--cc=sagi@grimberg.me \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).