| Message ID | 20240522221916.447239-1-smayhew@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfs: don't invalidate dentries on transient errors | expand |
On Wed, 2024-05-22 at 18:19 -0400, Scott Mayhew wrote: > This is a slight variation on a patch previously proposed by Neil > Brown > that never got merged. > > Prior to commit 5ceb9d7fdaaf ("NFS: Refactor > nfs_lookup_revalidate()"), > any error from nfs_lookup_verify_inode() other than -ESTALE would > result > in nfs_lookup_revalidate() returning that error (-ESTALE is mapped to > zero). > > Since that commit, all errors result in nfs_lookup_revalidate() > returning zero, resulting in dentries being invalidated where they > previously were not (particularly in the case of -ERESTARTSYS). > > Fix it by passing the actual error code to > nfs_lookup_revalidate_done(), > and leaving the decision on whether to map the error code to zero or > one to nfs_lookup_revalidate_done(). > > A simple reproducer is to run the following python code in a > subdirectory of an NFS mount (not in the root of the NFS mount): > > ---8<--- > import os > import multiprocessing > import time > > if __name__=="__main__": > multiprocessing.set_start_method("spawn") > > count = 0 > while True: > try: > os.getcwd() > pool = multiprocessing.Pool(10) > pool.close() > pool.terminate() > count += 1 > except Exception as e: > print(f"Failed after {count} iterations") > print(e) > break > ---8<--- > > Prior to commit 5ceb9d7fdaaf, the above code would run indefinitely. > After commit 5ceb9d7fdaaf, it fails almost immediately with -ENOENT. > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > --- > fs/nfs/dir.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index ac505671efbd..d9264ed4ac52 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -1635,6 +1635,14 @@ nfs_lookup_revalidate_done(struct inode *dir, > struct dentry *dentry, > if (inode && IS_ROOT(dentry)) > error = 1; > break; > + case -ESTALE: > + case -ENOENT: > + error = 0; > + break; > + case -ETIMEDOUT: > + if (NFS_SERVER(inode)->flags & NFS_MOUNT_SOFTREVAL) > + error = 1; > + break; > } > trace_nfs_lookup_revalidate_exit(dir, dentry, 0, error); > return error; > @@ -1680,18 +1688,8 @@ static int nfs_lookup_revalidate_dentry(struct > inode *dir, > > dir_verifier = nfs_save_change_attribute(dir); > ret = NFS_PROTO(dir)->lookup(dir, dentry, fhandle, fattr); > - if (ret < 0) { > - switch (ret) { > - case -ESTALE: > - case -ENOENT: > - ret = 0; > - break; > - case -ETIMEDOUT: > - if (NFS_SERVER(inode)->flags & > NFS_MOUNT_SOFTREVAL) > - ret = 1; > - } > + if (ret < 0) > goto out; > - } > > /* Request help from readdirplus */ > nfs_lookup_advise_force_readdirplus(dir, flags); > @@ -1735,7 +1733,7 @@ nfs_do_lookup_revalidate(struct inode *dir, > struct dentry *dentry, > unsigned int flags) > { > struct inode *inode; > - int error; > + int error = 0; > > nfs_inc_stats(dir, NFSIOS_DENTRYREVALIDATE); > inode = d_inode(dentry); > @@ -1780,7 +1778,7 @@ nfs_do_lookup_revalidate(struct inode *dir, > struct dentry *dentry, > out_bad: > if (flags & LOOKUP_RCU) > return -ECHILD; > - return nfs_lookup_revalidate_done(dir, dentry, inode, 0); > + return nfs_lookup_revalidate_done(dir, dentry, inode, > error); Won't this now cause us to skip the special handling of the root directory in nfs_lookup_revalidate_done() if the call to nfs_lookup_verify_inode() fails with an error? > } > > static int
On Wed, 22 May 2024, Trond Myklebust wrote: > On Wed, 2024-05-22 at 18:19 -0400, Scott Mayhew wrote: > > This is a slight variation on a patch previously proposed by Neil > > Brown > > that never got merged. > > > > Prior to commit 5ceb9d7fdaaf ("NFS: Refactor > > nfs_lookup_revalidate()"), > > any error from nfs_lookup_verify_inode() other than -ESTALE would > > result > > in nfs_lookup_revalidate() returning that error (-ESTALE is mapped to > > zero). > > > > Since that commit, all errors result in nfs_lookup_revalidate() > > returning zero, resulting in dentries being invalidated where they > > previously were not (particularly in the case of -ERESTARTSYS). > > > > Fix it by passing the actual error code to > > nfs_lookup_revalidate_done(), > > and leaving the decision on whether to map the error code to zero or > > one to nfs_lookup_revalidate_done(). > > > > A simple reproducer is to run the following python code in a > > subdirectory of an NFS mount (not in the root of the NFS mount): > > > > ---8<--- > > import os > > import multiprocessing > > import time > > > > if __name__=="__main__": > > multiprocessing.set_start_method("spawn") > > > > count = 0 > > while True: > > try: > > os.getcwd() > > pool = multiprocessing.Pool(10) > > pool.close() > > pool.terminate() > > count += 1 > > except Exception as e: > > print(f"Failed after {count} iterations") > > print(e) > > break > > ---8<--- > > > > Prior to commit 5ceb9d7fdaaf, the above code would run indefinitely. > > After commit 5ceb9d7fdaaf, it fails almost immediately with -ENOENT. > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > --- > > fs/nfs/dir.c | 24 +++++++++++------------- > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > > index ac505671efbd..d9264ed4ac52 100644 > > --- a/fs/nfs/dir.c > > +++ b/fs/nfs/dir.c > > @@ -1635,6 +1635,14 @@ nfs_lookup_revalidate_done(struct inode *dir, > > struct dentry *dentry, > > if (inode && IS_ROOT(dentry)) > > error = 1; > > break; > > + case -ESTALE: > > + case -ENOENT: > > + error = 0; > > + break; > > + case -ETIMEDOUT: > > + if (NFS_SERVER(inode)->flags & NFS_MOUNT_SOFTREVAL) > > + error = 1; > > + break; > > } > > trace_nfs_lookup_revalidate_exit(dir, dentry, 0, error); > > return error; > > @@ -1680,18 +1688,8 @@ static int nfs_lookup_revalidate_dentry(struct > > inode *dir, > > > > dir_verifier = nfs_save_change_attribute(dir); > > ret = NFS_PROTO(dir)->lookup(dir, dentry, fhandle, fattr); > > - if (ret < 0) { > > - switch (ret) { > > - case -ESTALE: > > - case -ENOENT: > > - ret = 0; > > - break; > > - case -ETIMEDOUT: > > - if (NFS_SERVER(inode)->flags & > > NFS_MOUNT_SOFTREVAL) > > - ret = 1; > > - } > > + if (ret < 0) > > goto out; > > - } > > > > /* Request help from readdirplus */ > > nfs_lookup_advise_force_readdirplus(dir, flags); > > @@ -1735,7 +1733,7 @@ nfs_do_lookup_revalidate(struct inode *dir, > > struct dentry *dentry, > > unsigned int flags) > > { > > struct inode *inode; > > - int error; > > + int error = 0; > > > > nfs_inc_stats(dir, NFSIOS_DENTRYREVALIDATE); > > inode = d_inode(dentry); > > @@ -1780,7 +1778,7 @@ nfs_do_lookup_revalidate(struct inode *dir, > > struct dentry *dentry, > > out_bad: > > if (flags & LOOKUP_RCU) > > return -ECHILD; > > - return nfs_lookup_revalidate_done(dir, dentry, inode, 0); > > + return nfs_lookup_revalidate_done(dir, dentry, inode, > > error); > > Won't this now cause us to skip the special handling of the root > directory in nfs_lookup_revalidate_done() if the call to > nfs_lookup_verify_inode() fails with an error? Yes, it will. I'll send a v2 in a bit. -Scott > > > } > > > > static int > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index ac505671efbd..d9264ed4ac52 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1635,6 +1635,14 @@ nfs_lookup_revalidate_done(struct inode *dir, struct dentry *dentry, if (inode && IS_ROOT(dentry)) error = 1; break; + case -ESTALE: + case -ENOENT: + error = 0; + break; + case -ETIMEDOUT: + if (NFS_SERVER(inode)->flags & NFS_MOUNT_SOFTREVAL) + error = 1; + break; } trace_nfs_lookup_revalidate_exit(dir, dentry, 0, error); return error; @@ -1680,18 +1688,8 @@ static int nfs_lookup_revalidate_dentry(struct inode *dir, dir_verifier = nfs_save_change_attribute(dir); ret = NFS_PROTO(dir)->lookup(dir, dentry, fhandle, fattr); - if (ret < 0) { - switch (ret) { - case -ESTALE: - case -ENOENT: - ret = 0; - break; - case -ETIMEDOUT: - if (NFS_SERVER(inode)->flags & NFS_MOUNT_SOFTREVAL) - ret = 1; - } + if (ret < 0) goto out; - } /* Request help from readdirplus */ nfs_lookup_advise_force_readdirplus(dir, flags); @@ -1735,7 +1733,7 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, unsigned int flags) { struct inode *inode; - int error; + int error = 0; nfs_inc_stats(dir, NFSIOS_DENTRYREVALIDATE); inode = d_inode(dentry); @@ -1780,7 +1778,7 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, out_bad: if (flags & LOOKUP_RCU) return -ECHILD; - return nfs_lookup_revalidate_done(dir, dentry, inode, 0); + return nfs_lookup_revalidate_done(dir, dentry, inode, error); } static int
This is a slight variation on a patch previously proposed by Neil Brown that never got merged. Prior to commit 5ceb9d7fdaaf ("NFS: Refactor nfs_lookup_revalidate()"), any error from nfs_lookup_verify_inode() other than -ESTALE would result in nfs_lookup_revalidate() returning that error (-ESTALE is mapped to zero). Since that commit, all errors result in nfs_lookup_revalidate() returning zero, resulting in dentries being invalidated where they previously were not (particularly in the case of -ERESTARTSYS). Fix it by passing the actual error code to nfs_lookup_revalidate_done(), and leaving the decision on whether to map the error code to zero or one to nfs_lookup_revalidate_done(). A simple reproducer is to run the following python code in a subdirectory of an NFS mount (not in the root of the NFS mount): ---8<--- import os import multiprocessing import time if __name__=="__main__": multiprocessing.set_start_method("spawn") count = 0 while True: try: os.getcwd() pool = multiprocessing.Pool(10) pool.close() pool.terminate() count += 1 except Exception as e: print(f"Failed after {count} iterations") print(e) break ---8<--- Prior to commit 5ceb9d7fdaaf, the above code would run indefinitely. After commit 5ceb9d7fdaaf, it fails almost immediately with -ENOENT. Signed-off-by: Scott Mayhew <smayhew@redhat.com> --- fs/nfs/dir.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)