RE: [PATCH] nfsd: don't free files unconditionally in __nfsd_file_cache_purge

[Shachar Kagan <skagan@nvidia.com>]

On Sun, 2023-01-22 at 16:58 +0000, Shachar Kagan wrote:

> On Sat, 2023-01-21 at 17:04 +0000, Chuck Lever III wrote:
> >
> > > On Jan 20, 2023, at 3:54 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > On Fri, 2023-01-20 at 20:21 +0000, Chuck Lever III wrote:
> > > >
> > > > > On Jan 20, 2023, at 2:52 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > > > >
> > > > > nfsd_file_cache_purge is called when the server is shutting 
> > > > > down, in which case, tearing things down is generally fine, but 
> > > > > it also gets called when the exports cache is flushed.
> > > >
> > > > Yeah... cache flush is probably the case we've been missing.
> > > >
> > > >
> > > > > Instead of walking the cache and freeing everything 
> > > > > unconditionally, handle it the same as when we have a notification of conflicting access.
> > > > >
> > > > > Fixes: ac3a2585f018 ("nfsd: rework refcounting in filecache")
> > > > > Reported-by: Ruben Vestergaard <rubenv@drcmr.dk>
> > > > > Reported-by: Torkil Svensgaard <torkil@drcmr.dk>
> > > > > Reported-by: Shachar Kagan <skagan@nvidia.com>
> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>

I ran our tests with the commit, the issue didn't occur.
Thanks for the quick response.

> > > > > ---
> > > > > fs/nfsd/filecache.c | 61 
> > > > > +++++++++++++++++++++++++++------------------
> > > > > 1 file changed, 37 insertions(+), 24 deletions(-)
> > > > >
> > > > > v2: use the same method to purge entries from the cache as we do when
> > > > >   there is a notification of conflicting access.
> > > > >
> > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index 
> > > > > 58ac93e7e680..397ae212b98d 100644
> > > > > --- a/fs/nfsd/filecache.c
> > > > > +++ b/fs/nfsd/filecache.c
> > > > > @@ -661,6 +661,40 @@ static struct shrinker     nfsd_file_shrinker = {
> > > > >         .seeks = 1,
> > > > > };
> > > > >
> > > > > +/**
> > > > > + * maybe_queue_nfsd_file - try to unhash and queue a nfsd_file 
> > > > > +to be freed
> > > > > + * @nf: nfsd_file to attempt to queue
> > > > > + * @dispose: private list to queue successfully-put objects
> > > > > + *
> > > > > + * Unhash an nfsd_file, try to get a reference to it, and then 
> > > > > +put that
> > > > > + * reference. If it's the last reference, queue it to the dispose list.
> > > > > + *
> > > > > + * The caller MUST hold the rcu_read_lock() !
> > > >
> > > > __nfsd_file_cache_purge() isn't holding rcu_read_lock(), it's 
> > > > holding the nfsd_mutex. Is this comment incorrect, or is it just 
> > > > too specific? Or did I miss something obvious?
> > > >
> > >
> > > It's implicitly taken by rhashtable_walk_start and released by 
> > > rhashtable_walk_stop.
> > >
> > > FWIW, it'd be nice if there were a lockdep_assert_held equivalent 
> > > for the rcu_read_lock() here, but I didn't see one of those.
> >
> > Would
> >
> > static void
> > nfsd_file_cond_queue(struct nfsd_file *nf, struct list_head *dispose)
> >       __must_hold(RCU)
> > {
> >
> > do the trick?
> >
> 
> Seems like a reasonable thing to add.
> 
> >
> > > There is a
> > > rcu_read_lock_held(), but I didn't see a good way to get that to 
> > > compile out when lockdep was disabled.
> > >
> > > > > + */
> > > > > +static void
> > > > > +maybe_queue_nfsd_file(struct nfsd_file *nf, struct list_head 
> > > > > +*dispose)
> > > >
> > > > I prefer the name nfsd_file_try_to_queue() or nfsd_file_try_to_dispose().
> > > > nfsd_file_ should be the prefix where possible. Unless you're 
> > > > redriving, I can fix that.
> > > >
> > > >
> > >
> > > Rename at will.
> >
> > Done and applied to nfsd-fixes, replacing v1 of this patch.
> >
> 
> LGTM. Thanks!
> 
> >
> > > > > +{
> > > > > +       int decrement = 1;
> > > > > +
> > > > > +       /* If we raced with someone else unhashing, ignore it */
> > > > > +       if (!nfsd_file_unhash(nf))
> > > > > +               return;
> > > > > +
> > > > > +       /* If we can't get a reference, ignore it */
> > > > > +       if (!nfsd_file_get(nf))
> > > > > +               return;
> > > > > +
> > > > > +       /* Extra decrement if we remove from the LRU */
> > > > > +       if (nfsd_file_lru_remove(nf))
> > > > > +               ++decrement;
> > > > > +
> > > > > +       /* If refcount goes to 0, then put on the dispose list */
> > > > > +       if (refcount_sub_and_test(decrement, &nf->nf_ref)) {
> > > > > +               list_add(&nf->nf_lru, dispose);
> > > > > +               trace_nfsd_file_closing(nf);
> > > > > +       }
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * nfsd_file_queue_for_close: try to close out any open nfsd_files for an inode
> > > > > * @inode:   inode on which to close out nfsd_files
> > > > > @@ -688,30 +722,12 @@ nfsd_file_queue_for_close(struct inode 
> > > > > *inode, struct list_head *dispose)
> > > > >
> > > > >         rcu_read_lock();
> > > > >         do {
> > > > > -               int decrement = 1;
> > > > > -
> > > > >                 nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> > > > >                                        nfsd_file_rhash_params);
> > > > >                 if (!nf)
> > > > >                         break;
> > > > >
> > > > > -               /* If we raced with someone else unhashing, ignore it */
> > > > > -               if (!nfsd_file_unhash(nf))
> > > > > -                       continue;
> > > > > -
> > > > > -               /* If we can't get a reference, ignore it */
> > > > > -               if (!nfsd_file_get(nf))
> > > > > -                       continue;
> > > > > -
> > > > > -               /* Extra decrement if we remove from the LRU */
> > > > > -               if (nfsd_file_lru_remove(nf))
> > > > > -                       ++decrement;
> > > > > -
> > > > > -               /* If refcount goes to 0, then put on the dispose list */
> > > > > -               if (refcount_sub_and_test(decrement, &nf->nf_ref)) {
> > > > > -                       list_add(&nf->nf_lru, dispose);
> > > > > -                       trace_nfsd_file_closing(nf);
> > > > > -               }
> > > > > +               maybe_queue_nfsd_file(nf, dispose);
> > > > >         } while (1);
> > > > >         rcu_read_unlock();
> > > > > }
> > > > > @@ -928,11 +944,8 @@ __nfsd_file_cache_purge(struct net *net)
> > > > >
> > > > >                 nf = rhashtable_walk_next(&iter);
> > > > >                 while (!IS_ERR_OR_NULL(nf)) {
> > > > > -                       if (!net || nf->nf_net == net) {
> > > > > -                               nfsd_file_unhash(nf);
> > > > > -                               nfsd_file_lru_remove(nf);
> > > > > -                               list_add(&nf->nf_lru, &dispose);
> > > > > -                       }
> > > > > +                       if (!net || nf->nf_net == net)
> > > > > +                               maybe_queue_nfsd_file(nf, 
> > > > > + &dispose);
> > > > >                         nf = rhashtable_walk_next(&iter);
> > > > >                 }
> > > > >
> > > > > --
> > > > > 2.39.0
> > > > >
> > > >
> > > > --
> > > > Chuck Lever
> > > >
> > > >
> > > >
> > >
> > > --
> > > Jeff Layton <jlayton@kernel.org>
> >
> > --
> > Chuck Lever
> >
> >
> >
> 
> --
> Jeff Layton <jlayton@kernel.org>