| Message ID | 20221101144647.136696-5-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: clean up refcounting in the filecache | expand |
> On Nov 1, 2022, at 10:46 AM, Jeff Layton <jlayton@kernel.org> wrote: > > The list_lru_add and list_lru_del functions use list_empty checks to see > whether the object is already on the LRU. That's fine in most cases, but > we occasionally repurpose nf_lru after unhashing. It's possible for an > LRU removal to remove it from a different list altogether if we lose a > race. > > Add a new NFSD_FILE_LRU flag, which indicates that the object actually > resides on the LRU and not some other list. Use that when adding and > removing it from the LRU instead of just relying on list_empty checks. > > Add an extra HASHED check after adding the entry to the LRU. If it's now > clear, just remove it from the LRU again and put the reference if that > remove is successful. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/filecache.c | 50 ++++++++++++++++++++++++++++++--------------- > fs/nfsd/filecache.h | 1 + > 2 files changed, 35 insertions(+), 16 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index e67297ad12bf..bcea201d79c3 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -409,18 +409,22 @@ nfsd_file_check_writeback(struct nfsd_file *nf) > static bool nfsd_file_lru_add(struct nfsd_file *nf) > { > set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags); > - if (list_lru_add(&nfsd_file_lru, &nf->nf_lru)) { > - trace_nfsd_file_lru_add(nf); > - return true; > + if (!test_and_set_bit(NFSD_FILE_LRU, &nf->nf_flags)) { > + if (list_lru_add(&nfsd_file_lru, &nf->nf_lru)) { > + trace_nfsd_file_lru_add(nf); > + return true; > + } > } > return false; > } > > static bool nfsd_file_lru_remove(struct nfsd_file *nf) > { > - if (list_lru_del(&nfsd_file_lru, &nf->nf_lru)) { > - trace_nfsd_file_lru_del(nf); > - return true; > + if (test_and_clear_bit(NFSD_FILE_LRU, &nf->nf_flags)) { > + if (list_lru_del(&nfsd_file_lru, &nf->nf_lru)) { > + trace_nfsd_file_lru_del(nf); > + return true; > + } > } > return false; > } > @@ -476,21 +480,31 @@ nfsd_file_put(struct nfsd_file *nf) > might_sleep(); > trace_nfsd_file_put(nf); > > - /* > - * The HASHED check is racy. We may end up with the occasional > - * unhashed entry on the LRU, but they should get cleaned up > - * like any other. > - */ > if (test_bit(NFSD_FILE_GC, &nf->nf_flags) && > test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) { > /* > - * If this is the last reference (nf_ref == 1), then transfer > - * it to the LRU. If the add to the LRU fails, just put it as > - * usual. > + * If this is the last reference (nf_ref == 1), then try to > + * transfer it to the LRU. > */ > - if (refcount_dec_not_one(&nf->nf_ref) || nfsd_file_lru_add(nf)) { > - nfsd_file_schedule_laundrette(); > + if (refcount_dec_not_one(&nf->nf_ref)) > return; > + > + /* Try to add it to the LRU. If that fails, decrement. */ > + if (nfsd_file_lru_add(nf)) { > + /* If it's still hashed, we're done */ > + if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) { > + nfsd_file_schedule_laundrette(); > + return; > + } > + > + /* > + * We're racing with unhashing, so try to remove it from > + * the LRU. If removal fails, then someone else already > + * has our reference and we're done. If it succeeds, > + * fall through to decrement. > + */ > + if (!nfsd_file_lru_remove(nf)) > + return; > } > } > if (refcount_dec_and_test(&nf->nf_ref)) > @@ -594,6 +608,10 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru, > return LRU_ROTATE; > } > > + /* Make sure we're not racing with another removal. */ > + if (!test_and_clear_bit(NFSD_FILE_LRU, &nf->nf_flags)) > + return LRU_SKIP; The other return statements in nfsd_file_lru_cb have tracepoints so that garbage collection activity can be observed. Can you add a tracepoint for this new return as well? > + > /* > * Put the reference held on behalf of the LRU. If it wasn't the last > * one, then just remove it from the LRU and ignore it. > diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h > index b7efb2c3ddb1..e52ab7d5a44c 100644 > --- a/fs/nfsd/filecache.h > +++ b/fs/nfsd/filecache.h > @@ -39,6 +39,7 @@ struct nfsd_file { > #define NFSD_FILE_PENDING (1) > #define NFSD_FILE_REFERENCED (2) > #define NFSD_FILE_GC (3) > +#define NFSD_FILE_LRU (4) /* file is on LRU */ > unsigned long nf_flags; > struct inode *nf_inode; /* don't deref */ > refcount_t nf_ref; > -- > 2.38.1 > -- Chuck Lever
> On Nov 1, 2022, at 10:46 AM, Jeff Layton <jlayton@kernel.org> wrote: > > The list_lru_add and list_lru_del functions use list_empty checks to see > whether the object is already on the LRU. That's fine in most cases, but > we occasionally repurpose nf_lru after unhashing. It's possible for an > LRU removal to remove it from a different list altogether if we lose a > race. > > Add a new NFSD_FILE_LRU flag, which indicates that the object actually > resides on the LRU and not some other list. Use that when adding and > removing it from the LRU instead of just relying on list_empty checks. > > Add an extra HASHED check after adding the entry to the LRU. If it's now > clear, just remove it from the LRU again and put the reference if that > remove is successful. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/filecache.c | 50 ++++++++++++++++++++++++++++++--------------- > fs/nfsd/filecache.h | 1 + > 2 files changed, 35 insertions(+), 16 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index e67297ad12bf..bcea201d79c3 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -409,18 +409,22 @@ nfsd_file_check_writeback(struct nfsd_file *nf) > static bool nfsd_file_lru_add(struct nfsd_file *nf) > { > set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags); > - if (list_lru_add(&nfsd_file_lru, &nf->nf_lru)) { > - trace_nfsd_file_lru_add(nf); > - return true; > + if (!test_and_set_bit(NFSD_FILE_LRU, &nf->nf_flags)) { > + if (list_lru_add(&nfsd_file_lru, &nf->nf_lru)) { > + trace_nfsd_file_lru_add(nf); > + return true; > + } > } > return false; > } > > static bool nfsd_file_lru_remove(struct nfsd_file *nf) > { > - if (list_lru_del(&nfsd_file_lru, &nf->nf_lru)) { > - trace_nfsd_file_lru_del(nf); > - return true; > + if (test_and_clear_bit(NFSD_FILE_LRU, &nf->nf_flags)) { > + if (list_lru_del(&nfsd_file_lru, &nf->nf_lru)) { > + trace_nfsd_file_lru_del(nf); > + return true; > + } > } > return false; > } > @@ -476,21 +480,31 @@ nfsd_file_put(struct nfsd_file *nf) > might_sleep(); > trace_nfsd_file_put(nf); > > - /* > - * The HASHED check is racy. We may end up with the occasional > - * unhashed entry on the LRU, but they should get cleaned up > - * like any other. > - */ > if (test_bit(NFSD_FILE_GC, &nf->nf_flags) && > test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) { > /* > - * If this is the last reference (nf_ref == 1), then transfer > - * it to the LRU. If the add to the LRU fails, just put it as > - * usual. > + * If this is the last reference (nf_ref == 1), then try to > + * transfer it to the LRU. > */ > - if (refcount_dec_not_one(&nf->nf_ref) || nfsd_file_lru_add(nf)) { > - nfsd_file_schedule_laundrette(); > + if (refcount_dec_not_one(&nf->nf_ref)) > return; > + > + /* Try to add it to the LRU. If that fails, decrement. */ > + if (nfsd_file_lru_add(nf)) { > + /* If it's still hashed, we're done */ > + if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) { > + nfsd_file_schedule_laundrette(); > + return; > + } > + > + /* > + * We're racing with unhashing, so try to remove it from > + * the LRU. If removal fails, then someone else already > + * has our reference and we're done. If it succeeds, > + * fall through to decrement. > + */ > + if (!nfsd_file_lru_remove(nf)) > + return; > } > } > if (refcount_dec_and_test(&nf->nf_ref)) > @@ -594,6 +608,10 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru, > return LRU_ROTATE; > } > > + /* Make sure we're not racing with another removal. */ > + if (!test_and_clear_bit(NFSD_FILE_LRU, &nf->nf_flags)) > + return LRU_SKIP; > + > /* > * Put the reference held on behalf of the LRU. If it wasn't the last > * one, then just remove it from the LRU and ignore it. > diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h > index b7efb2c3ddb1..e52ab7d5a44c 100644 > --- a/fs/nfsd/filecache.h > +++ b/fs/nfsd/filecache.h > @@ -39,6 +39,7 @@ struct nfsd_file { > #define NFSD_FILE_PENDING (1) > #define NFSD_FILE_REFERENCED (2) > #define NFSD_FILE_GC (3) > +#define NFSD_FILE_LRU (4) /* file is on LRU */ Be sure to add NFSD_FILE_LRU to the show_nf_flags() macro: 866 /* 867 * from fs/nfsd/filecache.h 868 */ 869 #define show_nf_flags(val) \ 870 __print_flags(val, "|", \ 871 { 1 << NFSD_FILE_HASHED, "HASHED" }, \ 872 { 1 << NFSD_FILE_PENDING, "PENDING" }, \ 873 { 1 << NFSD_FILE_REFERENCED, "REFERENCED"}, \ 874 { 1 << NFSD_FILE_GC, "GC"}) > unsigned long nf_flags; > struct inode *nf_inode; /* don't deref */ > refcount_t nf_ref; > -- > 2.38.1 > -- Chuck Lever
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index e67297ad12bf..bcea201d79c3 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -409,18 +409,22 @@ nfsd_file_check_writeback(struct nfsd_file *nf) static bool nfsd_file_lru_add(struct nfsd_file *nf) { set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags); - if (list_lru_add(&nfsd_file_lru, &nf->nf_lru)) { - trace_nfsd_file_lru_add(nf); - return true; + if (!test_and_set_bit(NFSD_FILE_LRU, &nf->nf_flags)) { + if (list_lru_add(&nfsd_file_lru, &nf->nf_lru)) { + trace_nfsd_file_lru_add(nf); + return true; + } } return false; } static bool nfsd_file_lru_remove(struct nfsd_file *nf) { - if (list_lru_del(&nfsd_file_lru, &nf->nf_lru)) { - trace_nfsd_file_lru_del(nf); - return true; + if (test_and_clear_bit(NFSD_FILE_LRU, &nf->nf_flags)) { + if (list_lru_del(&nfsd_file_lru, &nf->nf_lru)) { + trace_nfsd_file_lru_del(nf); + return true; + } } return false; } @@ -476,21 +480,31 @@ nfsd_file_put(struct nfsd_file *nf) might_sleep(); trace_nfsd_file_put(nf); - /* - * The HASHED check is racy. We may end up with the occasional - * unhashed entry on the LRU, but they should get cleaned up - * like any other. - */ if (test_bit(NFSD_FILE_GC, &nf->nf_flags) && test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) { /* - * If this is the last reference (nf_ref == 1), then transfer - * it to the LRU. If the add to the LRU fails, just put it as - * usual. + * If this is the last reference (nf_ref == 1), then try to + * transfer it to the LRU. */ - if (refcount_dec_not_one(&nf->nf_ref) || nfsd_file_lru_add(nf)) { - nfsd_file_schedule_laundrette(); + if (refcount_dec_not_one(&nf->nf_ref)) return; + + /* Try to add it to the LRU. If that fails, decrement. */ + if (nfsd_file_lru_add(nf)) { + /* If it's still hashed, we're done */ + if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) { + nfsd_file_schedule_laundrette(); + return; + } + + /* + * We're racing with unhashing, so try to remove it from + * the LRU. If removal fails, then someone else already + * has our reference and we're done. If it succeeds, + * fall through to decrement. + */ + if (!nfsd_file_lru_remove(nf)) + return; } } if (refcount_dec_and_test(&nf->nf_ref)) @@ -594,6 +608,10 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru, return LRU_ROTATE; } + /* Make sure we're not racing with another removal. */ + if (!test_and_clear_bit(NFSD_FILE_LRU, &nf->nf_flags)) + return LRU_SKIP; + /* * Put the reference held on behalf of the LRU. If it wasn't the last * one, then just remove it from the LRU and ignore it. diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h index b7efb2c3ddb1..e52ab7d5a44c 100644 --- a/fs/nfsd/filecache.h +++ b/fs/nfsd/filecache.h @@ -39,6 +39,7 @@ struct nfsd_file { #define NFSD_FILE_PENDING (1) #define NFSD_FILE_REFERENCED (2) #define NFSD_FILE_GC (3) +#define NFSD_FILE_LRU (4) /* file is on LRU */ unsigned long nf_flags; struct inode *nf_inode; /* don't deref */ refcount_t nf_ref;
The list_lru_add and list_lru_del functions use list_empty checks to see whether the object is already on the LRU. That's fine in most cases, but we occasionally repurpose nf_lru after unhashing. It's possible for an LRU removal to remove it from a different list altogether if we lose a race. Add a new NFSD_FILE_LRU flag, which indicates that the object actually resides on the LRU and not some other list. Use that when adding and removing it from the LRU instead of just relying on list_empty checks. Add an extra HASHED check after adding the entry to the LRU. If it's now clear, just remove it from the LRU again and put the reference if that remove is successful. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/filecache.c | 50 ++++++++++++++++++++++++++++++--------------- fs/nfsd/filecache.h | 1 + 2 files changed, 35 insertions(+), 16 deletions(-)