From: Chuck Lever III <chuck.lever@oracle.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: Ruben Vestergaard <rubenv@drcmr.dk>,
Torkil Svensgaard <torkil@drcmr.dk>,
Shachar Kagan <skagan@nvidia.com>,
Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] nfsd: don't free files unconditionally in __nfsd_file_cache_purge
Date: Fri, 20 Jan 2023 20:21:18 +0000 [thread overview]
Message-ID: <C04F7C2C-848A-47C5-9C4E-9B9E5E82BAFF@oracle.com> (raw)
In-Reply-To: <20230120195215.42243-1-jlayton@kernel.org>
> 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>
> ---
> 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?
> + */
> +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.
> +{
> + 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
next prev parent reply other threads:[~2023-01-20 20:21 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-20 19:52 [PATCH] nfsd: don't free files unconditionally in __nfsd_file_cache_purge Jeff Layton
2023-01-20 20:21 ` Chuck Lever III [this message]
2023-01-20 20:54 ` Jeff Layton
2023-01-21 17:04 ` Chuck Lever III
2023-01-21 17:20 ` Jeff Layton
2023-01-22 14:58 ` Shachar Kagan
2023-01-22 23:40 ` Chuck Lever III
2023-01-23 10:57 ` Shachar Kagan
-- strict thread matches above, loose matches on Subject: below --
2023-01-19 19:20 Jeff Layton
2023-01-19 22:59 ` Chuck Lever III
2023-01-19 23:29 ` Jeff Layton
2023-01-20 18:36 ` Jeff Layton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=C04F7C2C-848A-47C5-9C4E-9B9E5E82BAFF@oracle.com \
--to=chuck.lever@oracle.com \
--cc=jlayton@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=rubenv@drcmr.dk \
--cc=skagan@nvidia.com \
--cc=torkil@drcmr.dk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).