Re: [PATCH] nfsd: don't call trace_nfsd_deleg_none() if read delegation is given

From: Chuck Lever <chuck.lever@oracle.com>
To: Hou Tao <houtao1@huawei.com>
Cc: Bruce Fields <bfields@fieldses.org>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] nfsd: don't call trace_nfsd_deleg_none() if read delegation is given
Date: Thu, 27 Aug 2020 10:43:20 -0400	[thread overview]
Message-ID: <6F61F417-95DA-4CD7-A81A-FA8C6299CF40@oracle.com> (raw)
In-Reply-To: <20200827070237.19942-1-houtao1@huawei.com>

Hello!

> On Aug 27, 2020, at 3:02 AM, Hou Tao <houtao1@huawei.com> wrote:
> 
> Don't call trace_nfsd_deleg_none() if read delegation is given,
> else two exclusive traces will be printed:
> 
>    nfsd_deleg_open: client 5f45b854:e6058001 stateid 00000030:00000001
>    nfsd_deleg_none: client 5f45b854:e6058001 stateid 0000002f:00000001

These are reporting two different state IDs: the first is a delegation
state ID, and the second is an open state ID.

So in the "no delegation" case, we want to see just the open state ID.
In the "delegation" case, we do want to see both.

You could argue (successfully) that the names of the tracepoints are
pretty lousy. Maybe better to rename:

  nfsd_deleg_open -> nfsd_deleg_read
  nfsd_deleg_none -> nfsd_open

What do you think?

> Fix it by calling trace_nfsd_deleg_none() directly in appropriate
> places instead of calling it by checking the value of op_delegate_type.
> 
> Also remove the unnecessary assignment "status = nfs_ok", because
> we can ensure status will be nfs_ok after the call of
> nfs4_inc_and_copy_stateid().
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
> fs/nfsd/nfs4state.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c09a2a4281ec9..2e6376af701ff 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5131,6 +5131,8 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
> 	nfs4_put_stid(&dp->dl_stid);
> 	return;
> out_no_deleg:
> +	trace_nfsd_deleg_none(&stp->st_stid.sc_stateid);
> +
> 	open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE;
> 	if (open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS &&
> 	    open->op_delegate_type != NFS4_OPEN_DELEGATE_NONE) {
> @@ -5232,7 +5234,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> 		if (open->op_deleg_want & NFS4_SHARE_WANT_NO_DELEG) {
> 			open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE_EXT;
> 			open->op_why_no_deleg = WND4_NOT_WANTED;
> -			goto nodeleg;
> +			trace_nfsd_deleg_none(&stp->st_stid.sc_stateid);
> +			goto out;
> 		}
> 	}
> 
> @@ -5241,9 +5244,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> 	* OPEN succeeds even if we fail.
> 	*/
> 	nfs4_open_delegation(current_fh, open, stp);
> -nodeleg:
> -	status = nfs_ok;
> -	trace_nfsd_deleg_none(&stp->st_stid.sc_stateid);
> out:
> 	/* 4.1 client trying to upgrade/downgrade delegation? */
> 	if (open->op_delegate_type == NFS4_OPEN_DELEGATE_NONE && dp &&
> -- 
> 2.25.0.4.g0ad7144999
> 

--
Chuck Lever