Received-SPF: pass (google.com: domain of linux-nfs+bounces-78-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 26 Nov 2023 12:41:09 -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 06/11] nfsd: allow admin-revoked state to appear in
 /proc/fs/nfsd/clients/*/states
Message-ID: <ZWODNaH7TLLJFvQB@tissot.1015granger.net>
References: <20231124002925.1816-1-neilb@suse.de>
 <20231124002925.1816-7-neilb@suse.de>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231124002925.1816-7-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?2638QF+OcH467zuvcN4boHNake0kLaW90jVAALoAUHcEzaC1q69znEgCD5Yc?=
 =?us-ascii?Q?96b7U1K9WvbvSaFrJHTWJ3kWXR6ekZif0xNKG67FaWRF2PIWgJpw9hkQKZrV?=
 =?us-ascii?Q?bBtbFdJHQEPIQIWPGzAhaugUUUKwPrFusuOuuNqXvFBkAppSHnk6b5fSw6Rt?=
 =?us-ascii?Q?y9mSulJwj6ph3v5wJZNGuAxWc1rH8aUux277SCPJrZ+dEHMLaseIBaFSWid4?=
 =?us-ascii?Q?V9rO1vwO+SVaaXF9/rhUehSde+v9tl5C7nnecbNDollkI+Np4KxIMk12WF2y?=
 =?us-ascii?Q?aRAU1pyJhWZlRbiq5ZRWaHEZ9J8zXxpLXcIqzGAHqCCeGYQ1NuaIDWBRgRn2?=
 =?us-ascii?Q?cA+1MxWHVbcsYmtHN24Cg1Fnm3SpbI4YH9Y5ZGbAMdc+7ll5c0NM2VXbSRo7?=
 =?us-ascii?Q?WMeeHE8S9iOre611jTtG7RGL7uPcqDt5YDnE6hmv/zfRHrFkG6+3U/dRvDEg?=
 =?us-ascii?Q?kHoUIRebJp15bfMrMvXPBtnavLhyTCeiprsLeksKz4thYuXbZKKOzt5HJgvq?=
 =?us-ascii?Q?WgmleBOVzpy0xORpB5KQdo7OnTsagaBAJYV+J5aF16haE0JYd/z30jGH7n0m?=
 =?us-ascii?Q?BB6XD6gIW0OAQKnr7jKknRqtM53kMrFFNH0R8SEfjJKMHQyY9f0FR3yxSMf7?=
 =?us-ascii?Q?Ma3IE1Ww16X9J4cTXyybwYhOoG3lsHDJSSM5XSk/+o4SI5OytJUcu5w9TwAr?=
 =?us-ascii?Q?j9uJfbuQAiRWT0IwaILaqY7jhlXMr9K06EjGVE82n7yxSiOmtc6mbErp/nFc?=
 =?us-ascii?Q?1v4awMj6Qu7FAzH2OVoqnzwDxLYjuMboNFEkLqfUhfo3V92XKGJHzDShf2PW?=
 =?us-ascii?Q?kv7iuTltcT5mVY6JQCLI5TPh+h7czdcgVoLACco0MOhSQlisfT9Y/5XVjuY+?=
 =?us-ascii?Q?gyX8xqpQ1x7ENC+h9EA098PTF56DkIkSxd4s8exNj8r5SXWfyUXNx/epIiaV?=
 =?us-ascii?Q?8hMTEK1uLGcXTiVv8HI0MKyKaoN9rDPMXmB5I2UVDM1fbYz3JKqJyATari7u?=
 =?us-ascii?Q?NBOosRQG27u8EaMby304QdXmLL7LhcDIc/7plcwpSeyYTVvGlK0ScjpjWudJ?=
 =?us-ascii?Q?Q1WVEYDldpg7ztBh+UELpq/+cszBYxd7tYva7H/Fz2YD19bnLASKL1cUtOrw?=
 =?us-ascii?Q?gEWVyEUTqSRLj6/nMocE+ZTx8hXXcv5ciN3P0FZ7pqy4yWYWQOw9LUKb8P6Q?=
 =?us-ascii?Q?vKhJuwZyrmhh3ryGUvENYVtOqPov4qLliTxslGjK8ebn6/FAUKhjNfzQXNPM?=
 =?us-ascii?Q?L0uRN8krwpR6ruRC1JMO1N6rCB5cYJ5+Qm3bp5idQrPdxnM5pL7woLGZKzMf?=
 =?us-ascii?Q?3/+I+c/QDwQjYeIDkQf6C6BrL9kTUYMwjuY5SOL9njcSVb9Fu5cTD+5OO/5x?=
 =?us-ascii?Q?OGBpnsX5uuTTl0N8hwNJ9pAdvr85AmNGZFp4cAE0hX5JBb8SmzQTqbeZDama?=
 =?us-ascii?Q?AKQM5YMdT6UKxjq9qYk4O5/fGydgyJ5Fj2DO51a/h7bXDqKAJ+e1veljsH86?=
 =?us-ascii?Q?M1U8GbwRFCsO17u2G99rowVhx+Zt99FOkjMQ+nzTrEZuUwcX74+zuO/DGRh1?=
 =?us-ascii?Q?6gNbvgfre3AKzH1GhJiWdEXPBSx0I5ZJ57H7+xGmZlA6M1v7lEONd0w/sFfm?=
 =?us-ascii?Q?bw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	8x1DWUyQXeOFLlg1Vtxs5vAiyrQA0Jd6womXg9pGl00AyktsIKeUOqiovcV0Nz0FoRyOBq7TBQDFGDuT/npH0uV4Fu4qD17uwjbzRz/bZhk+YV0Iyykay2jE04jyiorfkqNcwPSsL4mHwNycEuFpXSSFaQ3R7JkhaGj+++4QZPpOsnoJp2bWDKcegf/kkPKyfvEzuxKhMuxF3IzuD40hv8F99JgS1PK8YIYWgOEkHqweTgJKE6CUeEu21i3yAmLlj84I+PhOWHW3onAzeMfFcHMFwRdhK6B3kK1W7l7x7+3c8C3NNq5PDDfM4/IA5qdHIW/oZMLJydOvrNGmk5l29vUC1u35e5GDyvfo6xDaQ7dSXaRGMQ/GV+HdWFuXAu9/hoe+DEyCv7WqTmKPW6WKpS6FUQIZlBBicYUV9e54VbftMT9WQsL+unSQUeR3V3ew4ugzoXYYTZN+MwfzWQ38QFcNWdL3TMICL6pRP6r/59nIZhLtx2vX41Y4CAOJcLSNkZyvRf6/vNhgYEsHH7h0M0a6b0Cqr3R2Rv1yHt1cVvgxJV2TSyWYkUzkakWOtUgruDpPJQha7rsHew8d838QBuatqrHwf/bTCXhFqpxK1b0BTJc4NC1eGDfxliiKkaZJrnwV3fdA0qutn3nuWEpnhnxEUWSyNzOiqM60GQQ2oiAflFsL8i1sU6/OBIiWa3asNoLhWcISOLWTdTWmYmUpVVJIHP9WTU6zcSyH8qp71TWASGakAM/lhLsuUGouYPl1nsm6kvPamtN0aGEBIr3+mKE16B6YP0rcX6aLDI3gb001WkR0fYZI7696Eid4d/dt0fJXFkaklQ2fWLPSJfyfoRBLEk54Wdh9foKbRG6GlKVJFZ7Q/cyiUauz2IrRjQpX
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 25c5de68-28a1-4b34-e7df-08dbeea6e1b1
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Nov 2023 17:41:12.3202
 (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: Lp2Q5IMe6MRZ3AO1hTkrOSDUDJEpqyJHKXun20TtgLaK10k6eVwBt7yf9DmoPzElI3yoRbbw4hrOhRXj3zXASg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB5106
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-11-26_17,2023-11-22_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=876 spamscore=0
 adultscore=0 suspectscore=0 phishscore=0 bulkscore=0 malwarescore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311060000 definitions=main-2311260131
X-Proofpoint-ORIG-GUID: j4pNujm-cpc11jnStzi5NH3XIEuKR_EX
X-Proofpoint-GUID: j4pNujm-cpc11jnStzi5NH3XIEuKR_EX

On Fri, Nov 24, 2023 at 11:28:41AM +1100, NeilBrown wrote:
> Change the "show" functions to show some content even if a file cannot
> be found.
> This is primarily useful for debugging - to ensure states are being
> removed eventually.
> 
> Also remove a "Kinda dead" comment which is no longer correct as we
> now support write delegations.

Nit: I know it's in the same piece of code, but the "Kinda dead"
clean-up is perhaps not relevant to the purpose of this patch. Maybe
do it as a separate patch?


> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 82 ++++++++++++++++++++++-----------------------
>  1 file changed, 41 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 477a9e9aebbd..52e680235afe 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2680,17 +2680,10 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st)
>  	struct nfs4_stateowner *oo;
>  	unsigned int access, deny;
>  
> -	if (st->sc_type != NFS4_OPEN_STID && st->sc_type != NFS4_LOCK_STID)
> -		return 0; /* XXX: or SEQ_SKIP? */
>  	ols = openlockstateid(st);
>  	oo = ols->st_stateowner;
>  	nf = st->sc_file;
>  
> -	spin_lock(&nf->fi_lock);
> -	file = find_any_file_locked(nf);
> -	if (!file)
> -		goto out;
> -
>  	seq_printf(s, "- ");
>  	nfs4_show_stateid(s, &st->sc_stateid);
>  	seq_printf(s, ": { type: open, ");
> @@ -2705,14 +2698,19 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st)
>  		deny & NFS4_SHARE_ACCESS_READ ? "r" : "-",
>  		deny & NFS4_SHARE_ACCESS_WRITE ? "w" : "-");
>  
> -	nfs4_show_superblock(s, file);
> -	seq_printf(s, ", ");
> -	nfs4_show_fname(s, file);
> -	seq_printf(s, ", ");
> +	spin_lock(&nf->fi_lock);
> +	file = find_any_file_locked(nf);
> +	if (file) {
> +		nfs4_show_superblock(s, file);
> +		seq_puts(s, ", ");
> +		nfs4_show_fname(s, file);
> +		seq_puts(s, ", ");
> +	}
> +	spin_unlock(&nf->fi_lock);
>  	nfs4_show_owner(s, oo);
> +	if (st->sc_status & NFS4_STID_ADMIN_REVOKED)
> +		seq_puts(s, ", admin-revoked");

Wondering if this addition (and the other similar additions below)
would be more appropriately done in the previous patch. These
additions seem to have a different purpose than the purpose stated
in the patch description: "Change the 'show' functions to show some
content even if a file cannot be found."

Just a thought.


>  	seq_printf(s, " }\n");
> -out:
> -	spin_unlock(&nf->fi_lock);
>  	return 0;
>  }
>  
> @@ -2726,30 +2724,31 @@ static int nfs4_show_lock(struct seq_file *s, struct nfs4_stid *st)
>  	ols = openlockstateid(st);
>  	oo = ols->st_stateowner;
>  	nf = st->sc_file;
> -	spin_lock(&nf->fi_lock);
> -	file = find_any_file_locked(nf);
> -	if (!file)
> -		goto out;
>  
>  	seq_printf(s, "- ");
>  	nfs4_show_stateid(s, &st->sc_stateid);
>  	seq_printf(s, ": { type: lock, ");
>  
> -	/*
> -	 * Note: a lock stateid isn't really the same thing as a lock,
> -	 * it's the locking state held by one owner on a file, and there
> -	 * may be multiple (or no) lock ranges associated with it.
> -	 * (Same for the matter is true of open stateids.)
> -	 */
> +	spin_lock(&nf->fi_lock);
> +	file = find_any_file_locked(nf);
> +	if (file) {
> +		/*
> +		 * Note: a lock stateid isn't really the same thing as a lock,
> +		 * it's the locking state held by one owner on a file, and there
> +		 * may be multiple (or no) lock ranges associated with it.
> +		 * (Same for the matter is true of open stateids.)
> +		 */
>  
> -	nfs4_show_superblock(s, file);
> -	/* XXX: open stateid? */
> -	seq_printf(s, ", ");
> -	nfs4_show_fname(s, file);
> -	seq_printf(s, ", ");
> +		nfs4_show_superblock(s, file);
> +		/* XXX: open stateid? */
> +		seq_puts(s, ", ");
> +		nfs4_show_fname(s, file);
> +		seq_puts(s, ", ");
> +	}
>  	nfs4_show_owner(s, oo);
> +	if (st->sc_status & NFS4_STID_ADMIN_REVOKED)
> +		seq_puts(s, ", admin-revoked");
>  	seq_printf(s, " }\n");
> -out:
>  	spin_unlock(&nf->fi_lock);
>  	return 0;
>  }
> @@ -2762,27 +2761,28 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
>  
>  	ds = delegstateid(st);
>  	nf = st->sc_file;
> -	spin_lock(&nf->fi_lock);
> -	file = nf->fi_deleg_file;
> -	if (!file)
> -		goto out;
>  
>  	seq_printf(s, "- ");
>  	nfs4_show_stateid(s, &st->sc_stateid);
>  	seq_printf(s, ": { type: deleg, ");
>  
> -	/* Kinda dead code as long as we only support read delegs: */
> -	seq_printf(s, "access: %s, ",
> -		ds->dl_type == NFS4_OPEN_DELEGATE_READ ? "r" : "w");
> +	seq_printf(s, "access: %s",
> +		   ds->dl_type == NFS4_OPEN_DELEGATE_READ ? "r" : "w");
>  
>  	/* XXX: lease time, whether it's being recalled. */
>  
> -	nfs4_show_superblock(s, file);
> -	seq_printf(s, ", ");
> -	nfs4_show_fname(s, file);
> -	seq_printf(s, " }\n");
> -out:
> +	spin_lock(&nf->fi_lock);
> +	file = nf->fi_deleg_file;
> +	if (file) {
> +		seq_puts(s, ", ");
> +		nfs4_show_superblock(s, file);
> +		seq_puts(s, ", ");
> +		nfs4_show_fname(s, file);
> +	}
>  	spin_unlock(&nf->fi_lock);
> +	if (st->sc_status & NFS4_STID_ADMIN_REVOKED)
> +		seq_puts(s, ", admin-revoked");
> +	seq_puts(s, " }\n");
>  	return 0;
>  }
>  
> -- 
> 2.42.1
> 

-- 
Chuck Lever