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.
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 <[email protected]>
Reported-by: Torkil Svensgaard <[email protected]>
Reported-by: Shachar Kagan <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
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() !
+ */
+static void
+maybe_queue_nfsd_file(struct nfsd_file *nf, struct list_head *dispose)
+{
+ 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
> On Jan 20, 2023, at 2:52 PM, Jeff Layton <[email protected]> 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 <[email protected]>
> Reported-by: Torkil Svensgaard <[email protected]>
> Reported-by: Shachar Kagan <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> 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
On Fri, 2023-01-20 at 20:21 +0000, Chuck Lever III wrote:
>
> > On Jan 20, 2023, at 2:52 PM, Jeff Layton <[email protected]> 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 <[email protected]>
> > Reported-by: Torkil Svensgaard <[email protected]>
> > Reported-by: Shachar Kagan <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > 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. 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.
> > +{
> > + 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 <[email protected]>
On Sat, 2023-01-21 at 17:04 +0000, Chuck Lever III wrote:
>
> > On Jan 20, 2023, at 3:54 PM, Jeff Layton <[email protected]> wrote:
> >
> > On Fri, 2023-01-20 at 20:21 +0000, Chuck Lever III wrote:
> > >
> > > > On Jan 20, 2023, at 2:52 PM, Jeff Layton <[email protected]> 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 <[email protected]>
> > > > Reported-by: Torkil Svensgaard <[email protected]>
> > > > Reported-by: Shachar Kagan <[email protected]>
> > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
>
> --
> Chuck Lever
>
>
>
--
Jeff Layton <[email protected]>
> On Jan 20, 2023, at 3:54 PM, Jeff Layton <[email protected]> wrote:
>
> On Fri, 2023-01-20 at 20:21 +0000, Chuck Lever III wrote:
>>
>>> On Jan 20, 2023, at 2:52 PM, Jeff Layton <[email protected]> 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 <[email protected]>
>>> Reported-by: Torkil Svensgaard <[email protected]>
>>> Reported-by: Shachar Kagan <[email protected]>
>>> Signed-off-by: Jeff Layton <[email protected]>
>>> ---
>>> 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?
> 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.
>>> +{
>>> + 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 <[email protected]>
--
Chuck Lever
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 <[email protected]> wrote:
> > >
> > > On Fri, 2023-01-20 at 20:21 +0000, Chuck Lever III wrote:
> > > >
> > > > > On Jan 20, 2023, at 2:52 PM, Jeff Layton <[email protected]> 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 <[email protected]>
> > > > > Reported-by: Torkil Svensgaard <[email protected]>
> > > > > Reported-by: Shachar Kagan <[email protected]>
> > > > > Signed-off-by: Jeff Layton <[email protected]>
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 <[email protected]>
> >
> > --
> > Chuck Lever
> >
> >
> >
>
> --
> Jeff Layton <[email protected]>
> On Jan 22, 2023, at 9:58 AM, Shachar Kagan <[email protected]> wrote:
>
>
> 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 <[email protected]> wrote:
>>>>
>>>> On Fri, 2023-01-20 at 20:21 +0000, Chuck Lever III wrote:
>>>>>
>>>>>> On Jan 20, 2023, at 2:52 PM, Jeff Layton <[email protected]> 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 <[email protected]>
>>>>>> Reported-by: Torkil Svensgaard <[email protected]>
>>>>>> Reported-by: Shachar Kagan <[email protected]>
>>>>>> Signed-off-by: Jeff Layton <[email protected]>
>
> I ran our tests with the commit, the issue didn't occur.
> Thanks for the quick response.
May I add Tested-by: Shachar Kagan <[email protected]> ?
--
Chuck Lever
>> On Jan 22, 2023, at 9:58 AM, Shachar Kagan <[email protected]> wrote:
>>
>>> On Sat, 2023-01-21 at 17:04 +0000, Chuck Lever III wrote:
>>>>
>>>>> On Jan 20, 2023, at 3:54 PM, Jeff Layton <[email protected]> wrote:
>>>>>
>>>>> On Fri, 2023-01-20 at 20:21 +0000, Chuck Lever III wrote:
>>>>>>
>>>>>>> On Jan 20, 2023, at 2:52 PM, Jeff Layton <[email protected]> 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 <[email protected]>
>>>>>>> Reported-by: Torkil Svensgaard <[email protected]>
>>>>>>> Reported-by: Shachar Kagan <[email protected]>
>>>>>>> Signed-off-by: Jeff Layton <[email protected]>
>>
>> I ran our tests with the commit, the issue didn't occur.
>> Thanks for the quick response.
>
> May I add Tested-by: Shachar Kagan <[email protected]> ?
>
Yes
> --
> Chuck Lever
--
Shachar Kagan