diff mbox series

nfsd: don't call trace_nfsd_deleg_none() if read delegation is given

Message ID 20200827070237.19942-1-houtao1@huawei.com
State New
Headers show
Series nfsd: don't call trace_nfsd_deleg_none() if read delegation is given | expand

Commit Message

Hou Tao Aug. 27, 2020, 7:02 a.m. UTC
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

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(-)

Comments

Chuck Lever Aug. 27, 2020, 2:43 p.m. UTC | #1
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
Hou Tao Aug. 28, 2020, 2:23 a.m. UTC | #2
Hi,

On 2020/8/27 22:43, Chuck Lever wrote:
> 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.
> 
Thanks for your explanation.

> 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?
> 
After the renaming, the trace looks clearer. I will do it in v2.

Thanks

>> 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
> 
> 
> 
> .
>
diff mbox series

Patch

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 &&