Received-SPF: pass (google.com: domain of linux-nfs+bounces-3351-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 23 May 2024 08:18:14 -0400
From: Scott Mayhew <smayhew@redhat.com>
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "anna.schumaker@netapp.com" <anna.schumaker@netapp.com>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	"neilb@suse.de" <neilb@suse.de>
Subject: Re: [PATCH] nfs: don't invalidate dentries on transient errors
Message-ID: <Zk80Bm4nuT7eKdD3@aion>
References: <20240522221916.447239-1-smayhew@redhat.com>
 <9ecb1225e5746054c27ca9488d34510147e58edd.camel@hammerspace.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <9ecb1225e5746054c27ca9488d34510147e58edd.camel@hammerspace.com>

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.
> >=20
> > 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).
> >=20
> > 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).
> >=20
> > Fix it by passing the actual error code to
> > nfs_lookup_revalidate_done(),
> > and leaving the decision on whether to=A0 map the error code to zero or
> > one to nfs_lookup_revalidate_done().
> >=20
> > 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):
> >=20
> > ---8<---
> > import os
> > import multiprocessing
> > import time
> >=20
> > if __name__=3D=3D"__main__":
> > =A0=A0=A0 multiprocessing.set_start_method("spawn")
> >=20
> > =A0=A0=A0 count =3D 0
> > =A0=A0=A0 while True:
> > =A0=A0=A0=A0=A0=A0=A0 try:
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 os.getcwd()
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 pool =3D multiprocessing.Pool(10)
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 pool.close()
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 pool.terminate()
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 count +=3D 1
> > =A0=A0=A0=A0=A0=A0=A0 except Exception as e:
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 print(f"Failed after {count} iteratio=
ns")
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 print(e)
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 break
> > ---8<---
> >=20
> > Prior to commit 5ceb9d7fdaaf, the above code would run indefinitely.
> > After commit 5ceb9d7fdaaf, it fails almost immediately with -ENOENT.
> >=20
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> > =A0fs/nfs/dir.c | 24 +++++++++++-------------
> > =A01 file changed, 11 insertions(+), 13 deletions(-)
> >=20
> > 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,
> > =A0		if (inode && IS_ROOT(dentry))
> > =A0			error =3D 1;
> > =A0		break;
> > +	case -ESTALE:
> > +	case -ENOENT:
> > +		error =3D 0;
> > +		break;
> > +	case -ETIMEDOUT:
> > +		if (NFS_SERVER(inode)->flags & NFS_MOUNT_SOFTREVAL)
> > +			error =3D 1;
> > +		break;
> > =A0	}
> > =A0	trace_nfs_lookup_revalidate_exit(dir, dentry, 0, error);
> > =A0	return error;
> > @@ -1680,18 +1688,8 @@ static int nfs_lookup_revalidate_dentry(struct
> > inode *dir,
> > =A0
> > =A0	dir_verifier =3D nfs_save_change_attribute(dir);
> > =A0	ret =3D NFS_PROTO(dir)->lookup(dir, dentry, fhandle, fattr);
> > -	if (ret < 0) {
> > -		switch (ret) {
> > -		case -ESTALE:
> > -		case -ENOENT:
> > -			ret =3D 0;
> > -			break;
> > -		case -ETIMEDOUT:
> > -			if (NFS_SERVER(inode)->flags &
> > NFS_MOUNT_SOFTREVAL)
> > -				ret =3D 1;
> > -		}
> > +	if (ret < 0)
> > =A0		goto out;
> > -	}
> > =A0
> > =A0	/* Request help from readdirplus */
> > =A0	nfs_lookup_advise_force_readdirplus(dir, flags);
> > @@ -1735,7 +1733,7 @@ nfs_do_lookup_revalidate(struct inode *dir,
> > struct dentry *dentry,
> > =A0			 unsigned int flags)
> > =A0{
> > =A0	struct inode *inode;
> > -	int error;
> > +	int error =3D 0;
> > =A0
> > =A0	nfs_inc_stats(dir, NFSIOS_DENTRYREVALIDATE);
> > =A0	inode =3D d_inode(dentry);
> > @@ -1780,7 +1778,7 @@ nfs_do_lookup_revalidate(struct inode *dir,
> > struct dentry *dentry,
> > =A0out_bad:
> > =A0	if (flags & LOOKUP_RCU)
> > =A0		return -ECHILD;
> > -	return nfs_lookup_revalidate_done(dir, dentry, inode, 0);
> > +	return nfs_lookup_revalidate_done(dir, dentry, inode,
> > error);
>=20
> 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
>=20
> > =A0}
> > =A0
> > =A0static int
>=20
> --=20
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>=20
>=20