| Message ID | 20200827070237.19942-1-houtao1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: don't call trace_nfsd_deleg_none() if read delegation is given | expand |
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
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 --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 &&
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(-)