Re: [PATCH rfc] nfs: propagate readlink errors in nfs_symlink_filler

[Trond Myklebust <trondmy@hammerspace.com>]

On Wed, 2024-05-22 at 22:40 +0300, Sagi Grimberg wrote:
> Hey Trond,
> 
> > > filehandle is stale? There will have been an unlink() on the
> > > symlink at
> > > some point in the recent past.
> > > 
> > 
> > No reason that I can see. However given that this was observed in
> > the 
> > wild, and essentially
> > a common pattern with symlinks (overwrite a config file for
> > example), 
> > I think its reasonable
> > to have the vfs at least do a single retry, by simply returning
> > ESTALE.
> > However NFS cannot distinguish between first and second retries 
> > afaict... Perhaps the
> > vfs can help with a ESTALE->ENOENT conversion?
> 
> So what do you suggest we do here? IMO at a minimum NFS should retry 
> once similar
> to nfs4_file_open (it would probably address 99.9% of the use-cases 
> where symlinks are
> not overwritten in a high enough frequency for the client to see 2 
> consecutive stale readlink
> rpc rplies).
> 
> I can send a patch paired with a vfs ESTALE conversion patch? 
> alternatively retry locally in NFS...
> I would like to understand your position here.
> 

Looking more closely at nfs_get_link(), it is obvious that it can
already return ESTALE (thanks to the call to nfs_revalidate_mapping())
and looking at do_readlinkat(), it has already been plumbed through
with a call to retry_estale().

So I think we can take your patch as is, since it doesn't add any error
cases that callers of readlink() don't have to handle already.

We might still want to think about cleaning up the output of the VFS in
all these cases, so that we don't return ESTALE when it isn't allowed
by POSIX, but that would be a separate task.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com