[PATCH 0/3] nfsd: plug some filecache refcount leaks

[PATCH 0/3] nfsd: plug some filecache refcount leaks

[Jeff Layton]

Youzhong Yang sent an email to the list (along with some draft patches)
that indicated some nfsd_file refcount leaks. I went crawling over the
filecache code (again) and found a couple of places where we didn't put
references when we should. I'm not sure if it'll fix the problem they
reported, but they are bugs.

Plus, let's start counting nfsd_file allocations. The last patch adds
support for this.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Jeff Layton (3):
      nfsd: fix refcount leak when failing to hash nfsd_file
      nfsd: fix refcount leak when file is unhashed after being found
      nfsd: count nfsd_file allocations

 fs/nfsd/filecache.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
---
base-commit: 24decb225ed20a5ba46a79f4609e109cb0e4a359
change-id: 20240710-nfsd-next-01e2afdebb31

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file

[Jeff Layton]

At this point, we have a new nf that we couldn't properly insert into
the hashtable. Just free it before retrying, since it was never hashed.

Fixes: c6593366c0bf ("nfsd: don't kill nfsd_files because of lease break error")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/filecache.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index f84913691b78..4fb5e8546831 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -1038,8 +1038,10 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (likely(ret == 0))
 		goto open_file;
 
-	if (ret == -EEXIST)
+	if (ret == -EEXIST) {
+		nfsd_file_free(nf);
 		goto retry;
+	}
 	trace_nfsd_file_insert_err(rqstp, inode, may_flags, ret);
 	status = nfserr_jukebox;
 	goto construction_err;

-- 
2.45.2

Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file

[Youzhong Yang]

Shouldn't we have fh_put(fhp) before 'retry'?

On Wed, Jul 10, 2024 at 9:06 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> At this point, we have a new nf that we couldn't properly insert into
> the hashtable. Just free it before retrying, since it was never hashed.
>
> Fixes: c6593366c0bf ("nfsd: don't kill nfsd_files because of lease break error")
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/filecache.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index f84913691b78..4fb5e8546831 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -1038,8 +1038,10 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>         if (likely(ret == 0))
>                 goto open_file;
>
> -       if (ret == -EEXIST)
> +       if (ret == -EEXIST) {
> +               nfsd_file_free(nf);
>                 goto retry;
> +       }
>         trace_nfsd_file_insert_err(rqstp, inode, may_flags, ret);
>         status = nfserr_jukebox;
>         goto construction_err;
>
> --
> 2.45.2
>

Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file

[Jeff Layton]

On Thu, 2024-07-11 at 13:05 -0400, Youzhong Yang wrote:
> Shouldn't we have fh_put(fhp) before 'retry'?
> 

A subtle question, actually...

It probably wouldn't hurt to do that, but I don't think it's required.

The main reason we call fh_put is to force a second call to
nfsd_set_fh_dentry. IOW, we want to redo the lookup by filehandle and
find the inode.

In the EEXIST case, presumably we have found the inode but we raced
with another task in setting an nfsd_file for it in the hash. That's
different from the case where the thing was unhashed or we got an
EOPENSTALE.  So, I think we probably don't require refinding the inode
in that case.

More pointedly, I'm not sure this particular case is actually possible.
The entries are hashed on the inode pointer value, and we're searching
and inserting into the hash under the i_lock.

Chuck, thoughts?

> On Wed, Jul 10, 2024 at 9:06 AM Jeff Layton <jlayton@kernel.org>
> wrote:
> > 
> > At this point, we have a new nf that we couldn't properly insert
> > into
> > the hashtable. Just free it before retrying, since it was never
> > hashed.
> > 
> > Fixes: c6593366c0bf ("nfsd: don't kill nfsd_files because of lease
> > break error")
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/filecache.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index f84913691b78..4fb5e8546831 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -1038,8 +1038,10 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp,
> > struct svc_fh *fhp,
> >         if (likely(ret == 0))
> >                 goto open_file;
> > 
> > -       if (ret == -EEXIST)
> > +       if (ret == -EEXIST) {
> > +               nfsd_file_free(nf);
> >                 goto retry;
> > +       }
> >         trace_nfsd_file_insert_err(rqstp, inode, may_flags, ret);
> >         status = nfserr_jukebox;
> >         goto construction_err;
> > 
> > --
> > 2.45.2
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file

[Chuck Lever III]

> On Jul 11, 2024, at 1:53 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Thu, 2024-07-11 at 13:05 -0400, Youzhong Yang wrote:
>> Shouldn't we have fh_put(fhp) before 'retry'?
>> 
> 
> A subtle question, actually...
> 
> It probably wouldn't hurt to do that, but I don't think it's required.
> 
> The main reason we call fh_put is to force a second call to
> nfsd_set_fh_dentry. IOW, we want to redo the lookup by filehandle and
> find the inode.
> 
> In the EEXIST case, presumably we have found the inode but we raced
> with another task in setting an nfsd_file for it in the hash. That's
> different from the case where the thing was unhashed or we got an
> EOPENSTALE.  So, I think we probably don't require refinding the inode
> in that case.
> 
> More pointedly, I'm not sure this particular case is actually possible.
> The entries are hashed on the inode pointer value, and we're searching
> and inserting into the hash under the i_lock.
> 
> Chuck, thoughts?

Is the question whether we want to dput() the dentry that
is attached to the fhp ? fh_verify's API contract says:

310  * Regardless of success or failure of fh_verify(), fh_put() should be
311  * called on @fhp when the caller is finished with the filehandle. 

It looks like none of nfsd_file_acquire's callers do an
fh_put() in their error flows.

But maybe I've misunderstood the issue.


>> On Wed, Jul 10, 2024 at 9:06 AM Jeff Layton <jlayton@kernel.org>
>> wrote:
>>> 
>>> At this point, we have a new nf that we couldn't properly insert
>>> into
>>> the hashtable. Just free it before retrying, since it was never
>>> hashed.
>>> 
>>> Fixes: c6593366c0bf ("nfsd: don't kill nfsd_files because of lease
>>> break error")
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>  fs/nfsd/filecache.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>> index f84913691b78..4fb5e8546831 100644
>>> --- a/fs/nfsd/filecache.c
>>> +++ b/fs/nfsd/filecache.c
>>> @@ -1038,8 +1038,10 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp,
>>> struct svc_fh *fhp,
>>>         if (likely(ret == 0))
>>>                 goto open_file;
>>> 
>>> -       if (ret == -EEXIST)
>>> +       if (ret == -EEXIST) {
>>> +               nfsd_file_free(nf);
>>>                 goto retry;
>>> +       }
>>>         trace_nfsd_file_insert_err(rqstp, inode, may_flags, ret);
>>>         status = nfserr_jukebox;
>>>         goto construction_err;
>>> 
>>> --
>>> 2.45.2
>>> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>


--
Chuck Lever

Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file

[Jeff Layton]

On Thu, 2024-07-11 at 18:16 +0000, Chuck Lever III wrote:
> 
> 
> > On Jul 11, 2024, at 1:53 PM, Jeff Layton <jlayton@kernel.org>
> > wrote:
> > 
> > On Thu, 2024-07-11 at 13:05 -0400, Youzhong Yang wrote:
> > > Shouldn't we have fh_put(fhp) before 'retry'?
> > > 
> > 
> > A subtle question, actually...
> > 
> > It probably wouldn't hurt to do that, but I don't think it's
> > required.
> > 
> > The main reason we call fh_put is to force a second call to
> > nfsd_set_fh_dentry. IOW, we want to redo the lookup by filehandle
> > and
> > find the inode.
> > 
> > In the EEXIST case, presumably we have found the inode but we raced
> > with another task in setting an nfsd_file for it in the hash.
> > That's
> > different from the case where the thing was unhashed or we got an
> > EOPENSTALE.  So, I think we probably don't require refinding the
> > inode
> > in that case.
> > 
> > More pointedly, I'm not sure this particular case is actually
> > possible.
> > The entries are hashed on the inode pointer value, and we're
> > searching
> > and inserting into the hash under the i_lock.
> > 
> > Chuck, thoughts?
> 
> Is the question whether we want to dput() the dentry that
> is attached to the fhp ? fh_verify's API contract says:
> 
> 310  * Regardless of success or failure of fh_verify(), fh_put()
> should be
> 311  * called on @fhp when the caller is finished with the
> filehandle. 
> 
> It looks like none of nfsd_file_acquire's callers do an
> fh_put() in their error flows.
> 
> But maybe I've misunderstood the issue.
> 

Note that this API is weird and doesn't conform to typical get/put
semantics.

The fhp is instantiated before nfsd_file_do_acquire is called, and all
of the callers that I can see do eventually call fh_put on it. fh_put
is idempotent, so there should be no harm in calling it multiple times.

My question here though was more about this EEXIST case. Should we even
bother checking for that? I don't see how it's possible.

> 
> > > On Wed, Jul 10, 2024 at 9:06 AM Jeff Layton <jlayton@kernel.org>
> > > wrote:
> > > > 
> > > > At this point, we have a new nf that we couldn't properly
> > > > insert
> > > > into
> > > > the hashtable. Just free it before retrying, since it was never
> > > > hashed.
> > > > 
> > > > Fixes: c6593366c0bf ("nfsd: don't kill nfsd_files because of
> > > > lease
> > > > break error")
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/nfsd/filecache.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > > > index f84913691b78..4fb5e8546831 100644
> > > > --- a/fs/nfsd/filecache.c
> > > > +++ b/fs/nfsd/filecache.c
> > > > @@ -1038,8 +1038,10 @@ nfsd_file_do_acquire(struct svc_rqst
> > > > *rqstp,
> > > > struct svc_fh *fhp,
> > > >         if (likely(ret == 0))
> > > >                 goto open_file;
> > > > 
> > > > -       if (ret == -EEXIST)
> > > > +       if (ret == -EEXIST) {
> > > > +               nfsd_file_free(nf);
> > > >                 goto retry;
> > > > +       }
> > > >         trace_nfsd_file_insert_err(rqstp, inode, may_flags,
> > > > ret);
> > > >         status = nfserr_jukebox;
> > > >         goto construction_err;
> > > > 
> > > > --
> > > > 2.45.2
> > > > 
> > 
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> 
> 
> --
> Chuck Lever
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file

[Chuck Lever III]

> On Jul 11, 2024, at 2:20 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Thu, 2024-07-11 at 18:16 +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Jul 11, 2024, at 1:53 PM, Jeff Layton <jlayton@kernel.org>
>>> wrote:
>>> 
>>> On Thu, 2024-07-11 at 13:05 -0400, Youzhong Yang wrote:
>>>> Shouldn't we have fh_put(fhp) before 'retry'?
>>>> 
>>> 
>>> A subtle question, actually...
>>> 
>>> It probably wouldn't hurt to do that, but I don't think it's
>>> required.
>>> 
>>> The main reason we call fh_put is to force a second call to
>>> nfsd_set_fh_dentry. IOW, we want to redo the lookup by filehandle
>>> and
>>> find the inode.
>>> 
>>> In the EEXIST case, presumably we have found the inode but we raced
>>> with another task in setting an nfsd_file for it in the hash.
>>> That's
>>> different from the case where the thing was unhashed or we got an
>>> EOPENSTALE.  So, I think we probably don't require refinding the
>>> inode
>>> in that case.
>>> 
>>> More pointedly, I'm not sure this particular case is actually
>>> possible.
>>> The entries are hashed on the inode pointer value, and we're
>>> searching
>>> and inserting into the hash under the i_lock.
>>> 
>>> Chuck, thoughts?
>> 
>> Is the question whether we want to dput() the dentry that
>> is attached to the fhp ? fh_verify's API contract says:
>> 
>> 310  * Regardless of success or failure of fh_verify(), fh_put()
>> should be
>> 311  * called on @fhp when the caller is finished with the
>> filehandle. 
>> 
>> It looks like none of nfsd_file_acquire's callers do an
>> fh_put() in their error flows.
>> 
>> But maybe I've misunderstood the issue.
>> 
> 
> Note that this API is weird and doesn't conform to typical get/put
> semantics.
> 
> The fhp is instantiated before nfsd_file_do_acquire is called, and all
> of the callers that I can see do eventually call fh_put on it. fh_put
> is idempotent, so there should be no harm in calling it multiple times.
> 
> My question here though was more about this EEXIST case. Should we even
> bother checking for that? I don't see how it's possible.

If memory serves, at one point nfsd_file_acquire() used
rhtable_insert_yada(), which returns -EEXIST for certain
table overflow cases. Possibly with the list version of
rhtable, consumers can't get -EEXIST at all.


>>>> On Wed, Jul 10, 2024 at 9:06 AM Jeff Layton <jlayton@kernel.org>
>>>> wrote:
>>>>> 
>>>>> At this point, we have a new nf that we couldn't properly
>>>>> insert
>>>>> into
>>>>> the hashtable. Just free it before retrying, since it was never
>>>>> hashed.
>>>>> 
>>>>> Fixes: c6593366c0bf ("nfsd: don't kill nfsd_files because of
>>>>> lease
>>>>> break error")
>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>>> ---
>>>>>  fs/nfsd/filecache.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>>>> index f84913691b78..4fb5e8546831 100644
>>>>> --- a/fs/nfsd/filecache.c
>>>>> +++ b/fs/nfsd/filecache.c
>>>>> @@ -1038,8 +1038,10 @@ nfsd_file_do_acquire(struct svc_rqst
>>>>> *rqstp,
>>>>> struct svc_fh *fhp,
>>>>>         if (likely(ret == 0))
>>>>>                 goto open_file;
>>>>> 
>>>>> -       if (ret == -EEXIST)
>>>>> +       if (ret == -EEXIST) {
>>>>> +               nfsd_file_free(nf);
>>>>>                 goto retry;
>>>>> +       }
>>>>>         trace_nfsd_file_insert_err(rqstp, inode, may_flags,
>>>>> ret);
>>>>>         status = nfserr_jukebox;
>>>>>         goto construction_err;
>>>>> 
>>>>> --
>>>>> 2.45.2
>>>>> 
>>> 
>>> -- 
>>> Jeff Layton <jlayton@kernel.org>
>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever

[PATCH 2/3] nfsd: fix refcount leak when file is unhashed after being found

[Jeff Layton]

If we wait_for_construction and find that the file is no longer hashed,
and we're going to retry the open, the old nfsd_file reference is
currently leaked. Put the reference before retrying.

Fixes: c6593366c0bf ("nfsd: don't kill nfsd_files because of lease break error")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/filecache.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 4fb5e8546831..52063f2cf0df 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -1056,6 +1056,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			status = nfserr_jukebox;
 			goto construction_err;
 		}
+		nfsd_file_put(nf);
 		open_retry = false;
 		fh_put(fhp);
 		goto retry;

-- 
2.45.2

[PATCH 3/3] nfsd: count nfsd_file allocations

[Jeff Layton]

We already count the frees (via nfsd_file_releases). Count the
allocations as well. Also switch the direct call to nfsd_file_slab_free
in nfsd_file_do_acquire to nfsd_file_free, so that the allocs and
releases match up.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/filecache.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 52063f2cf0df..8997058ddbcb 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -56,6 +56,7 @@
 
 static DEFINE_PER_CPU(unsigned long, nfsd_file_cache_hits);
 static DEFINE_PER_CPU(unsigned long, nfsd_file_acquisitions);
+static DEFINE_PER_CPU(unsigned long, nfsd_file_allocations);
 static DEFINE_PER_CPU(unsigned long, nfsd_file_releases);
 static DEFINE_PER_CPU(unsigned long, nfsd_file_total_age);
 static DEFINE_PER_CPU(unsigned long, nfsd_file_evictions);
@@ -215,6 +216,7 @@ nfsd_file_alloc(struct net *net, struct inode *inode, unsigned char need,
 	if (unlikely(!nf))
 		return NULL;
 
+	this_cpu_inc(nfsd_file_allocations);
 	INIT_LIST_HEAD(&nf->nf_lru);
 	INIT_LIST_HEAD(&nf->nf_gc);
 	nf->nf_birthtime = ktime_get();
@@ -912,6 +914,7 @@ nfsd_file_cache_shutdown(void)
 	for_each_possible_cpu(i) {
 		per_cpu(nfsd_file_cache_hits, i) = 0;
 		per_cpu(nfsd_file_acquisitions, i) = 0;
+		per_cpu(nfsd_file_allocations, i) = 0;
 		per_cpu(nfsd_file_releases, i) = 0;
 		per_cpu(nfsd_file_total_age, i) = 0;
 		per_cpu(nfsd_file_evictions, i) = 0;
@@ -1027,7 +1030,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (unlikely(nf)) {
 		spin_unlock(&inode->i_lock);
 		rcu_read_unlock();
-		nfsd_file_slab_free(&new->nf_rcu);
+		nfsd_file_free(new);
 		goto wait_for_construction;
 	}
 	nf = new;
@@ -1205,7 +1208,7 @@ nfsd_file_acquire_opened(struct svc_rqst *rqstp, struct svc_fh *fhp,
  */
 int nfsd_file_cache_stats_show(struct seq_file *m, void *v)
 {
-	unsigned long releases = 0, evictions = 0;
+	unsigned long allocations = 0, releases = 0, evictions = 0;
 	unsigned long hits = 0, acquisitions = 0;
 	unsigned int i, count = 0, buckets = 0;
 	unsigned long lru = 0, total_age = 0;
@@ -1230,6 +1233,7 @@ int nfsd_file_cache_stats_show(struct seq_file *m, void *v)
 	for_each_possible_cpu(i) {
 		hits += per_cpu(nfsd_file_cache_hits, i);
 		acquisitions += per_cpu(nfsd_file_acquisitions, i);
+		allocations += per_cpu(nfsd_file_allocations, i);
 		releases += per_cpu(nfsd_file_releases, i);
 		total_age += per_cpu(nfsd_file_total_age, i);
 		evictions += per_cpu(nfsd_file_evictions, i);
@@ -1240,6 +1244,7 @@ int nfsd_file_cache_stats_show(struct seq_file *m, void *v)
 	seq_printf(m, "lru entries:   %lu\n", lru);
 	seq_printf(m, "cache hits:    %lu\n", hits);
 	seq_printf(m, "acquisitions:  %lu\n", acquisitions);
+	seq_printf(m, "allocations:   %lu\n", allocations);
 	seq_printf(m, "releases:      %lu\n", releases);
 	seq_printf(m, "evictions:     %lu\n", evictions);
 	if (releases)

-- 
2.45.2

Re: [PATCH 0/3] nfsd: plug some filecache refcount leaks

[Chuck Lever]

On Wed, Jul 10, 2024 at 09:05:30AM -0400, Jeff Layton wrote:
> Youzhong Yang sent an email to the list (along with some draft patches)
> that indicated some nfsd_file refcount leaks. I went crawling over the
> filecache code (again) and found a couple of places where we didn't put
> references when we should. I'm not sure if it'll fix the problem they
> reported, but they are bugs.
> 
> Plus, let's start counting nfsd_file allocations. The last patch adds
> support for this.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> Jeff Layton (3):
>       nfsd: fix refcount leak when failing to hash nfsd_file
>       nfsd: fix refcount leak when file is unhashed after being found
>       nfsd: count nfsd_file allocations
> 
>  fs/nfsd/filecache.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> ---
> base-commit: 24decb225ed20a5ba46a79f4609e109cb0e4a359
> change-id: 20240710-nfsd-next-01e2afdebb31
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

I've browsed these, they seem straightforward.

Since this is the week before a merge window, I would like to save
these and Youzhong's patch for v6.12. The Fixes: tags will get them
pulled back into the stable kernels once they are merged.

'Salright?

-- 
Chuck Lever

Re: [PATCH 0/3] nfsd: plug some filecache refcount leaks

[Jeff Layton]

On Wed, 2024-07-10 at 10:35 -0400, Chuck Lever wrote:
> On Wed, Jul 10, 2024 at 09:05:30AM -0400, Jeff Layton wrote:
> > Youzhong Yang sent an email to the list (along with some draft
> > patches)
> > that indicated some nfsd_file refcount leaks. I went crawling over
> > the
> > filecache code (again) and found a couple of places where we didn't
> > put
> > references when we should. I'm not sure if it'll fix the problem
> > they
> > reported, but they are bugs.
> > 
> > Plus, let's start counting nfsd_file allocations. The last patch
> > adds
> > support for this.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > Jeff Layton (3):
> >       nfsd: fix refcount leak when failing to hash nfsd_file
> >       nfsd: fix refcount leak when file is unhashed after being
> > found
> >       nfsd: count nfsd_file allocations
> > 
> >  fs/nfsd/filecache.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > ---
> > base-commit: 24decb225ed20a5ba46a79f4609e109cb0e4a359
> > change-id: 20240710-nfsd-next-01e2afdebb31
> > 
> > Best regards,
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 
> 
> I've browsed these, they seem straightforward.
> 
> Since this is the week before a merge window, I would like to save
> these and Youzhong's patch for v6.12. The Fixes: tags will get them
> pulled back into the stable kernels once they are merged.
> 
> 'Salright?
> 

Agreed. Leaks suck, but they (usually) aren't fatal, and interactions
in the filecache can be...subtle. I wouldn't mind a bit more testing
before we send this on.

Thanks!
-- 
Jeff Layton <jlayton@kernel.org>