Re: [PATCH v2 6/7] nfsd: filecache: don't repeatedly add/remove files on the lru list

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2025-02-18 at 10:39 -0500, cel@kernel.org wrote:
> From: NeilBrown <neilb@suse.de>
> 
> There is no need to remove a file from the lru every time we access it,
> and then add it back.  It is sufficient to set the REFERENCED flag every
> time we put the file.  The order in the lru of REFERENCED files is
> largely irrelevant as they will all be moved to the end.
> 
> With this patch, files are added only when they are allocated (if
> want_gc) and they are removed only by the list_lru_(shrink_)walk
> callback or when forcibly removing a file.
> 
> This should reduce contention on the list_lru spinlock(s) and reduce
> memory traffic a little.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/filecache.c | 47 ++++++++++++++++-----------------------------
>  1 file changed, 17 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 0d621833a9f2..56935349f0e4 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -319,15 +319,14 @@ nfsd_file_check_writeback(struct nfsd_file *nf)
>  		mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
>  }
>  
> -static bool nfsd_file_lru_add(struct nfsd_file *nf)
> +static void nfsd_file_lru_add(struct nfsd_file *nf)
>  {
> -	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> -	set_bit(NFSD_FILE_RECENT, &nf->nf_flags);
> -	if (list_lru_add_obj(&nfsd_file_lru, &nf->nf_lru)) {
> +	refcount_inc(&nf->nf_ref);
> +	if (list_lru_add_obj(&nfsd_file_lru, &nf->nf_lru))
>  		trace_nfsd_file_lru_add(nf);
> -		return true;
> -	}
> -	return false;
> +	else
> +		WARN_ON(1);
> +	nfsd_file_schedule_laundrette();
>  }
>  
>  static bool nfsd_file_lru_remove(struct nfsd_file *nf)
> @@ -363,16 +362,10 @@ nfsd_file_put(struct nfsd_file *nf)
>  
>  	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 try to
> -		 * transfer it to the LRU.
> -		 */
> -		if (refcount_dec_not_one(&nf->nf_ref))
> -			return;
> -
> -		if (nfsd_file_lru_add(nf))
> -			return;
> +		set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> +		set_bit(NFSD_FILE_RECENT, &nf->nf_flags);
>  	}
> +
>  	if (refcount_dec_and_test(&nf->nf_ref))
>  		nfsd_file_free(nf);
>  }
> @@ -516,13 +509,12 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,
>  	}
>  
>  	/*
> -	 * 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.
> +	 * Put the reference held on behalf of the LRU if it is the last
> +	 * reference, else rotate.
>  	 */
> -	if (!refcount_dec_and_test(&nf->nf_ref)) {
> +	if (!refcount_dec_if_one(&nf->nf_ref)) {
>  		trace_nfsd_file_gc_in_use(nf);
> -		list_lru_isolate(lru, &nf->nf_lru);
> -		return LRU_REMOVED;
> +		return LRU_ROTATE;
>  	}
>  
>  	/* Refcount went to zero. Unhash it and queue it to the dispose list */
> @@ -1062,16 +1054,8 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct net *net,
>  	nf = nfsd_file_lookup_locked(net, current_cred(), inode, need, want_gc);
>  	rcu_read_unlock();
>  
> -	if (nf) {
> -		/*
> -		 * If the nf is on the LRU then it holds an extra reference
> -		 * that must be put if it's removed. It had better not be
> -		 * the last one however, since we should hold another.
> -		 */
> -		if (nfsd_file_lru_remove(nf))
> -			refcount_dec(&nf->nf_ref);
> +	if (nf)
>  		goto wait_for_construction;
> -	}
>  
>  	new = nfsd_file_alloc(net, inode, need, want_gc);
>  	if (!new) {
> @@ -1165,6 +1149,9 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct net *net,
>  	 */
>  	if (status != nfs_ok || inode->i_nlink == 0)
>  		nfsd_file_unhash(nf);
> +	else if (want_gc)
> +		nfsd_file_lru_add(nf);
> +
>  	clear_and_wake_up_bit(NFSD_FILE_PENDING, &nf->nf_flags);
>  	if (status == nfs_ok)
>  		goto out;

Reviewed-by: Jeff Layton <jlayton@kernel.org>