Received-SPF: pass (google.com: domain of linux-nfs+bounces-2199-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 4 Mar 2024 18:30:54 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: NeilBrown <neilb@suse.de>, 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: <ZeZZru5hL_77IRP_@manet.1015granger.net>
References: <20240304044304.3657-1-neilb@suse.de>
 <20240304044304.3657-4-neilb@suse.de>
 <ZeXWGJqreYH8aayB@klimt.1015granger.net>
 <170958874536.24797.2684794071853900422@noble.neil.brown.name>
 <ZeZFGdOD3KWkF1Zf@manet.1015granger.net>
 <170959178935.24797.7531672348129457687@noble.neil.brown.name>
 <ZeZRC21DdOkuKroo@manet.1015granger.net>
 <477e896777379d9b060a735a3873e2ea3096f76f.camel@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <477e896777379d9b060a735a3873e2ea3096f76f.camel@kernel.org>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?FHA8oyuzEHzKuhDBj5JboMHekGZeu9XMJpcuKgmUW7VvdHZiHLp1Yzasjxii?=
 =?us-ascii?Q?p9Cox/qE+6qGHmcDSinseVDSfWc0IYEvHsGleJABSdvRNZ0Myn1vcA6txTvn?=
 =?us-ascii?Q?smVqr4+0mqfian1454k/sOJ1D1+zsU6Z05QE7AcIR4DqoDq+VIFN+EqJJ4kn?=
 =?us-ascii?Q?CpUOAvhMkW9LLWwUzYim4aliQCdD+Z40X/gPSuIm0maG1fqdIQzEdR6E0oCM?=
 =?us-ascii?Q?JIFRB5sLCuQzIT8oIWgv9XoATTnCxaL3SuLLfRAhxlXUeSEcNiJGTNFrptML?=
 =?us-ascii?Q?xzqYNA5eGt1LhRHjlYBe6Dlfm3TNoL2J2MVppRdwcHtnGuSz5XMBxSrxmOL1?=
 =?us-ascii?Q?9nE0Zdz/yOjZAJmC0GSO30yDNiB59csB7ok8e2A7j9zXeQQIRV6jzlgxxXcI?=
 =?us-ascii?Q?SqkSZqa9r4Q17UlFRWuilTZV/XprxUMJOVOS/riXZmxeEczrKYl2hQR84pJA?=
 =?us-ascii?Q?5Mwac5ip4bdCDE433eu5c00svxVVxWyYguSHRAwbBt3wk+o4SEwiYzzdT0Wl?=
 =?us-ascii?Q?guu9xwIjLAlo2b3Hfs0owv3JFizhCm3pdjR3WFJtSg2v+Z7msQbtWJL7gkXY?=
 =?us-ascii?Q?DwnCk5WVHmgISy8Yt8hqVH1pO/z9B6sBu+YDf1GKpnjmnKpFCtfGyJXJFV4G?=
 =?us-ascii?Q?jpBQm44ILB++Sl+sI4KdFM9eS2CdPFxoHTzTbQWVsqAxL7Zgm2MSOtlVXNMX?=
 =?us-ascii?Q?25oSXT0eNQ7tLuTsfvyBDt0AhG+FKfcZEXSheyl+/dutf+aM7gzjrZabzdCV?=
 =?us-ascii?Q?pJ20Zf+KZJQkB3ptJ5XTvkd2gBtoiko2f3WUbo+mRkAuKKx8yw4VjIzgA07C?=
 =?us-ascii?Q?IqrcJ8HHJXBWJ3AWgLdZevwvuQLkcNihB4SysCj5uBRinmEYtUXSSpsg8cwk?=
 =?us-ascii?Q?Lo5KEgp0V1sVWiEhAd/5LnBL1+WAJ3Ah358O8+lfe6vVHZ0Fqwye+ZBToBjQ?=
 =?us-ascii?Q?Xlp/pg27YSKt4rQP7s8DcwEl/afZe4xn2ocbDNL1+nn8Q6QJ7aB+c/Lwf7RY?=
 =?us-ascii?Q?gvqUwbKKvkT/ISC1oempdynD7ZAE4/FpN0uCtH7EnRJW27WF/kM3D3EjfdGM?=
 =?us-ascii?Q?PSLRnNwKZKJKGfsCuowcsDTwBseIndMz45diJfR1HHhpDdM8BFOjWteZn0QJ?=
 =?us-ascii?Q?RSFtGjfh6oGAmpVfx/MCkVJ0/RoPMxKkjmQkOS6ZJzPg2t8XjAzHvW5ZfusD?=
 =?us-ascii?Q?PmTTKK2+OEyABKpOm/54Hlg24i5tPSlA+h5IgdrptJK0XbpBAmAS5kWdhorB?=
 =?us-ascii?Q?jYbTtuut2hNelfjLejiSzAY5hZ9abWv8KBhwPot1tFfGGVJKSd1J1HHaZ9xJ?=
 =?us-ascii?Q?FwS1Uz7gQnRY+aSl//+DEMUUIB2vQNIR8+mFDTkeA9+b2GA2ulgse89eg3F+?=
 =?us-ascii?Q?4nt9hlIezWUZsT4x5jLfqbj9Je+MOezu3X7hJsmzAR6METx10u+2PMZABg4Z?=
 =?us-ascii?Q?ud70ZiyhKkHiabGT4RwBWOI3yJ8GCLoubzWNwr3vN/j21EOHDxyP5YzIAPSU?=
 =?us-ascii?Q?AF3J/BkRyJUjY0xl1iyuzLY0huddcXWzlWnGoBaxi0Ian/U9M9dPkHbq2AHZ?=
 =?us-ascii?Q?eMVZILLRFl0FkxULrrMVvixTVDX+pz+dQEy1NwacWQRceRRHDRm0o7onjtAN?=
 =?us-ascii?Q?Vg=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	Nmpd9Jpkub6mto+vH/7x3vzCHoJyv+8dfVVSjwOqsJ6orGaLlIez2saGJmG/qp967O709BgKy7UNlNp8unXD8ZGHh0TSyBwfOHQ8+7vdETWj0f4cxc9CXIPz9LB+Q0dFiSIYwZ1Gfr1ZY2thjB212oia3xD99DUow30sMu4p//ZI75jjuqpc4j7A3FWId9Cjg3M/mHKSaIAMUNrnWZxsfWHrvftIsY0mD+Cnu1+4WhtFNnMT51avXKMTp1HuDfIYgZndoH88+WT0N/YvTKCMu+Fd3RvdkhAahwLD4fOZTo2Iz/iuU0vti7l3gar3Xgy9zNYdW8/V6C5s76BQGElM2av0rBof+nzwqbzEjyXxyQ/dvcCksmf0cLdqRRzD9UdCR2ZJZMv5V+CO1sz6TXJMGcbdmLUlvF8pmbo+ETYgXMsXI83ZxxJK+cyEfoDc332Gfd30/Z1nRM+sONwKckxXXUELCIFljlKMrxUvPMkmmaTAjn0UP0iF4XFak2K8jUf9U1UBzuxvq2Ovsu1LM+0zen1nx/ed/tE1o/ArRnjqFSWlnKy1cE7H9S5PsHyZViyTQMqMQF/6ISoXl2/pje7OtFiR8PJ36FgmRWA97Ga1dhE=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e6eef044-e3d4-438b-c764-08dc3ca324d7
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Mar 2024 23:30:57.6433
 (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: ltm9sJifQ5Cs2//bwj+4s2fr2yy/cF26SIEms1juWHihSglBVGtaK5y7+GRravpagsClSPTIcQoQydTCjMLYXQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB6218
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_18,2024-03-04_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 bulkscore=0 mlxscore=0
 spamscore=0 phishscore=0 suspectscore=0 malwarescore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2403040181
X-Proofpoint-ORIG-GUID: Y2r5fXhI3D_tf74ZDYDTAdIdjvjDMceU
X-Proofpoint-GUID: Y2r5fXhI3D_tf74ZDYDTAdIdjvjDMceU

On Mon, Mar 04, 2024 at 06:11:24PM -0500, Jeff Layton wrote:
> On Mon, 2024-03-04 at 17:54 -0500, Chuck Lever wrote:
> > On Tue, Mar 05, 2024 at 09:36:29AM +1100, NeilBrown wrote:
> > > On Tue, 05 Mar 2024, Chuck Lever wrote:
> > > > On Tue, Mar 05, 2024 at 08:45:45AM +1100, NeilBrown wrote:
> > > > > On Tue, 05 Mar 2024, Chuck Lever wrote:
> > > > > > 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" ?
> > > > > 
> > > > > That same thing that reliably prevented the original mutex_lock from
> > > > > waiting forever.

Note that this patch fixes a deadlock here. So clearly, there /were/
situations where "waiting forever" was possible with the mutex version
of this code.


> > > > > It waits until rp_locked is set to RP_UNLOCKED, which is precisely when
> > > > > we previously called mutex_unlock.  But it *also* aborts the wait if
> > > > > rp_locked is set to RP_UNHASHED - so it is now more reliable.
> > > > > 
> > > > > Does that answer the question?
> > > > 
> > > > Hm. I guess then we are no worse off with wait_var_event().
> > > > 
> > > > I'm not as familiar with this logic as perhaps I should be. How long
> > > > does it take for the wake-up to occur, typically?
> > > 
> > > wait_var_event() is paired with wake_up_var().
> > > The wake up happens when wake_up_var() is called, which in this code is
> > > always immediately after atomic_set() updates the variable.
> > 
> > I'm trying to ascertain the actual wall-clock time that the nfsd thread
> > is sleeping, at most. Is this going to be a possible DoS vector? Can
> > it impact the ability for the server to shut down without hanging?
> 
> Prior to this patch, there was a mutex in play here and we just released
> it to wake up the waiters. This is more or less doing the same thing, it
> just indicates the resulting state better.

Well, it adds a third state so that a recovery action can be taken
on wake-up in some cases. That avoids a deadlock, so this does count
as a bug fix.


> I doubt this will materially change how long the tasks are waiting.

It might not be a longer wait, but it still seems difficult to prove
that the wait_var_event() will /always/ be awoken somehow.

Applying for now.


-- 
Chuck Lever