| Message ID | 20220930191550.172087-3-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: filecache fixes | expand |
On Fri, 2022-09-30 at 15:15 -0400, Jeff Layton wrote: > Once we call nfsd_file_put, there is no guarantee that "nf" can still be > safely accessed. That may have been the last reference. > > Change the code to instead check for whether nf_ref is 2 and then unhash > it and put the reference if we're successful. > > We might occasionally race with another lookup and end up unhashing it > when it probably shouldn't have been, but that should hopefully be rare > and will just result in the competing lookup having to create a new > nfsd_file. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/filecache.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index 6237715bd23e..58f4d9267f4a 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -461,12 +461,14 @@ nfsd_file_put(struct nfsd_file *nf) > */ > void nfsd_file_close(struct nfsd_file *nf) > { > - nfsd_file_put(nf); > - if (refcount_dec_if_one(&nf->nf_ref)) { > - nfsd_file_unhash(nf); > - nfsd_file_lru_remove(nf); > - nfsd_file_free(nf); > + /* One for the reference being put, and one for the hash */ > + if (refcount_read(&nf->nf_ref) == 2) { > + if (nfsd_file_unhash(nf)) > + nfsd_file_put_noref(nf); > } > + /* put the ref for the stateid */ > + nfsd_file_put(nf); > + Chuck if you're ok with this one, can you fix the stray newline above? > } > > struct nfsd_file * Thanks,
> On Sep 30, 2022, at 4:58 PM, Jeff Layton <jlayton@kernel.org> wrote: > > On Fri, 2022-09-30 at 15:15 -0400, Jeff Layton wrote: >> Once we call nfsd_file_put, there is no guarantee that "nf" can still be >> safely accessed. That may have been the last reference. >> >> Change the code to instead check for whether nf_ref is 2 and then unhash >> it and put the reference if we're successful. >> >> We might occasionally race with another lookup and end up unhashing it >> when it probably shouldn't have been, but that should hopefully be rare >> and will just result in the competing lookup having to create a new >> nfsd_file. >> >> Signed-off-by: Jeff Layton <jlayton@kernel.org> >> --- >> fs/nfsd/filecache.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c >> index 6237715bd23e..58f4d9267f4a 100644 >> --- a/fs/nfsd/filecache.c >> +++ b/fs/nfsd/filecache.c >> @@ -461,12 +461,14 @@ nfsd_file_put(struct nfsd_file *nf) >> */ >> void nfsd_file_close(struct nfsd_file *nf) >> { >> - nfsd_file_put(nf); >> - if (refcount_dec_if_one(&nf->nf_ref)) { >> - nfsd_file_unhash(nf); >> - nfsd_file_lru_remove(nf); >> - nfsd_file_free(nf); >> + /* One for the reference being put, and one for the hash */ >> + if (refcount_read(&nf->nf_ref) == 2) { >> + if (nfsd_file_unhash(nf)) >> + nfsd_file_put_noref(nf); >> } >> + /* put the ref for the stateid */ >> + nfsd_file_put(nf); >> + > > Chuck if you're ok with this one, can you fix the stray newline above? Sure. >> } >> >> struct nfsd_file * > > > Thanks, > -- > Jeff Layton <jlayton@kernel.org> -- Chuck Lever
On Sat, 01 Oct 2022, Jeff Layton wrote: > Once we call nfsd_file_put, there is no guarantee that "nf" can still be > safely accessed. That may have been the last reference. > > Change the code to instead check for whether nf_ref is 2 and then unhash > it and put the reference if we're successful. > > We might occasionally race with another lookup and end up unhashing it > when it probably shouldn't have been, but that should hopefully be rare > and will just result in the competing lookup having to create a new > nfsd_file. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/filecache.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index 6237715bd23e..58f4d9267f4a 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -461,12 +461,14 @@ nfsd_file_put(struct nfsd_file *nf) > */ > void nfsd_file_close(struct nfsd_file *nf) > { > - nfsd_file_put(nf); > - if (refcount_dec_if_one(&nf->nf_ref)) { > - nfsd_file_unhash(nf); > - nfsd_file_lru_remove(nf); > - nfsd_file_free(nf); > + /* One for the reference being put, and one for the hash */ > + if (refcount_read(&nf->nf_ref) == 2) { > + if (nfsd_file_unhash(nf)) > + nfsd_file_put_noref(nf); > } > + /* put the ref for the stateid */ > + nfsd_file_put(nf); > + This looks racy. What if a get happens after the read and before the unhash? If we unhash the nfsd_file at last close, why does the hash table hold a counted reference at all? When it is hashed, set the NFSD_FILE_HASHED flag. On last-put, if that flag is set, unhash it. If you want to unhash it earlier, test/clear the flag and delete from rhashtable. NeilBrown > } > > struct nfsd_file * > -- > 2.37.3 > >
On Sat, 2022-10-01 at 15:03 +1000, NeilBrown wrote: > On Sat, 01 Oct 2022, Jeff Layton wrote: > > Once we call nfsd_file_put, there is no guarantee that "nf" can still be > > safely accessed. That may have been the last reference. > > > > Change the code to instead check for whether nf_ref is 2 and then unhash > > it and put the reference if we're successful. > > > > We might occasionally race with another lookup and end up unhashing it > > when it probably shouldn't have been, but that should hopefully be rare > > and will just result in the competing lookup having to create a new > > nfsd_file. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/filecache.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > index 6237715bd23e..58f4d9267f4a 100644 > > --- a/fs/nfsd/filecache.c > > +++ b/fs/nfsd/filecache.c > > @@ -461,12 +461,14 @@ nfsd_file_put(struct nfsd_file *nf) > > */ > > void nfsd_file_close(struct nfsd_file *nf) > > { > > - nfsd_file_put(nf); > > - if (refcount_dec_if_one(&nf->nf_ref)) { > > - nfsd_file_unhash(nf); > > - nfsd_file_lru_remove(nf); > > - nfsd_file_free(nf); > > + /* One for the reference being put, and one for the hash */ > > + if (refcount_read(&nf->nf_ref) == 2) { > > + if (nfsd_file_unhash(nf)) > > + nfsd_file_put_noref(nf); > > } > > + /* put the ref for the stateid */ > > + nfsd_file_put(nf); > > + > > This looks racy. What if a get happens after the read and before the unhash? > It depends on whether the "getter" sees the HASHED flag or not in nfsd_file_do_acquire. If HASHED is still set, then it'll get a reference to the old soon to be unhashed nfsd_file. If it's no longer HASHED in nfsd_file_do_acquire, it will fall into the "Did construction of this file fail?" case, and either retry the lookup or return nfserr_jukebox. Either is an acceptable outcome since this should presumably be a rare occurrence. > If we unhash the nfsd_file at last close, why does the hash table hold a > counted reference at all? > When it is hashed, set the NFSD_FILE_HASHED flag. On last-put, if that > flag is set, unhash it. > If you want to unhash it earlier, test/clear the flag and delete from > rhashtable. > That's not the way the refcounting works today and I don't see a clear benefit to making that change. If you want to propose patches to rework it, I'd be happy to review them though. > > > > } > > > > struct nfsd_file * > > -- > > 2.37.3 > > > >
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index 6237715bd23e..58f4d9267f4a 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -461,12 +461,14 @@ nfsd_file_put(struct nfsd_file *nf) */ void nfsd_file_close(struct nfsd_file *nf) { - nfsd_file_put(nf); - if (refcount_dec_if_one(&nf->nf_ref)) { - nfsd_file_unhash(nf); - nfsd_file_lru_remove(nf); - nfsd_file_free(nf); + /* One for the reference being put, and one for the hash */ + if (refcount_read(&nf->nf_ref) == 2) { + if (nfsd_file_unhash(nf)) + nfsd_file_put_noref(nf); } + /* put the ref for the stateid */ + nfsd_file_put(nf); + } struct nfsd_file *
Once we call nfsd_file_put, there is no guarantee that "nf" can still be safely accessed. That may have been the last reference. Change the code to instead check for whether nf_ref is 2 and then unhash it and put the reference if we're successful. We might occasionally race with another lookup and end up unhashing it when it probably shouldn't have been, but that should hopefully be rare and will just result in the competing lookup having to create a new nfsd_file. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/filecache.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)