Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever III" <chuck.lever@oracle.com>
Cc:     "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v4 3/7] NFSD: Add an NFSD_FILE_GC flag to enable nfsd_file
 garbage collection
In-reply-to: <030F4BAB-6042-44C8-AC20-B1503D8A8004@oracle.com>
References: <166612295223.1291.11761205673682408148.stgit@manet.1015granger.net>,
 <166612311828.1291.6808456808715954510.stgit@manet.1015granger.net>,
 <166657883468.12462.7206160925496863213@noble.neil.brown.name>,
 <030F4BAB-6042-44C8-AC20-B1503D8A8004@oracle.com>
Date:   Tue, 25 Oct 2022 08:27:02 +1100
Message-id: <166664682244.12462.3621556759521787062@noble.neil.brown.name>
Precedence: bulk

On Tue, 25 Oct 2022, Chuck Lever III wrote:
>=20
> > On Oct 23, 2022, at 10:33 PM, NeilBrown <neilb@suse.de> wrote:
> >=20
> > On Wed, 19 Oct 2022, Chuck Lever wrote:
> >> NFSv4 operations manage the lifetime of nfsd_file items they use by
> >> means of NFSv4 OPEN and CLOSE. Hence there's no need for them to be
> >> garbage collected.
> >>=20
> >> Introduce a mechanism to enable garbage collection for nfsd_file
> >> items used only by NFSv2/3 callers.
> >>=20
> >> Note that the change in nfsd_file_put() ensures that both CLOSE and
> >> DELEGRETURN will actually close out and free an nfsd_file on last
> >> reference of a non-garbage-collected file.
> >>=20
> >> Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=3D394
> >> Suggested-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> >> Tested-by: Jeff Layton <jlayton@kernel.org>
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >> fs/nfsd/filecache.c |   61 +++++++++++++++++++++++++++++++++++++++++++++=
------
> >> fs/nfsd/filecache.h |    3 +++
> >> fs/nfsd/nfs3proc.c  |    4 ++-
> >> fs/nfsd/trace.h     |    3 ++-
> >> fs/nfsd/vfs.c       |    4 ++-
> >> 5 files changed, 63 insertions(+), 12 deletions(-)
> >>=20
> >> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> >> index b7aa523c2010..87fce5c95726 100644
> >> --- a/fs/nfsd/filecache.c
> >> +++ b/fs/nfsd/filecache.c
> >> @@ -63,6 +63,7 @@ struct nfsd_file_lookup_key {
> >> 	struct net			*net;
> >> 	const struct cred		*cred;
> >> 	unsigned char			need;
> >> +	unsigned char			gc:1;
> >> 	enum nfsd_file_lookup_type	type;
> >> };
> >>=20
> >> @@ -162,6 +163,8 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_com=
pare_arg *arg,
> >> 			return 1;
> >> 		if (!nfsd_match_cred(nf->nf_cred, key->cred))
> >> 			return 1;
> >> +		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) !=3D key->gc)
> >> +			return 1;
> >> 		if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) =3D=3D 0)
> >> 			return 1;
> >> 		break;
> >> @@ -297,6 +300,8 @@ nfsd_file_alloc(struct nfsd_file_lookup_key *key, un=
signed int may)
> >> 		nf->nf_flags =3D 0;
> >> 		__set_bit(NFSD_FILE_HASHED, &nf->nf_flags);
> >> 		__set_bit(NFSD_FILE_PENDING, &nf->nf_flags);
> >> +		if (key->gc)
> >> +			__set_bit(NFSD_FILE_GC, &nf->nf_flags);
> >> 		nf->nf_inode =3D key->inode;
> >> 		/* nf_ref is pre-incremented for hash table */
> >> 		refcount_set(&nf->nf_ref, 2);
> >> @@ -428,16 +433,27 @@ nfsd_file_put_noref(struct nfsd_file *nf)
> >> 	}
> >> }
> >>=20
> >> +static void
> >> +nfsd_file_unhash_and_put(struct nfsd_file *nf)
> >> +{
> >> +	if (nfsd_file_unhash(nf))
> >> +		nfsd_file_put_noref(nf);
> >> +}
> >> +
> >> void
> >> nfsd_file_put(struct nfsd_file *nf)
> >> {
> >> 	might_sleep();
> >>=20
> >> -	nfsd_file_lru_add(nf);
> >> +	if (test_bit(NFSD_FILE_GC, &nf->nf_flags) =3D=3D 1)
> >=20
> > Clearly this is a style choice on which sensible people might disagree,
> > but I much prefer to leave out the "=3D=3D 1" That is what most callers in
> > fs/nfsd/ do - only exceptions are here in filecache.c.
> > Even "!=3D 0" would be better than "=3D=3D 1".
> > I think test_bit() is declared as a bool, but it is hard to be certain.
>=20
> The definitions of test_bit() I've seen return "int" which is why
> I wrote it this way.

Just for completeness, I found

#define test_bit(nr, addr)              bitop(_test_bit, nr, addr)

in linux/bitops.h.

bitop() is defined just above and (I think) in the case where nr is
constant and addr is not NULL, this becomes a call to const_test_bit().
In include/asm-generic/bitops/generic-non-atomic.h I found

static __always_inline bool
const_test_bit(unsigned long nr, const volatile unsigned long *addr)

In the non-constant case it calls _test_bit() which is

static __always_inline bool
_test_bit(unsigned long nr, const volatile unsigned long *addr)

But as an int that is known to be 0 or 1 is type-compatible with a bool,
ht shouldn't really matter from a type perspective, only for a human
readability perspective.

Thanks,
NeilBrown