Received-SPF: pass (google.com: domain of linux-nfs+bounces-2168-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 4 Mar 2024 09:09:28 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: NeilBrown <neilb@suse.de>
Cc: Jeff Layton <jlayton@kernel.org>, linux-nfs@vger.kernel.org,
        Olga Kornievskaia <kolga@netapp.com>, Dai Ngo <Dai.Ngo@oracle.com>,
        Tom Talpey <tom@talpey.com>
Subject: Re: [PATCH 3/4] nfsd: replace rp_mutex to avoid deadlock in
 move_to_close_lru()
Message-ID: <ZeXWGJqreYH8aayB@klimt.1015granger.net>
References: <20240304044304.3657-1-neilb@suse.de>
 <20240304044304.3657-4-neilb@suse.de>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240304044304.3657-4-neilb@suse.de>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?Sf59J5RkwN1Vh8VWJGt8ZL3ibN+zXwHt/Rhm0CVF5JFR7SJ2J8KwTfqN7GFc?=
 =?us-ascii?Q?DZry96YRBk4coY6taFbccgEatyBqMRR+HdFkB3bZDDGvKK+ImSHrgJ8gnvJm?=
 =?us-ascii?Q?kSugabXLfbnZEyUFVM3Kw+0pUjPLu2r2mszM1Lp3KONx1DcXXmaj0G2oouLb?=
 =?us-ascii?Q?lxkOjz0Uj43Z/7LUznT4Pm0Q3nus2UGAh+Ir6P1qBRNOwywTbRwF8aI4c/hp?=
 =?us-ascii?Q?ii6uM3jOvsd0JjesEdh05uSG7dIllREoINcjZzKM1aX/Gj4M5MQhBod58ATI?=
 =?us-ascii?Q?azw1L2Fqlpip8syTN9n+wyP+zsybRJSlVRuKvf3V5be9NDbeP4j6Q261t+95?=
 =?us-ascii?Q?w2Ra3JkEFU8BuCMooJVN7CWHOZLVJpUTV4rH/cfnpCFu2iKSh3Vab0QnxBq1?=
 =?us-ascii?Q?qxpnuobKMV3mQESt2mj1+J0at6cvMal8MPWf5Jc9LE+BX+IHyvanSjaPSXhq?=
 =?us-ascii?Q?Fe5s6h+R4BcJaapcenuoLaiewwYkdqs3qG4J0EFZu36/mwbAQLbMj5J1Gndg?=
 =?us-ascii?Q?z5qXEQFvRISBVhUuHtyehMZnvO9rWOu/ivhzXrvIKDKxBYwboHOdg0IwtXLM?=
 =?us-ascii?Q?4vMKZ5+6iq+ZB9VBCoymyXR8MBEukegW+ZLfl57sUHSyrImJcbpy/eCtSg8g?=
 =?us-ascii?Q?QWB2l7do7/mRfY5bgAdORuk6xyLG3pXRuO9ndP9CQBhua/Ftm48SrQY4DL8B?=
 =?us-ascii?Q?C03BD8qmU+gnuz33Gwl+oRZbhy3wffaaxSxy4kqJTJsag/qiMY+tdGTvI0TV?=
 =?us-ascii?Q?ZjnLT8kd62A12Xx55+CnzjT7RnGKWww4ptensyc7bxylePIxu3CK+ZseLWlO?=
 =?us-ascii?Q?v2clNlHK3Xi89OFTVWECUdusTtnHDV/mzeQj9blEPHCSoP14PkA4wtmXhwi0?=
 =?us-ascii?Q?WSmtubZJrcwCsbDjx2b1thv5Q2VX8m2BRV6gKARmc1x8+DzFAj/FUVz9ZZ+z?=
 =?us-ascii?Q?7J6idPwONL7QijUYyybh236jWl+WdFoOsEfhzvDUgZZMyGsqOzpmig0KGJCb?=
 =?us-ascii?Q?bglP5/N7Ex7AVQJD6ALv3efPBH35bMT0DfUs6VqD68ayEbhhrEIz0swxbJhf?=
 =?us-ascii?Q?s196BdVwES97CF1a3C2YxWXcz6nsGx43/ehnVEKqA0qUFyG617XMo51fPHJB?=
 =?us-ascii?Q?RXjxOPorv21XxVEiylblIB7ZvdUQB7bFsXipsX7YlQjepCu5KQPJugf1USQ1?=
 =?us-ascii?Q?urmcbXRg7OOYvA+WCNUHYf3UUptdUr8SSVDtKzMzg7w2/IK0g3H9co6zJQgp?=
 =?us-ascii?Q?TKfoRAtC3J8qS4Z8o3e7SVv/Es62hDAeorpxNhhXtjyd/Bqa6UZ5xF3r3+5l?=
 =?us-ascii?Q?zUE62zW9PIwTvU+loXXhRglSX9O9vdA9QIw8KRhD4R+fC3x5saavrrmHmBRp?=
 =?us-ascii?Q?JCJZ2AQaagRcCxkaxkMMC6+nUd4UvErnD1fdqScz2WfTb1Cnp5L2rzUzN+Wz?=
 =?us-ascii?Q?NAQwJPIgyTGJTR325PhKlIcOApG3QWJDWEiFJgnuFPxsh8LUcDLqmAfk5YO2?=
 =?us-ascii?Q?PNuz2rWuIDubd45BkdsUvx7TdfmDaw3MrHEAO/cJCVCKLbR1PgilfamQheb8?=
 =?us-ascii?Q?7RGQR45ps7ohHftKTWFVF4uWyltk9YRc8pV5tsuppUoIA8z46puF1tXLAth6?=
 =?us-ascii?Q?/A=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	Tm4AaVEDwlF1iiQ9vvY3qb9rofTIvCNan3e9aiyX57HJAaHYefrAhGFUZEsxMIdoXvtF/KEsis/Bu9afdXEhuMRyO6Dib1hve//KYYYdvF5BFtAIo6lxgNVOfK/B9BV4vcn/4cQsyGluO8toNitAkUmNX0Ka4ZehrbQJ2b4+CwC0aldJSJejiwytPYlNWtJ3wzGEkS89v1svXxLas5alFTUn6eprtgyhJKaW1LJzfQa2zmCnMLRdpgaFFcBjOcJOweGPiRPKwCnR7LBxXlqNVlffvw7v30WqhjrDexI/bH+EtSXoEGyzO/BT7BXCffXQSZjAn1yxDbYaqIKuaw2aJGgr7Hn+Hj7+bU09qpe4jz0WUrKvkBfECvwriKf/hFzU6Xcct6YCLFHCSWdGMBj0nADhgvo9q/9/1MeqrCDOk2l//FzGNf1kVhwH/j4G6KazWDqQrV7cHQ7TovClU5UWrUKLLcw+1GjyDwgTMoCcqrK7w4fA7XIOZWi97ntE9ghMeLIed3YNMRWQWst2bzv7OwBM1TPo2HpNcMJ+A9Kw2qzJj1SKN6g7wDU+L/id7lUzefxxpPyFJ4XVlDLijLLamySPG2sBbufjd9XI/YmGMUQ=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 71aaac4e-4655-45ca-cbba-08dc3c54b5fd
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Mar 2024 14:09:30.9023
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2o1/Ftm3CTM2Jr+R+fsxHYMYOq+/HGPFi8sBPE6uAi0MHGZ7+dQDg/wvrooZh9S2NaLkv0vBlr+otGJpXT7J8w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4680
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-03-04_09,2024-03-04_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxlogscore=840
 adultscore=0 phishscore=0 spamscore=0 mlxscore=0 bulkscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2403040107
X-Proofpoint-ORIG-GUID: 04qudiuc1pNhZqh07Q5q1xxqAoWcsQhj
X-Proofpoint-GUID: 04qudiuc1pNhZqh07Q5q1xxqAoWcsQhj

On Mon, Mar 04, 2024 at 03:40:21PM +1100, NeilBrown wrote:
> move_to_close_lru() waits for sc_count to become zero while holding
> rp_mutex.  This can deadlock if another thread holds a reference and is
> waiting for rp_mutex.
> 
> By the time we get to move_to_close_lru() the openowner is unhashed and
> cannot be found any more.  So code waiting for the mutex can safely
> retry the lookup if move_to_close_lru() has started.
> 
> So change rp_mutex to an atomic_t with three states:
> 
>  RP_UNLOCK   - state is still hashed, not locked for reply
>  RP_LOCKED   - state is still hashed, is locked for reply
>  RP_UNHASHED - state is not hashed, no code can get a lock.
> 
> Use wait_var_event() to wait for either a lock, or for the owner to be
> unhashed.  In the latter case, retry the lookup.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 38 +++++++++++++++++++++++++++++++-------
>  fs/nfsd/state.h     |  2 +-
>  2 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 690d0e697320..47e879d5d68a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4430,21 +4430,32 @@ nfsd4_init_leases_net(struct nfsd_net *nn)
>  	atomic_set(&nn->nfsd_courtesy_clients, 0);
>  }
>  
> +enum rp_lock {
> +	RP_UNLOCKED,
> +	RP_LOCKED,
> +	RP_UNHASHED,
> +};
> +
>  static void init_nfs4_replay(struct nfs4_replay *rp)
>  {
>  	rp->rp_status = nfserr_serverfault;
>  	rp->rp_buflen = 0;
>  	rp->rp_buf = rp->rp_ibuf;
> -	mutex_init(&rp->rp_mutex);
> +	atomic_set(&rp->rp_locked, RP_UNLOCKED);
>  }
>  
> -static void nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
> -		struct nfs4_stateowner *so)
> +static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
> +				      struct nfs4_stateowner *so)
>  {
>  	if (!nfsd4_has_session(cstate)) {
> -		mutex_lock(&so->so_replay.rp_mutex);
> +		wait_var_event(&so->so_replay.rp_locked,
> +			       atomic_cmpxchg(&so->so_replay.rp_locked,
> +					      RP_UNLOCKED, RP_LOCKED) != RP_LOCKED);

What reliably prevents this from being a "wait forever" ?


> +		if (atomic_read(&so->so_replay.rp_locked) == RP_UNHASHED)
> +			return -EAGAIN;
>  		cstate->replay_owner = nfs4_get_stateowner(so);
>  	}
> +	return 0;
>  }
>  
>  void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate)
> @@ -4453,7 +4464,8 @@ void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate)
>  
>  	if (so != NULL) {
>  		cstate->replay_owner = NULL;
> -		mutex_unlock(&so->so_replay.rp_mutex);
> +		atomic_set(&so->so_replay.rp_locked, RP_UNLOCKED);
> +		wake_up_var(&so->so_replay.rp_locked);
>  		nfs4_put_stateowner(so);
>  	}
>  }
> @@ -4691,7 +4703,11 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net *net)
>  	 * Wait for the refcount to drop to 2. Since it has been unhashed,
>  	 * there should be no danger of the refcount going back up again at
>  	 * this point.
> +	 * Some threads with a reference might be waiting for rp_locked,
> +	 * so tell them to stop waiting.
>  	 */
> +	atomic_set(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED);
> +	wake_up_var(&oo->oo_owner.so_replay.rp_locked);
>  	wait_event(close_wq, refcount_read(&s->st_stid.sc_count) == 2);
>  
>  	release_all_access(s);
> @@ -5064,11 +5080,15 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
>  	clp = cstate->clp;
>  
>  	strhashval = ownerstr_hashval(&open->op_owner);
> +retry:
>  	oo = find_or_alloc_open_stateowner(strhashval, open, cstate);
>  	open->op_openowner = oo;
>  	if (!oo)
>  		return nfserr_jukebox;
> -	nfsd4_cstate_assign_replay(cstate, &oo->oo_owner);
> +	if (nfsd4_cstate_assign_replay(cstate, &oo->oo_owner) == -EAGAIN) {
> +		nfs4_put_stateowner(&oo->oo_owner);
> +		goto retry;
> +	}
>  	status = nfsd4_check_seqid(cstate, &oo->oo_owner, open->op_seqid);
>  	if (status)
>  		return status;
> @@ -6828,11 +6848,15 @@ nfs4_preprocess_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
>  	trace_nfsd_preprocess(seqid, stateid);
>  
>  	*stpp = NULL;
> +retry:
>  	status = nfsd4_lookup_stateid(cstate, stateid, typemask, &s, nn);
>  	if (status)
>  		return status;
>  	stp = openlockstateid(s);
> -	nfsd4_cstate_assign_replay(cstate, stp->st_stateowner);
> +	if (nfsd4_cstate_assign_replay(cstate, stp->st_stateowner) == -EAGAIN) {
> +		nfs4_put_stateowner(stp->st_stateowner);
> +		goto retry;
> +	}
>  
>  	status = nfs4_seqid_op_checks(cstate, stateid, seqid, stp);
>  	if (!status)

My tool chain reports that this hunk won't apply to nfsd-next.

In my copy of fs/nfsd/nfs4state.c, nfs4_preprocess_seqid_op() starts
at line 7180, so something is whack-a-doodle.


> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 41bdc913fa71..6a3becd86112 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -446,7 +446,7 @@ struct nfs4_replay {
>  	unsigned int		rp_buflen;
>  	char			*rp_buf;
>  	struct knfsd_fh		rp_openfh;
> -	struct mutex		rp_mutex;
> +	atomic_t		rp_locked;
>  	char			rp_ibuf[NFSD4_REPLAY_ISIZE];
>  };
>  
> -- 
> 2.43.0
> 

-- 
Chuck Lever