On Thu, 2023-01-05 at 14:18 +0000, Chuck Lever III wrote:
> Hi Jeff-
>
>
> > On Jan 5, 2023, at 7:15 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >
> > Since v4 files are expected to be long-lived, there's little value in
> > closing them out of the cache when there is conflicting access.
>
> Seems like targeting long-lived nfsd_file items could actually
> be a hazardous behavior. Are you sure it's safe to leave it in
> stable kernels?
>
Basically it just means we end up doing more opens than are needed in
this situation.
The notifiers just unnecessarily unhash a nfsd_file in this case, even
though it doesn't have any chance of freeing it until the client issues
a CLOSE, due to the persistent references held by the stateids.
So, this really is just an optimization and not a "real bug".
>
> > Rename NFSD_FILE_KEY_INODE to NFSD_FILE_KEY_INODE_GC,
>
> I'd prefer to keep the name as it is. It's for searching for
> inodes, and adding the ".gc = true" to the search criteria is
> enough to show what you are looking for.
>
Ok.
>
> > and change the
> > comparator to also match the gc value in the key. Change both of the
> > current users of that key to set the gc value in the key to "true".
> >
> > Also, test_bit returns bool, AFAICT, so I think we're ok to drop the
> > !! from the condition later in the comparator.
>
> And I'd prefer that you leave this clean up for another patch.
>
Ok, I'll drop that hunk.
>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/nfsd/filecache.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index 9fff1fa09d08..a67b22579c6e 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -78,7 +78,7 @@ static struct rhashtable nfsd_file_rhash_tbl
> > ____cacheline_aligned_in_smp;
> >
> > enum nfsd_file_lookup_type {
> > - NFSD_FILE_KEY_INODE,
> > + NFSD_FILE_KEY_INODE_GC,
> > NFSD_FILE_KEY_FULL,
> > };
> >
> > @@ -174,7 +174,9 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
> > const struct nfsd_file *nf = ptr;
> >
> > switch (key->type) {
> > - case NFSD_FILE_KEY_INODE:
> > + case NFSD_FILE_KEY_INODE_GC:
> > + if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
> > + return 1;
> > if (nf->nf_inode != key->inode)
> > return 1;
> > break;
> > @@ -187,7 +189,7 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
> > return 1;
> > if (!nfsd_match_cred(nf->nf_cred, key->cred))
> > return 1;
> > - if (!!test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
> > + if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
> > return 1;
> > if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0)
> > return 1;
> > @@ -681,8 +683,9 @@ static void
> > nfsd_file_queue_for_close(struct inode *inode, struct list_head *dispose)
> > {
> > struct nfsd_file_lookup_key key = {
> > - .type = NFSD_FILE_KEY_INODE,
> > + .type = NFSD_FILE_KEY_INODE_GC,
> > .inode = inode,
> > + .gc = true,
> > };
> > struct nfsd_file *nf;
> >
> > @@ -1057,8 +1060,9 @@ bool
> > nfsd_file_is_cached(struct inode *inode)
> > {
> > struct nfsd_file_lookup_key key = {
> > - .type = NFSD_FILE_KEY_INODE,
> > + .type = NFSD_FILE_KEY_INODE_GC,
> > .inode = inode,
> > + .gc = true,
> > };
> > bool ret = false;
> >
> > --
> > 2.39.0
> >
>
> --
> Chuck Lever
>
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
