Re: [PATCH 3/3] nfsd: fix nfsd_file_unhash_and_dispose

[Chuck Lever III <chuck.lever@oracle.com>]

> On Sep 30, 2022, at 3:15 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> This function is called two reasons:
> 
> We're either shutting down and purging the filecache, or we've gotten a
> notification about a file delete, so we want to go ahead and unhash it
> so that it'll get cleaned up when we close.
> 
> We're either walking the hashtable or doing a lookup in it and we
> don't take a reference in either case. What we want to do in both cases
> is to try and unhash the object and put it on the dispose list if that
> was successful. If it's no longer hashed, then we don't want to touch
> it, with the assumption being that something else is already cleaning
> up the sentinel reference.
> 
> Instead of trying to selectively decrement the refcount in this
> function, just unhash it, and if that was successful, move it to the
> dispose list. Then, the disposal routine will just clean that up as
> usual.
> 
> Also, just make this a void function, drop the WARN_ON_ONCE, and the
> comments about deadlocking since the nature of the purported deadlock
> is no longer clear.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/filecache.c | 32 ++++++--------------------------
> 1 file changed, 6 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 58f4d9267f4a..16bd71a3894e 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -408,19 +408,14 @@ nfsd_file_unhash(struct nfsd_file *nf)
> /*
>  * Return true if the file was unhashed.
>  */

If you're changing the function to return void, the above
comment is now stale.

> -static bool
> +static void
> nfsd_file_unhash_and_dispose(struct nfsd_file *nf, struct list_head *dispose)
> {
> 	trace_nfsd_file_unhash_and_dispose(nf);
> -	if (!nfsd_file_unhash(nf))
> -		return false;
> -	/* keep final reference for nfsd_file_lru_dispose */

This comment has been stale since nfsd_file_lru_dispose() was
renamed or removed. The only trouble I have is there isn't a
comment left that explains why we're not decrementing the hash
table reference here. ("don't have to" is enough to say about
it, but there should be something).


> -	if (refcount_dec_not_one(&nf->nf_ref))
> -		return true;
> -
> -	nfsd_file_lru_remove(nf);
> -	list_add(&nf->nf_lru, dispose);
> -	return true;
> +	if (nfsd_file_unhash(nf)) {
> +		nfsd_file_lru_remove(nf);
> +		list_add(&nf->nf_lru, dispose);
> +	}
> }
> 
> static void
> @@ -564,8 +559,6 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose)
>  * @lock: LRU list lock (unused)
>  * @arg: dispose list
>  *
> - * Note this can deadlock with nfsd_file_cache_purge.
> - *
>  * Return values:
>  *   %LRU_REMOVED: @item was removed from the LRU
>  *   %LRU_ROTATE: @item is to be moved to the LRU tail
> @@ -750,8 +743,6 @@ nfsd_file_close_inode(struct inode *inode)
>  *
>  * Walk the LRU list and close any entries that have not been used since
>  * the last scan.
> - *
> - * Note this can deadlock with nfsd_file_cache_purge.
>  */
> static void
> nfsd_file_delayed_close(struct work_struct *work)
> @@ -893,16 +884,12 @@ nfsd_file_cache_init(void)
> 	goto out;
> }
> 
> -/*
> - * Note this can deadlock with nfsd_file_lru_cb.
> - */
> static void
> __nfsd_file_cache_purge(struct net *net)
> {
> 	struct rhashtable_iter iter;
> 	struct nfsd_file *nf;
> 	LIST_HEAD(dispose);
> -	bool del;
> 
> 	rhashtable_walk_enter(&nfsd_file_rhash_tbl, &iter);
> 	do {
> @@ -912,14 +899,7 @@ __nfsd_file_cache_purge(struct net *net)
> 		while (!IS_ERR_OR_NULL(nf)) {
> 			if (net && nf->nf_net != net)
> 				continue;
> -			del = nfsd_file_unhash_and_dispose(nf, &dispose);
> -
> -			/*
> -			 * Deadlock detected! Something marked this entry as
> -			 * unhased, but hasn't removed it from the hash list.
> -			 */
> -			WARN_ON_ONCE(!del);
> -
> +			nfsd_file_unhash_and_dispose(nf, &dispose);
> 			nf = rhashtable_walk_next(&iter);
> 		}
> 
> -- 
> 2.37.3
> 

--
Chuck Lever