Received-SPF: pass (google.com: domain of linux-nfs+bounces-2606-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: Tue, 2 Apr 2024 17:03:31 -0400
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 1/4  v3] nfsd: perform all find_openstateowner_str calls
 in the one place.
Message-ID: <Zgxyo8SH/DQYgF1j@tissot.1015granger.net>
References: <171010907730.13576.5751778995820664441@noble.neil.brown.name>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <171010907730.13576.5751778995820664441@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?JUB3U0Nt9/5QmXdG6x3lfGKKoH9bzFiFt5Z0OgESeeJmXzD7f/I7nvcTK/17?=
 =?us-ascii?Q?dwyr9F+J6JQ0cRH/3yWSEFBzeLKDW4GNj+ImPU9PetFC3KwaznDsW95xa+mU?=
 =?us-ascii?Q?H2Oj4OgRZrexXp4nWMGf3cOo0GolJgUgaLQAqyw4kOFBJTK1idmwr0AbibEX?=
 =?us-ascii?Q?lcXjLpvv1LoQfZqsVl3VLokQ6Cd7sdMB2lSFGlftg+nMpGKlVZOacHf8qxUO?=
 =?us-ascii?Q?OcyyCVOCkGboGJXWs4EczgwU04mv3Ks/lXdgHJOR4UoNqyYFEbeb85LBIzbq?=
 =?us-ascii?Q?uWm9PnwRO/AVaUNZ2KXBj2KHHcKswUQfXTMO1ldbphhkEP/v01z4Q9Pher2G?=
 =?us-ascii?Q?qt1sPM5BJv8Vu4C0BchQ2+3FaWqwyagdWEDOiKnDWALiZqgKCGv7FxaocSBs?=
 =?us-ascii?Q?9d2lHh0o43pbB4UU5QilJ1Wq3DVIND93WRwEJC7oGqb/hCtWOYKLZjUb4T2L?=
 =?us-ascii?Q?4iLB2vyOFkYOu82rdjb8tZQeeFWnv1/x8eebmdDBQijF6qlhP8G97lQO8UBo?=
 =?us-ascii?Q?5dzVYMT+bUL2Nx98rKI6HEwqlIkJM8yEC+uzaZkdoiT91h8qPjhZCUhJM/YJ?=
 =?us-ascii?Q?iLGqlBpCOfumJgQ8neoCYwiMjR/ExNZNBXpvs7JcIQV4RhOjhIFDm8bXLJJB?=
 =?us-ascii?Q?WHqCEdSr0NIxwlAKeeiRlPJq9RdmXIZAJrEu6dgla0fIdLhQVYH29ceWMqtL?=
 =?us-ascii?Q?VQ+wTRvB5QmUVZB1ALpmKc5a/eNFlorEMkE+unDVf6xkNMOutHE3ypc7jXfw?=
 =?us-ascii?Q?CUQepFQF1/79Fep39qh+MJYRr8sGR5HPz3hFii6ArDUoJ+lHhck+KWyAMG8f?=
 =?us-ascii?Q?LaMxcgZLRU38/f0t7XbiJptDj1JVU/Yd+XWBW2cWB+6+vWB3Q0Lia5/pSPLy?=
 =?us-ascii?Q?VfUUI5nmr1BJSEGAP7fIDPhNlFDkMfPVeiV5fRx98sMro18FJm7lHlYOlHiy?=
 =?us-ascii?Q?NxRZnhKF1estRK7nzRTlOTubuKEhBERTBLiEBbhCgvzNZYk4GGJNze+7m/9o?=
 =?us-ascii?Q?bkjCQ17RCd5D8aAtapi4FmupoaRAzTGe54l9C1X16/P8Miwpf8twzBSwznXE?=
 =?us-ascii?Q?BnntWmw7WBXZS7HCW5oObKTdwfqkvty88q7aTkVNnWMBxViaOaGpU7waevjQ?=
 =?us-ascii?Q?0qock4zfFRqPJlt/bkMfGml3FjtTRTzd0qE6tPnrdSJHYAt6Kmi/3t+50GFf?=
 =?us-ascii?Q?bA4xBynfgwgMUJSXf3tg918ubizDl863uQWvcWUMoF6MZZWQHpq7S3qcLoeN?=
 =?us-ascii?Q?5uoxLMnzzrx7tEQl9kxnhdOOwXODfEAs2qxJgzJnHyeL1Nnp51sFycflll87?=
 =?us-ascii?Q?rQSO7Np714rTsqTCi2mwp6yEBuiNQK+iH48soOaiVi7AGMYP7iSevjcG+MVm?=
 =?us-ascii?Q?gyRCEQYfrmFjnbpqq3LwNVzL70aOWsKKZDU6jqWwYbbz28z1IJXFs+QxDaAS?=
 =?us-ascii?Q?RGry7toqo92tJquuljA+W/8HP8aXm030GsQ4sSnTcyvJeEqKaFA+mPMPH/rz?=
 =?us-ascii?Q?NXKgSXGkrU0Z3yPq0flyBhO7ghUVwq49C+Rnn4435LtNcaAAcJ9lukm+uck2?=
 =?us-ascii?Q?xbXeI23qQcKSlHpOjfdt6mo1iT0Ua7FVESTKDoYPE25/Hcl78xPsbtvKZ0ay?=
 =?us-ascii?Q?jw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	Hl6xDXw4lGBSovuPkqhcnnGuHpz73mrYTY0eAEYEcHsec9ziFQbS3wV4FgLRFV8BOwNDxfO+YoMkHu5pP+cixH41+ZRV/Q0W8MH3wsjXG3KeZW/uR2QxpvTsuVhrdcmvyJmPcedbMqHhS0PBkGGRRbpr2RMrxHR22nDLbF4K8YJBX2vXWTbw9cfDYjgcUpuqSPqu2Wb1p0xVE8+5mYcATuyvAjmJvn4I4sO1MnMnuewhgTCqr7Ky17ufsgZ4u5OO9wPLpJQQaVY1SktznGqKuhrzdNroe/H7w173oBWpWoC3HkZmZYFOeBFQ5BgnJOUkmsFjZqJMK+gl6ba13Ti7IOZeGiWFtDdRqRYDDb1LHwt7IWiKawJgB3LzGO1v2fPAy7ZcYX1n76M6ciVJK/DhK2ndGKSWfUmUoGFHJ5V7ZIJvbm3NlA5Dy0CRi92huIhQpJZuf34T1hVjUjY1iwQwxoDOOvewhZ/ECxTlFC9N1SQ1Q9Q+GkWYufwNyOHAI5s2nXly1IDdEHpkPAFTG2rtWeR3c6QWJIeXPfs4BcACX7RhUqk4OZI/k98k75P2NUJAdkfLnbVmRfnrc8smaXYSLr68ajVkv7D6yMcBuoitEUE=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 45f84102-44f2-4ca8-d447-08dc53585c01
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Apr 2024 21:03:34.7287
 (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: cqqk/sP0cns4jPJ25ykPMPADO8yxanM+9HnAkW8sO3CzZLMvpXiOvrRRoHYPjrw7jkvjw6r/WCG05z685TimFQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB7252
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-04-02_14,2024-04-01_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 suspectscore=0
 phishscore=0 bulkscore=0 spamscore=0 adultscore=0 mlxlogscore=830
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2403210000 definitions=main-2404020156
X-Proofpoint-GUID: MYcNftqlXM0CK3Pb4JQpoxuSADljgvmP
X-Proofpoint-ORIG-GUID: MYcNftqlXM0CK3Pb4JQpoxuSADljgvmP

On Mon, Mar 11, 2024 at 09:17:57AM +1100, NeilBrown wrote:
> 
> Currently find_openstateowner_str lookups are done both in
> nfsd4_process_open1() and alloc_init_open_stateowner() - the latter
> possibly being a surprise based on its name.
> 
> It would be easier to follow, and more conformant to common patterns, if
> the lookup was all in the one place.
> 
> So replace alloc_init_open_stateowner() with
> find_or_alloc_open_stateowner() and use the latter in
> nfsd4_process_open1() without any calls to find_openstateowner_str().
> 
> This means all finds are find_openstateowner_str_locked() and
> find_openstateowner_str() is no longer needed.  So discard
> find_openstateowner_str() and rename find_openstateowner_str_locked() to
> find_openstateowner_str().
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> 
> The previous version of this patch could deadlock on ->cl_lcok as it
> called release_openowner() while holding that lock.
> This version doesn't.
> 
> Subsequent patch in the original series should be unchanged.  If needed
> I can resent them all after -rc1 is out.

I have, from March 5, 2024:

1/4: nfsd: move nfsd4_cstate_assign_replay() earlier in open handling.
2/4: nfsd: perform all find_openstateowner_str calls in the one place.
3/4: nfsd: replace rp_mutex to avoid deadlock in move_to_close_lru()
4/4: nfsd: drop st_mutex_mutex before calling move_to_close_lru()

Note that "nfsd: perform all find_open..." is 2/4, not 1/4. It
applies cleanly to v6.9-rc2, but after it has been applied, the
others do not apply cleanly. If I apply "nfsd: move nfsd4_cstate..."
first, then "nfsd: perform all find_open..." does not apply cleanly.

Please rebase this series on v6.9-rc2, test it, and re-post the
full series as "v4".  Thanks!


>  fs/nfsd/nfs4state.c | 93 +++++++++++++++++++--------------------------
>  1 file changed, 40 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ee9aa4843443..3e144b1a1386 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -541,7 +541,7 @@ same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner)
>  }
>  
>  static struct nfs4_openowner *
> -find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
> +find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open,
>  			struct nfs4_client *clp)
>  {
>  	struct nfs4_stateowner *so;
> @@ -558,18 +558,6 @@ find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
>  	return NULL;
>  }
>  
> -static struct nfs4_openowner *
> -find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open,
> -			struct nfs4_client *clp)
> -{
> -	struct nfs4_openowner *oo;
> -
> -	spin_lock(&clp->cl_lock);
> -	oo = find_openstateowner_str_locked(hashval, open, clp);
> -	spin_unlock(&clp->cl_lock);
> -	return oo;
> -}
> -
>  static inline u32
>  opaque_hashval(const void *ptr, int nbytes)
>  {
> @@ -4855,34 +4843,46 @@ nfsd4_find_and_lock_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
>  }
>  
>  static struct nfs4_openowner *
> -alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
> -			   struct nfsd4_compound_state *cstate)
> +find_or_alloc_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
> +			      struct nfsd4_compound_state *cstate)
>  {
>  	struct nfs4_client *clp = cstate->clp;
> -	struct nfs4_openowner *oo, *ret;
> +	struct nfs4_openowner *oo, *new = NULL;
>  
> -	oo = alloc_stateowner(openowner_slab, &open->op_owner, clp);
> -	if (!oo)
> -		return NULL;
> -	oo->oo_owner.so_ops = &openowner_ops;
> -	oo->oo_owner.so_is_open_owner = 1;
> -	oo->oo_owner.so_seqid = open->op_seqid;
> -	oo->oo_flags = 0;
> -	if (nfsd4_has_session(cstate))
> -		oo->oo_flags |= NFS4_OO_CONFIRMED;
> -	oo->oo_time = 0;
> -	oo->oo_last_closed_stid = NULL;
> -	INIT_LIST_HEAD(&oo->oo_close_lru);
> +retry:
>  	spin_lock(&clp->cl_lock);
> -	ret = find_openstateowner_str_locked(strhashval, open, clp);
> -	if (ret == NULL) {
> -		hash_openowner(oo, clp, strhashval);
> -		ret = oo;
> -	} else
> -		nfs4_free_stateowner(&oo->oo_owner);
> -
> +	oo = find_openstateowner_str(strhashval, open, clp);
> +	if (!oo && new) {
> +		hash_openowner(new, clp, strhashval);
> +		spin_unlock(&clp->cl_lock);
> +		return new;
> +	}
>  	spin_unlock(&clp->cl_lock);
> -	return ret;
> +
> +	if (oo && !(oo->oo_flags & NFS4_OO_CONFIRMED)) {
> +		/* Replace unconfirmed owners without checking for replay. */
> +		release_openowner(oo);
> +		oo = NULL;
> +	}
> +	if (oo) {
> +		if (new)
> +			nfs4_free_stateowner(&new->oo_owner);
> +		return oo;
> +	}
> +
> +	new = alloc_stateowner(openowner_slab, &open->op_owner, clp);
> +	if (!new)
> +		return NULL;
> +	new->oo_owner.so_ops = &openowner_ops;
> +	new->oo_owner.so_is_open_owner = 1;
> +	new->oo_owner.so_seqid = open->op_seqid;
> +	new->oo_flags = 0;
> +	if (nfsd4_has_session(cstate))
> +		new->oo_flags |= NFS4_OO_CONFIRMED;
> +	new->oo_time = 0;
> +	new->oo_last_closed_stid = NULL;
> +	INIT_LIST_HEAD(&new->oo_close_lru);
> +	goto retry;
>  }
>  
>  static struct nfs4_ol_stateid *
> @@ -5331,27 +5331,14 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
>  	clp = cstate->clp;
>  
>  	strhashval = ownerstr_hashval(&open->op_owner);
> -	oo = find_openstateowner_str(strhashval, open, clp);
> +	oo = find_or_alloc_open_stateowner(strhashval, open, cstate);
>  	open->op_openowner = oo;
> -	if (!oo) {
> -		goto new_owner;
> -	}
> -	if (!(oo->oo_flags & NFS4_OO_CONFIRMED)) {
> -		/* Replace unconfirmed owners without checking for replay. */
> -		release_openowner(oo);
> -		open->op_openowner = NULL;
> -		goto new_owner;
> -	}
> +	if (!oo)
> +		return nfserr_jukebox;
>  	status = nfsd4_check_seqid(cstate, &oo->oo_owner, open->op_seqid);
>  	if (status)
>  		return status;
> -	goto alloc_stateid;
> -new_owner:
> -	oo = alloc_init_open_stateowner(strhashval, open, cstate);
> -	if (oo == NULL)
> -		return nfserr_jukebox;
> -	open->op_openowner = oo;
> -alloc_stateid:
> +
>  	open->op_stp = nfs4_alloc_open_stateid(clp);
>  	if (!open->op_stp)
>  		return nfserr_jukebox;
> -- 
> 2.43.0
> 

-- 
Chuck Lever