On Tue, May 21, 2024 at 03:58:40PM +0300, Sagi Grimberg wrote: > There is an inherent race where a symlink file may have been overriden Nit: Do you mean "overwritten" ? > (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@xxxxxxxxxxxx> > Signed-off-by: Sagi Grimberg <sagi@xxxxxxxxxxx> > --- > 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 Nit: "overwrites" ? > also encounter ESTALE, propagating the error back to userspace. > The man pages for openat/readlinkat do not list an ESTALE errno. Speaking only as a community member, I consider that an undesirable behavior regression. IMO it's a bug for a system call to return an errno that isn't documented. That's likely why this logic has worked this way for forever. > 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. I'm not entirely convinced by your patch description that returning an EIO on occasion is a problem. Is it reasonable for the app to expect that readlinkat() will /never/ fail? Making symlink semantics more atomic on NFS mounts is probably a good goal. But IMO the proposed change by itself isn't going to get you that with high reliability and few or no undesirable side effects. Note that NFS client-side patches should be sent To: Trond, Anna, and Cc: linux-nfs@ . Trond and Anna need to weigh in on this. > 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, > -- > 2.40.1 > -- Chuck Lever
