Received-SPF: pass (google.com: domain of linux-nfs+bounces-3328-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 22 May 2024 07:41:10 +0300
From: Dan Aloni <dan.aloni@vastdata.com>
To: Chuck Lever III <chuck.lever@oracle.com>
Cc: Trond Myklebust <trondmy@hammerspace.com>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
	"sagi@grimberg.me" <sagi@grimberg.me>,
	"jlayton@kernel.org" <jlayton@kernel.org>,
	"hch@lst.de" <hch@lst.de>
Subject: Re: [PATCH rfc] nfs: propagate readlink errors in nfs_symlink_filler
Message-ID: <20240522044110.rwqyldj6u6523alm@gmail.com>
References: <20240521125840.186618-1-sagi@grimberg.me>
 <fa1a77fee6403454444fffce839924157778df95.camel@kernel.org>
 <ac2bfa20-d952-4917-8a70-1e821f9b57ca@grimberg.me>
 <d5409ff9ce51e439f442abb1cded7c7ab732b726.camel@hammerspace.com>
 <53269EF0-4995-474B-9460-29A640E8A46F@oracle.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <53269EF0-4995-474B-9460-29A640E8A46F@oracle.com>

On 2024-05-21 15:24:19, Chuck Lever III wrote:
> 
> 
> > On May 21, 2024, at 11:13 AM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> > 
> > On Tue, 2024-05-21 at 18:05 +0300, Sagi Grimberg wrote:
> >> 
> >> 
> >> On 21/05/2024 16:22, Jeff Layton wrote:
> >>> 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:
> >> 
> >> Returning ESTALE would be an improvement over returning EIO IMO,
> >> but it may be surprising for userspace to see an undocumented errno.
> >> Maybe the man pages can be amended?
> >> 
> >>> 
> >>> 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.
> >> 
> >> I tend to agree. FWIW Solaris has a config knob for number of stale
> >> retries
> >> it does, maybe there is an appetite to have something like that as
> >> well?
> >> 
> > 
> > Any reason why we couldn't just return ENOENT in the case where the
> > filehandle is stale? There will have been an unlink() on the symlink at
> > some point in the recent past.
> 
> To me ENOENT is preferable to both EIO and ESTALE.

Another view on that, where in the scenario of `rename` causing the
unlinking, there was no situation of 'no entry' as the directory entry
was only updated and not removed. So ENOENT in this regard by the
meaning of 'no entry' would not reflect what has really happened.

(unless you go with the 'no entity' interpretation of ENOENT, but that
would be against most of the POSIX-spec cases where ENOENT is returned
which deal primarily with missing path components i.e. names to
objects and not the objects themselves)

-- 
Dan Aloni