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 <[email protected]>
---
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);
+
}
struct nfsd_file *
--
2.37.3
> On Sep 30, 2022, at 4:58 PM, Jeff Layton <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
--
Chuck Lever
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 <[email protected]>
> ---
> 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,
--
Jeff Layton <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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
> >
> >
--
Jeff Layton <[email protected]>