Re: [PATCH v2] nfsd: drop st_mutex and rp_mutex before calling move_to_close_lru()

[Jeff Layton <jlayton@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3683 bytes --]

On Wed, 2024-01-17 at 14:48 +1100, NeilBrown wrote:
> move_to_close_lru() is currently called with ->st_mutex and .rp_mutex held.
> This can lead to a deadlock as move_to_close_lru() waits for sc_count to
> drop to 2, and some threads holding a reference might be waiting for either
> mutex.  These references will never be dropped so sc_count will never
> reach 2.
> 
> There can be no harm in dropping ->st_mutex to before
> move_to_close_lru() because the only place that takes the mutex is
> nfsd4_lock_ol_stateid(), and it quickly aborts if sc_type is
> NFS4_CLOSED_STID, which it will be before move_to_close_lru() is called.
> 
> Similarly dropping .rp_mutex is safe after the state is closed and so
> no longer usable.  Another way to look at this is that nothing
> significant happens between when nfsd4_close() now calls
> nfsd4_cstate_clear_replay(), and where nfsd4_proc_compound calls
> nfsd4_cstate_clear_replay() a little later.
> 
> See also
>  https://lore.kernel.org/lkml/4dd1fe21e11344e5969bb112e954affb@jd.com/T/
> where this problem was raised but not successfully resolved.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 40415929e2ae..0850191f9920 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7055,7 +7055,7 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
>  	return status;
>  }
>  
> -static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> +static bool nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>  {
>  	struct nfs4_client *clp = s->st_stid.sc_client;
>  	bool unhashed;
> @@ -7072,11 +7072,11 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>  		list_for_each_entry(stp, &reaplist, st_locks)
>  			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
>  		free_ol_stateid_reaplist(&reaplist);
> +		return false;
>  	} else {
>  		spin_unlock(&clp->cl_lock);
>  		free_ol_stateid_reaplist(&reaplist);
> -		if (unhashed)
> -			move_to_close_lru(s, clp->net);
> +		return unhashed;
>  	}
>  }
>  
> @@ -7092,6 +7092,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	struct nfs4_ol_stateid *stp;
>  	struct net *net = SVC_NET(rqstp);
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +	bool need_move_to_close_list;
>  
>  	dprintk("NFSD: nfsd4_close on file %pd\n", 
>  			cstate->current_fh.fh_dentry);
> @@ -7114,8 +7115,17 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	 */
>  	nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
>  
> -	nfsd4_close_open_stateid(stp);
> +	need_move_to_close_list = nfsd4_close_open_stateid(stp);
>  	mutex_unlock(&stp->st_mutex);
> +	if (need_move_to_close_list) {
> +		/* Drop the replay mutex early as move_to_close_lru()
> +		 * can wait for other threads which hold that mutex.
> +		 * This call is idempotent, so that fact that it will
> +		 * be called twice is harmless.
> +		 */
> +		nfsd4_cstate_clear_replay(cstate);
> +		move_to_close_lru(stp, net);
> +	}
>  
>  	/* v4.1+ suggests that we send a special stateid in here, since the
>  	 * clients should just ignore this anyway. Since this is not useful

There is a recent regression in pynfs test CLOSE12 in Chuck's nfsd-next
branch. In the attached capture, there is an extra 40 bytes on the end
of the CLOSE response in frame 112.

A bisect landed on this patch, though I don't see the cause just yet.

Thoughts?
-- 
Jeff Layton <jlayton@kernel.org>

[-- Attachment #2: nfs.pcap.xz --]
[-- Type: application/x-xz, Size: 3316 bytes --]