> On Nov 1, 2022, at 10:46 AM, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
