* [PATCH 0/3] nfsd: plug some filecache refcount leaks
@ 2024-07-10 13:05 Jeff Layton
2024-07-10 13:05 ` [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file Jeff Layton
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Jeff Layton @ 2024-07-10 13:05 UTC (permalink / raw)
To: Chuck Lever, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey
Cc: Youzhong Yang, linux-nfs, linux-kernel, 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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file
2024-07-10 13:05 [PATCH 0/3] nfsd: plug some filecache refcount leaks Jeff Layton
@ 2024-07-10 13:05 ` Jeff Layton
2024-07-11 17:05 ` Youzhong Yang
2024-07-10 13:05 ` [PATCH 2/3] nfsd: fix refcount leak when file is unhashed after being found Jeff Layton
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Jeff Layton @ 2024-07-10 13:05 UTC (permalink / raw)
To: Chuck Lever, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey
Cc: Youzhong Yang, linux-nfs, linux-kernel, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] nfsd: fix refcount leak when file is unhashed after being found
2024-07-10 13:05 [PATCH 0/3] nfsd: plug some filecache refcount leaks Jeff Layton
2024-07-10 13:05 ` [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file Jeff Layton
@ 2024-07-10 13:05 ` Jeff Layton
2024-07-10 13:05 ` [PATCH 3/3] nfsd: count nfsd_file allocations Jeff Layton
2024-07-10 14:35 ` [PATCH 0/3] nfsd: plug some filecache refcount leaks Chuck Lever
3 siblings, 0 replies; 11+ messages in thread
From: Jeff Layton @ 2024-07-10 13:05 UTC (permalink / raw)
To: Chuck Lever, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey
Cc: Youzhong Yang, linux-nfs, linux-kernel, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] nfsd: count nfsd_file allocations
2024-07-10 13:05 [PATCH 0/3] nfsd: plug some filecache refcount leaks Jeff Layton
2024-07-10 13:05 ` [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file Jeff Layton
2024-07-10 13:05 ` [PATCH 2/3] nfsd: fix refcount leak when file is unhashed after being found Jeff Layton
@ 2024-07-10 13:05 ` Jeff Layton
2024-07-10 14:35 ` [PATCH 0/3] nfsd: plug some filecache refcount leaks Chuck Lever
3 siblings, 0 replies; 11+ messages in thread
From: Jeff Layton @ 2024-07-10 13:05 UTC (permalink / raw)
To: Chuck Lever, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey
Cc: Youzhong Yang, linux-nfs, linux-kernel, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] nfsd: plug some filecache refcount leaks
2024-07-10 13:05 [PATCH 0/3] nfsd: plug some filecache refcount leaks Jeff Layton
` (2 preceding siblings ...)
2024-07-10 13:05 ` [PATCH 3/3] nfsd: count nfsd_file allocations Jeff Layton
@ 2024-07-10 14:35 ` Chuck Lever
2024-07-10 14:39 ` Jeff Layton
3 siblings, 1 reply; 11+ messages in thread
From: Chuck Lever @ 2024-07-10 14:35 UTC (permalink / raw)
To: Jeff Layton
Cc: Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Youzhong Yang,
linux-nfs, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] nfsd: plug some filecache refcount leaks
2024-07-10 14:35 ` [PATCH 0/3] nfsd: plug some filecache refcount leaks Chuck Lever
@ 2024-07-10 14:39 ` Jeff Layton
0 siblings, 0 replies; 11+ messages in thread
From: Jeff Layton @ 2024-07-10 14:39 UTC (permalink / raw)
To: Chuck Lever
Cc: Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Youzhong Yang,
linux-nfs, linux-kernel
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file
2024-07-10 13:05 ` [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file Jeff Layton
@ 2024-07-11 17:05 ` Youzhong Yang
2024-07-11 17:53 ` Jeff Layton
0 siblings, 1 reply; 11+ messages in thread
From: Youzhong Yang @ 2024-07-11 17:05 UTC (permalink / raw)
To: Jeff Layton
Cc: Chuck Lever, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey,
linux-nfs, linux-kernel
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file
2024-07-11 17:05 ` Youzhong Yang
@ 2024-07-11 17:53 ` Jeff Layton
2024-07-11 18:16 ` Chuck Lever III
0 siblings, 1 reply; 11+ messages in thread
From: Jeff Layton @ 2024-07-11 17:53 UTC (permalink / raw)
To: Youzhong Yang
Cc: Chuck Lever, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey,
linux-nfs, linux-kernel
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file
2024-07-11 17:53 ` Jeff Layton
@ 2024-07-11 18:16 ` Chuck Lever III
2024-07-11 18:20 ` Jeff Layton
0 siblings, 1 reply; 11+ messages in thread
From: Chuck Lever III @ 2024-07-11 18:16 UTC (permalink / raw)
To: Jeff Layton, Youzhong Yang
Cc: Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey,
Linux NFS Mailing List, Linux Kernel Mailing List
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file
2024-07-11 18:16 ` Chuck Lever III
@ 2024-07-11 18:20 ` Jeff Layton
2024-07-11 18:27 ` Chuck Lever III
0 siblings, 1 reply; 11+ messages in thread
From: Jeff Layton @ 2024-07-11 18:20 UTC (permalink / raw)
To: Chuck Lever III, Youzhong Yang
Cc: Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey,
Linux NFS Mailing List, Linux Kernel Mailing List
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file
2024-07-11 18:20 ` Jeff Layton
@ 2024-07-11 18:27 ` Chuck Lever III
0 siblings, 0 replies; 11+ messages in thread
From: Chuck Lever III @ 2024-07-11 18:27 UTC (permalink / raw)
To: Jeff Layton
Cc: Youzhong Yang, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey,
Linux NFS Mailing List, Linux Kernel Mailing List
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-07-11 18:28 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-10 13:05 [PATCH 0/3] nfsd: plug some filecache refcount leaks Jeff Layton
2024-07-10 13:05 ` [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file Jeff Layton
2024-07-11 17:05 ` Youzhong Yang
2024-07-11 17:53 ` Jeff Layton
2024-07-11 18:16 ` Chuck Lever III
2024-07-11 18:20 ` Jeff Layton
2024-07-11 18:27 ` Chuck Lever III
2024-07-10 13:05 ` [PATCH 2/3] nfsd: fix refcount leak when file is unhashed after being found Jeff Layton
2024-07-10 13:05 ` [PATCH 3/3] nfsd: count nfsd_file allocations Jeff Layton
2024-07-10 14:35 ` [PATCH 0/3] nfsd: plug some filecache refcount leaks Chuck Lever
2024-07-10 14:39 ` Jeff Layton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox