Received-SPF: pass (google.com: domain of linux-nfs+bounces-2187-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 17:03:05 -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: <ZeZFGdOD3KWkF1Zf@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>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <170958874536.24797.2684794071853900422@noble.neil.brown.name>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?hESgDy4cNCP69vDWxf491V28uEhveEZQTTyTp0++nJd/GMHXQ8fOsOVwJzRK?=
 =?us-ascii?Q?nTngQT/xeiaWOOGHCnoRZZY4XQR9/bY8vyA6glNtzjSecr8ydu+uh89zJD+l?=
 =?us-ascii?Q?Q7P3mdsiqI2cAzdw6Ym/GDzk84IFaObbgPveGL6olfhv1qlsL3szIQlPy0xW?=
 =?us-ascii?Q?XFP/wiw1GcHNkJqAjIRuqNgz2RNQXxamB6O/yz4ihI8Qt8625SnVwLHdtQMf?=
 =?us-ascii?Q?HzwERhv++yAf/rhrirpr1hOZIwC5ydULN7p1jmgrQL57qRqWshrvetKTe6Ub?=
 =?us-ascii?Q?91ECZFTine8lizwhN16gHhRWL9+GZIByd83aA7//q+zI/KK70d0VuuNZlkTN?=
 =?us-ascii?Q?z78ZN7E8gjWIOxwsPeAn5ma4h/9RNZX/Qo3LPaiDk1Y7eX1ULHKsFC3FeDDO?=
 =?us-ascii?Q?2W3exUW3fE8h/uuWZvo9ZS6oxehlHGbER7x4ZnbjIdJPW18Fp/VjJ3NYVX0f?=
 =?us-ascii?Q?SUq0ddju/yH0jENMBhDnOp/JjCexTUqv5SMi3DnmRcCMx/hdYnI8F8yTXiW7?=
 =?us-ascii?Q?L4LhpuF2WRbjngGJen9Z7J0T/OvXZievgeo/9d2alsa0whIOii0QPd5jLHmA?=
 =?us-ascii?Q?ToV3xm5Gj7Nny7AMY4agBmFcOH6F/1S/iPmpf24iH2Pw34b45dvSXGcNUL6l?=
 =?us-ascii?Q?Enw8Swp/+R9QETP5cn3j7G8lD5BOKuGnGezr9y2lSgpEg+wNcU3DHTC1fCMv?=
 =?us-ascii?Q?PenFzBm7kc59wKYC1eRMu/qcxYDbO8lW6nR+XWhhhuxNjAEgzqq49qx4FZPS?=
 =?us-ascii?Q?rSesSwywhVVIqZ8jrgug21Af7OuXLB9uVZmkfAn0iyXqXJZhoFjl5dUrrNFK?=
 =?us-ascii?Q?zcIvz/lbpVPF3Aglss5YmudizZcD3ss4gcTlGW4tuEPkQY5yWmpcsj7ifukH?=
 =?us-ascii?Q?XIOXbsENG2TJ0bI1H3/aYVgmixG/VbRj+H59hro3VMy/XfsJLUZnIMJC/Q3J?=
 =?us-ascii?Q?JOosHtxES2Xzr7+wEjkb76y9TCbodR28OtkfkOQik/PvDmciNzep06Ec/cO3?=
 =?us-ascii?Q?Nz7i5h04GmbsB+nYRpiNdSlEj5EKFQP0xtq8rm1HZlj/EDllxQ0Qrl0u73AV?=
 =?us-ascii?Q?8zKf5IijkPAzvGGpSzOACAaFQ54aXaUe3djfVabz1efhaAdU7MK2sKIltJBd?=
 =?us-ascii?Q?0TjbgeMIeXfm/7b6gM4KC9NHDrMrHg5g/NRHZZpoFVzT7VE8pvC2d06M7SIk?=
 =?us-ascii?Q?RMLX8IBsh++slJAYwlwdp8SoBUPL4i1yaoWlMAuMqy9AAHejdSi9cyEujWPz?=
 =?us-ascii?Q?4cNE2sym9BCbVJQrmETaIU4FZyvXFsne+VbJnEIxAg0A01AxQo9SOPDdqQOY?=
 =?us-ascii?Q?YOk9kOMEzfSe/yNwhJtDJiiztyPzRNB7kk2TbGwlpGuK3U1Uz8XotDmI/X1D?=
 =?us-ascii?Q?RnbUoN0L4vpCDNikVdW1UC8s81E7AiC0Pf70ep0v9DHvx7FDk9FawZ+tMYRh?=
 =?us-ascii?Q?IAsJr9mJr2CYkbhpgPKDg6B06VEo0G5O95b5OWFZcB5g+BcTLp6AZSbxsz2t?=
 =?us-ascii?Q?Lgo0uWyqrdD7eOeTVITRcGOS5yVw9fc0AmddLva2LYKpvwemmw10xKBBjN/z?=
 =?us-ascii?Q?FH44TICm4TB+6g6W1MyFE3j1CTke5M//TCMzb6lzYvGhk83oBKDUWbO0yf1W?=
 =?us-ascii?Q?VA=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	7elnGcK8RZ9+QY838rC0dgmJBuoDq9ySmw2C5bWxEDKtAqaoLWfvK18j/zt/6IZQCGTElcd4Ag1ne5MAo6ruxX9wVbrcobW94FL0uvAkX7tN0ocePejkwcO51ShYtHHSObfidY/dmGkIV7W/YhcaHvkkUSNYeIITigjVpOgLPUdpekiDFsF7FeqAd6ehHWsubJbrmjRx5edzfCYkpx7/TvKELKYuyARlaGApimIEAYBa7l2yTkCpheLjVLmf/bWkz74Li+tR+Fj1V0RiZDvCp2pRRi9Q6oOzgLR4QELLNP1dkq9zCbmpU8soSswW4JiC6kb25//OSJxHDWH4NxVzh06ZyrC0yuAvUPeTvwC/AWb72nEU67dx9OuJE6S/adyvwa89soq2cZv6AhPzBJJfFnrswaJupoAoBFUcECAoR9lmVgdvrWpM6BGI2ZLunhyFwzi5OREsyIKWIGhPggti0GzBKoPWoKb9+X+rOz9We5w39iXd8oNZuO3RUgc47/IgLSyAi472wq5H+UtqrSkUdZMy1zDze9u1j7syDweLanGGphHu7yTgehD9YbMu+51C+ZZ6bEDDER5HBZUwCsDn1NIRZHMw50KSLoziyZhDrhg=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 76d648c1-36ac-483c-5405-08dc3c96e451
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Mar 2024 22:03:15.3723
 (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: 0vj5yvvtdg46lybzClP3nWZWxrU0RfeqWSH+gJFcqaPwGml6dIoUPM1X0XcRawYqklb7aK7T3Mr9KHnrXGpXpg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB6580
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 phishscore=0 mlxlogscore=999
 suspectscore=0 adultscore=0 bulkscore=0 malwarescore=0 mlxscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2403040172
X-Proofpoint-ORIG-GUID: uFDE79WAdOX1WeVgKBHo0ds_ayp9XDBu
X-Proofpoint-GUID: uFDE79WAdOX1WeVgKBHo0ds_ayp9XDBu

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.
> 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?


> > > +		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.
> 
> You need better tools :-)

<shrug> Essentially it is git, importing an mbox. That kind of thing
is generally the weakest aspect of all these tools.  Do you know of
something more robust?


> > In my copy of fs/nfsd/nfs4state.c, nfs4_preprocess_seqid_op() starts
> > at line 7180, so something is whack-a-doodle.
> 
> I have been working against master because the old version of the fix
> was in nfsd-next.  I should have rebased when you removed it from
> nfsd-next.  I have now and will repost once you confirm you are
> comfortable with the answer about waiting above.  Would adding a comment
> there help?

The mechanism is clear from the code, so a new comment, if you add
one, should spell out what condition(s) mark rp_locked as UNLOCKED.

But I might be missing something that should be obvious, in which
case no comment is necessary.


> Thanks,
> NeilBrown
> 
> 
> > 
> > 
> > > 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
> > 
> 

-- 
Chuck Lever