| Message ID | 20221028121353.33567-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: don't call nfsd_file_put from client states seqfile display | expand |
> On Oct 28, 2022, at 8:13 AM, Jeff Layton <jlayton@kernel.org> wrote: > > We had a report of this: > > BUG: sleeping function called from invalid context at fs/nfsd/filecache.c:440 > > ...with a stack trace showing nfsd_file_put being called from > nfs4_show_open. This code has always tried to call fput while holding a > spinlock, but we recently changed this to use the filecache, and that > started triggering the might_sleep() in nfsd_file_put. > > states_start takes and holds the cl_lock while iterating over the > client's states, and we can't sleep with that held. > > Have the various nfs4_show_* functions instead hold the fi_lock instead > of taking a nfsd_file reference. > > Fixes: 78599c42ae3c ("nfsd4: add file to display list of client's opens") > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2138357 > Reported-by: Zhi Li <yieli@redhat.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4state.c | 51 +++++++++++++++++++++++++++++---------------- > 1 file changed, 33 insertions(+), 18 deletions(-) Applied to nfsd's for-next (internally for now; will push later). > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 1ded89235111..dde621debeb2 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -675,15 +675,26 @@ find_any_file(struct nfs4_file *f) > return ret; > } > > -static struct nfsd_file *find_deleg_file(struct nfs4_file *f) > +static struct nfsd_file *find_any_file_locked(struct nfs4_file *f) > { > - struct nfsd_file *ret = NULL; > + lockdep_assert_held(&f->fi_lock); > + > + if (f->fi_fds[O_RDWR]) > + return f->fi_fds[O_RDWR]; > + if (f->fi_fds[O_WRONLY]) > + return f->fi_fds[O_WRONLY]; > + if (f->fi_fds[O_RDONLY]) > + return f->fi_fds[O_RDONLY]; > + return NULL; > +} > + > +static struct nfsd_file *find_deleg_file_locked(struct nfs4_file *f) > +{ > + lockdep_assert_held(&f->fi_lock); > > - spin_lock(&f->fi_lock); > if (f->fi_deleg_file) > - ret = nfsd_file_get(f->fi_deleg_file); > - spin_unlock(&f->fi_lock); > - return ret; > + return f->fi_deleg_file; > + return NULL; > } > > static atomic_long_t num_delegations; > @@ -2616,9 +2627,11 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st) > ols = openlockstateid(st); > oo = ols->st_stateowner; > nf = st->sc_file; > - file = find_any_file(nf); > + > + spin_lock(&nf->fi_lock); > + file = find_any_file_locked(nf); > if (!file) > - return 0; > + goto out; > > seq_printf(s, "- "); > nfs4_show_stateid(s, &st->sc_stateid); > @@ -2640,8 +2653,8 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st) > seq_printf(s, ", "); > nfs4_show_owner(s, oo); > seq_printf(s, " }\n"); > - nfsd_file_put(file); > - > +out: > + spin_unlock(&nf->fi_lock); > return 0; > } > > @@ -2655,9 +2668,10 @@ static int nfs4_show_lock(struct seq_file *s, struct nfs4_stid *st) > ols = openlockstateid(st); > oo = ols->st_stateowner; > nf = st->sc_file; > - file = find_any_file(nf); > + spin_lock(&nf->fi_lock); > + file = find_any_file_locked(nf); > if (!file) > - return 0; > + goto out; > > seq_printf(s, "- "); > nfs4_show_stateid(s, &st->sc_stateid); > @@ -2677,8 +2691,8 @@ static int nfs4_show_lock(struct seq_file *s, struct nfs4_stid *st) > seq_printf(s, ", "); > nfs4_show_owner(s, oo); > seq_printf(s, " }\n"); > - nfsd_file_put(file); > - > +out: > + spin_unlock(&nf->fi_lock); > return 0; > } > > @@ -2690,9 +2704,10 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st) > > ds = delegstateid(st); > nf = st->sc_file; > - file = find_deleg_file(nf); > + spin_lock(&nf->fi_lock); > + file = find_deleg_file_locked(nf); > if (!file) > - return 0; > + goto out; > > seq_printf(s, "- "); > nfs4_show_stateid(s, &st->sc_stateid); > @@ -2708,8 +2723,8 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st) > seq_printf(s, ", "); > nfs4_show_fname(s, file); > seq_printf(s, " }\n"); > - nfsd_file_put(file); > - > +out: > + spin_unlock(&nf->fi_lock); > return 0; > } > > -- > 2.37.3 > -- Chuck Lever
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 1ded89235111..dde621debeb2 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -675,15 +675,26 @@ find_any_file(struct nfs4_file *f) return ret; } -static struct nfsd_file *find_deleg_file(struct nfs4_file *f) +static struct nfsd_file *find_any_file_locked(struct nfs4_file *f) { - struct nfsd_file *ret = NULL; + lockdep_assert_held(&f->fi_lock); + + if (f->fi_fds[O_RDWR]) + return f->fi_fds[O_RDWR]; + if (f->fi_fds[O_WRONLY]) + return f->fi_fds[O_WRONLY]; + if (f->fi_fds[O_RDONLY]) + return f->fi_fds[O_RDONLY]; + return NULL; +} + +static struct nfsd_file *find_deleg_file_locked(struct nfs4_file *f) +{ + lockdep_assert_held(&f->fi_lock); - spin_lock(&f->fi_lock); if (f->fi_deleg_file) - ret = nfsd_file_get(f->fi_deleg_file); - spin_unlock(&f->fi_lock); - return ret; + return f->fi_deleg_file; + return NULL; } static atomic_long_t num_delegations; @@ -2616,9 +2627,11 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st) ols = openlockstateid(st); oo = ols->st_stateowner; nf = st->sc_file; - file = find_any_file(nf); + + spin_lock(&nf->fi_lock); + file = find_any_file_locked(nf); if (!file) - return 0; + goto out; seq_printf(s, "- "); nfs4_show_stateid(s, &st->sc_stateid); @@ -2640,8 +2653,8 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st) seq_printf(s, ", "); nfs4_show_owner(s, oo); seq_printf(s, " }\n"); - nfsd_file_put(file); - +out: + spin_unlock(&nf->fi_lock); return 0; } @@ -2655,9 +2668,10 @@ static int nfs4_show_lock(struct seq_file *s, struct nfs4_stid *st) ols = openlockstateid(st); oo = ols->st_stateowner; nf = st->sc_file; - file = find_any_file(nf); + spin_lock(&nf->fi_lock); + file = find_any_file_locked(nf); if (!file) - return 0; + goto out; seq_printf(s, "- "); nfs4_show_stateid(s, &st->sc_stateid); @@ -2677,8 +2691,8 @@ static int nfs4_show_lock(struct seq_file *s, struct nfs4_stid *st) seq_printf(s, ", "); nfs4_show_owner(s, oo); seq_printf(s, " }\n"); - nfsd_file_put(file); - +out: + spin_unlock(&nf->fi_lock); return 0; } @@ -2690,9 +2704,10 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st) ds = delegstateid(st); nf = st->sc_file; - file = find_deleg_file(nf); + spin_lock(&nf->fi_lock); + file = find_deleg_file_locked(nf); if (!file) - return 0; + goto out; seq_printf(s, "- "); nfs4_show_stateid(s, &st->sc_stateid); @@ -2708,8 +2723,8 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st) seq_printf(s, ", "); nfs4_show_fname(s, file); seq_printf(s, " }\n"); - nfsd_file_put(file); - +out: + spin_unlock(&nf->fi_lock); return 0; }
We had a report of this: BUG: sleeping function called from invalid context at fs/nfsd/filecache.c:440 ...with a stack trace showing nfsd_file_put being called from nfs4_show_open. This code has always tried to call fput while holding a spinlock, but we recently changed this to use the filecache, and that started triggering the might_sleep() in nfsd_file_put. states_start takes and holds the cl_lock while iterating over the client's states, and we can't sleep with that held. Have the various nfs4_show_* functions instead hold the fi_lock instead of taking a nfsd_file reference. Fixes: 78599c42ae3c ("nfsd4: add file to display list of client's opens") Link: https://bugzilla.redhat.com/show_bug.cgi?id=2138357 Reported-by: Zhi Li <yieli@redhat.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4state.c | 51 +++++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 18 deletions(-)