| Message ID | 20221027215213.138304-3-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: clean up refcounting in filecache | expand |
On Fri, 28 Oct 2022, Jeff Layton wrote: > Currently, nfsd_files live on the LRU once they are added until they are > unhashed. There's no need to keep ones that are actively in use there. Is that true? nfsd_file_do_acquire() calls nfsd_file_lru_remove() Isn't that enough to keep the file off the lru while it is active? Thanks, NeilBrown > > Before incrementing the refcount, do a lockless check for nf_lru being > empty. If it's not then attempt to remove the entry from the LRU. If > that's successful, claim the LRU reference and return it. If the removal > fails (or if the list_head was empty), then just increment the counter > as we normally would. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/filecache.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index e63534f4b9f8..d2bbded805d4 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -420,14 +420,31 @@ nfsd_file_unhash(struct nfsd_file *nf) > return false; > } > > -struct nfsd_file * > -nfsd_file_get(struct nfsd_file *nf) > +static struct nfsd_file * > +__nfsd_file_get(struct nfsd_file *nf) > { > if (likely(refcount_inc_not_zero(&nf->nf_ref))) > return nf; > return NULL; > } > > +struct nfsd_file * > +nfsd_file_get(struct nfsd_file *nf) > +{ > + /* > + * Do a lockless list_empty check first, before attempting to > + * remove it, so we can avoid the spinlock when it's not on the > + * list. > + * > + * If we successfully remove it from the LRU, then we can just > + * claim the LRU reference and return it. Otherwise, we need to > + * bump the counter the old-fashioned way. > + */ > + if (!list_empty(&nf->nf_lru) && nfsd_file_lru_remove(nf)) > + return nf; > + return __nfsd_file_get(nf); > +} > + > /** > * nfsd_file_unhash_and_queue - unhash a file and queue it to the dispose list > * @nf: nfsd_file to be unhashed and queued > @@ -449,7 +466,7 @@ nfsd_file_unhash_and_queue(struct nfsd_file *nf, struct list_head *dispose) > * to take a reference. If that fails, just ignore > * the file altogether. > */ > - if (!nfsd_file_lru_remove(nf) && !nfsd_file_get(nf)) > + if (!nfsd_file_lru_remove(nf) && !__nfsd_file_get(nf)) > return false; > list_add(&nf->nf_lru, dispose); > return true; > -- > 2.37.3 > >
On Fri, 2022-10-28 at 09:20 +1100, NeilBrown wrote: > On Fri, 28 Oct 2022, Jeff Layton wrote: > > Currently, nfsd_files live on the LRU once they are added until they are > > unhashed. There's no need to keep ones that are actively in use there. > > Is that true? > nfsd_file_do_acquire() calls nfsd_file_lru_remove() > Isn't that enough to keep the file off the lru while it is active? > > Thanks, > NeilBrown > After patch #1, it doesn't call that anymore. That's probably a (minor) regression then. After patch #1, the LRU holds a reference. If you successfully remove it from the LRU, you need to transfer or put that reference. Doing the LRU handling in the get and put routines seems more natural, I think. Maybe I just need to squash this patch into #1? > > > > > Before incrementing the refcount, do a lockless check for nf_lru being > > empty. If it's not then attempt to remove the entry from the LRU. If > > that's successful, claim the LRU reference and return it. If the removal > > fails (or if the list_head was empty), then just increment the counter > > as we normally would. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/filecache.c | 23 ++++++++++++++++++++--- > > 1 file changed, 20 insertions(+), 3 deletions(-) > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > index e63534f4b9f8..d2bbded805d4 100644 > > --- a/fs/nfsd/filecache.c > > +++ b/fs/nfsd/filecache.c > > @@ -420,14 +420,31 @@ nfsd_file_unhash(struct nfsd_file *nf) > > return false; > > } > > > > -struct nfsd_file * > > -nfsd_file_get(struct nfsd_file *nf) > > +static struct nfsd_file * > > +__nfsd_file_get(struct nfsd_file *nf) > > { > > if (likely(refcount_inc_not_zero(&nf->nf_ref))) > > return nf; > > return NULL; > > } > > > > +struct nfsd_file * > > +nfsd_file_get(struct nfsd_file *nf) > > +{ > > + /* > > + * Do a lockless list_empty check first, before attempting to > > + * remove it, so we can avoid the spinlock when it's not on the > > + * list. > > + * > > + * If we successfully remove it from the LRU, then we can just > > + * claim the LRU reference and return it. Otherwise, we need to > > + * bump the counter the old-fashioned way. > > + */ > > + if (!list_empty(&nf->nf_lru) && nfsd_file_lru_remove(nf)) > > + return nf; > > + return __nfsd_file_get(nf); > > +} > > + > > /** > > * nfsd_file_unhash_and_queue - unhash a file and queue it to the dispose list > > * @nf: nfsd_file to be unhashed and queued > > @@ -449,7 +466,7 @@ nfsd_file_unhash_and_queue(struct nfsd_file *nf, struct list_head *dispose) > > * to take a reference. If that fails, just ignore > > * the file altogether. > > */ > > - if (!nfsd_file_lru_remove(nf) && !nfsd_file_get(nf)) > > + if (!nfsd_file_lru_remove(nf) && !__nfsd_file_get(nf)) > > return false; > > list_add(&nf->nf_lru, dispose); > > return true; > > -- > > 2.37.3 > > > >
On Fri, 28 Oct 2022, Jeff Layton wrote: > On Fri, 2022-10-28 at 09:20 +1100, NeilBrown wrote: > > On Fri, 28 Oct 2022, Jeff Layton wrote: > > > Currently, nfsd_files live on the LRU once they are added until they are > > > unhashed. There's no need to keep ones that are actively in use there. > > > > Is that true? > > nfsd_file_do_acquire() calls nfsd_file_lru_remove() > > Isn't that enough to keep the file off the lru while it is active? > > > > Thanks, > > NeilBrown > > > > After patch #1, it doesn't call that anymore. That's probably a (minor) > regression then. Yes, I eventually found that - thanks. > > After patch #1, the LRU holds a reference. If you successfully remove it > from the LRU, you need to transfer or put that reference. Doing the LRU > handling in the get and put routines seems more natural, I think. Maybe. But then you need a __get as well as a get. Though it might seem asymmetric, I would prefer removing from the lru in 'acquire' and adding to the lru in put. > > Maybe I just need to squash this patch into #1? Or do the "put" if lru_remove succeeds in the first patch. Then revise it all in the second. Thanks, NeilBrown > > > > > > > > > Before incrementing the refcount, do a lockless check for nf_lru being > > > empty. If it's not then attempt to remove the entry from the LRU. If > > > that's successful, claim the LRU reference and return it. If the removal > > > fails (or if the list_head was empty), then just increment the counter > > > as we normally would. > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > fs/nfsd/filecache.c | 23 ++++++++++++++++++++--- > > > 1 file changed, 20 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > > index e63534f4b9f8..d2bbded805d4 100644 > > > --- a/fs/nfsd/filecache.c > > > +++ b/fs/nfsd/filecache.c > > > @@ -420,14 +420,31 @@ nfsd_file_unhash(struct nfsd_file *nf) > > > return false; > > > } > > > > > > -struct nfsd_file * > > > -nfsd_file_get(struct nfsd_file *nf) > > > +static struct nfsd_file * > > > +__nfsd_file_get(struct nfsd_file *nf) > > > { > > > if (likely(refcount_inc_not_zero(&nf->nf_ref))) > > > return nf; > > > return NULL; > > > } > > > > > > +struct nfsd_file * > > > +nfsd_file_get(struct nfsd_file *nf) > > > +{ > > > + /* > > > + * Do a lockless list_empty check first, before attempting to > > > + * remove it, so we can avoid the spinlock when it's not on the > > > + * list. > > > + * > > > + * If we successfully remove it from the LRU, then we can just > > > + * claim the LRU reference and return it. Otherwise, we need to > > > + * bump the counter the old-fashioned way. > > > + */ > > > + if (!list_empty(&nf->nf_lru) && nfsd_file_lru_remove(nf)) > > > + return nf; > > > + return __nfsd_file_get(nf); > > > +} > > > + > > > /** > > > * nfsd_file_unhash_and_queue - unhash a file and queue it to the dispose list > > > * @nf: nfsd_file to be unhashed and queued > > > @@ -449,7 +466,7 @@ nfsd_file_unhash_and_queue(struct nfsd_file *nf, struct list_head *dispose) > > > * to take a reference. If that fails, just ignore > > > * the file altogether. > > > */ > > > - if (!nfsd_file_lru_remove(nf) && !nfsd_file_get(nf)) > > > + if (!nfsd_file_lru_remove(nf) && !__nfsd_file_get(nf)) > > > return false; > > > list_add(&nf->nf_lru, dispose); > > > return true; > > > -- > > > 2.37.3 > > > > > > > > -- > Jeff Layton <jlayton@kernel.org> >
> On Oct 27, 2022, at 6:55 PM, NeilBrown <neilb@suse.de> wrote: > > On Fri, 28 Oct 2022, Jeff Layton wrote: >> On Fri, 2022-10-28 at 09:20 +1100, NeilBrown wrote: >>> On Fri, 28 Oct 2022, Jeff Layton wrote: >>>> Currently, nfsd_files live on the LRU once they are added until they are >>>> unhashed. There's no need to keep ones that are actively in use there. >>> >>> Is that true? >>> nfsd_file_do_acquire() calls nfsd_file_lru_remove() >>> Isn't that enough to keep the file off the lru while it is active? >>> >>> Thanks, >>> NeilBrown >>> >> >> After patch #1, it doesn't call that anymore. That's probably a (minor) >> regression then. > > Yes, I eventually found that - thanks. > >> >> After patch #1, the LRU holds a reference. If you successfully remove it >> from the LRU, you need to transfer or put that reference. Doing the LRU >> handling in the get and put routines seems more natural, I think. > > Maybe. But then you need a __get as well as a get. > Though it might seem asymmetric, I would prefer removing from the lru in > 'acquire' and adding to the lru in put. That's exactly the design introduced by commit 4a0e73e635e3 ("NFSD: Leave open files out of the filecache LRU"). I also would like to keep that behavior -- that's what a real LRU is for. >> Maybe I just need to squash this patch into #1? > > Or do the "put" if lru_remove succeeds in the first patch. Then revise > it all in the second. > > Thanks, > NeilBrown > > >> >>> >>>> >>>> Before incrementing the refcount, do a lockless check for nf_lru being >>>> empty. If it's not then attempt to remove the entry from the LRU. If >>>> that's successful, claim the LRU reference and return it. If the removal >>>> fails (or if the list_head was empty), then just increment the counter >>>> as we normally would. >>>> >>>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>>> --- >>>> fs/nfsd/filecache.c | 23 ++++++++++++++++++++--- >>>> 1 file changed, 20 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c >>>> index e63534f4b9f8..d2bbded805d4 100644 >>>> --- a/fs/nfsd/filecache.c >>>> +++ b/fs/nfsd/filecache.c >>>> @@ -420,14 +420,31 @@ nfsd_file_unhash(struct nfsd_file *nf) >>>> return false; >>>> } >>>> >>>> -struct nfsd_file * >>>> -nfsd_file_get(struct nfsd_file *nf) >>>> +static struct nfsd_file * >>>> +__nfsd_file_get(struct nfsd_file *nf) >>>> { >>>> if (likely(refcount_inc_not_zero(&nf->nf_ref))) >>>> return nf; >>>> return NULL; >>>> } >>>> >>>> +struct nfsd_file * >>>> +nfsd_file_get(struct nfsd_file *nf) >>>> +{ >>>> + /* >>>> + * Do a lockless list_empty check first, before attempting to >>>> + * remove it, so we can avoid the spinlock when it's not on the >>>> + * list. >>>> + * >>>> + * If we successfully remove it from the LRU, then we can just >>>> + * claim the LRU reference and return it. Otherwise, we need to >>>> + * bump the counter the old-fashioned way. >>>> + */ >>>> + if (!list_empty(&nf->nf_lru) && nfsd_file_lru_remove(nf)) >>>> + return nf; >>>> + return __nfsd_file_get(nf); >>>> +} >>>> + >>>> /** >>>> * nfsd_file_unhash_and_queue - unhash a file and queue it to the dispose list >>>> * @nf: nfsd_file to be unhashed and queued >>>> @@ -449,7 +466,7 @@ nfsd_file_unhash_and_queue(struct nfsd_file *nf, struct list_head *dispose) >>>> * to take a reference. If that fails, just ignore >>>> * the file altogether. >>>> */ >>>> - if (!nfsd_file_lru_remove(nf) && !nfsd_file_get(nf)) >>>> + if (!nfsd_file_lru_remove(nf) && !__nfsd_file_get(nf)) >>>> return false; >>>> list_add(&nf->nf_lru, dispose); >>>> return true; >>>> -- >>>> 2.37.3 >>>> >>>> >> >> -- >> Jeff Layton <jlayton@kernel.org> >> -- Chuck Lever
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index e63534f4b9f8..d2bbded805d4 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -420,14 +420,31 @@ nfsd_file_unhash(struct nfsd_file *nf) return false; } -struct nfsd_file * -nfsd_file_get(struct nfsd_file *nf) +static struct nfsd_file * +__nfsd_file_get(struct nfsd_file *nf) { if (likely(refcount_inc_not_zero(&nf->nf_ref))) return nf; return NULL; } +struct nfsd_file * +nfsd_file_get(struct nfsd_file *nf) +{ + /* + * Do a lockless list_empty check first, before attempting to + * remove it, so we can avoid the spinlock when it's not on the + * list. + * + * If we successfully remove it from the LRU, then we can just + * claim the LRU reference and return it. Otherwise, we need to + * bump the counter the old-fashioned way. + */ + if (!list_empty(&nf->nf_lru) && nfsd_file_lru_remove(nf)) + return nf; + return __nfsd_file_get(nf); +} + /** * nfsd_file_unhash_and_queue - unhash a file and queue it to the dispose list * @nf: nfsd_file to be unhashed and queued @@ -449,7 +466,7 @@ nfsd_file_unhash_and_queue(struct nfsd_file *nf, struct list_head *dispose) * to take a reference. If that fails, just ignore * the file altogether. */ - if (!nfsd_file_lru_remove(nf) && !nfsd_file_get(nf)) + if (!nfsd_file_lru_remove(nf) && !__nfsd_file_get(nf)) return false; list_add(&nf->nf_lru, dispose); return true;
Currently, nfsd_files live on the LRU once they are added until they are unhashed. There's no need to keep ones that are actively in use there. Before incrementing the refcount, do a lockless check for nf_lru being empty. If it's not then attempt to remove the entry from the LRU. If that's successful, claim the LRU reference and return it. If the removal fails (or if the list_head was empty), then just increment the counter as we normally would. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/filecache.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)