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 <[email protected]>
---
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
Hello!
> On Aug 27, 2020, at 3:02 AM, Hou Tao <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
>> ---
>> 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
>
>
>
> .
>
Now when a read delegation is given, two delegation related traces
will be printed:
nfsd_deleg_open: client 5f45b854:e6058001 stateid 00000030:00000001
nfsd_deleg_none: client 5f45b854:e6058001 stateid 0000002f:00000001
Although the intention is to let developers know two stateid are
returned, the traces are confusing about whether or not a read delegation
is handled out. So renaming trace_nfsd_deleg_none() to trace_nfsd_open()
and trace_nfsd_deleg_open() to trace_nfsd_deleg_read() to make
the intension clearer.
The patched traces will be:
nfsd_deleg_read: client 5f48a967:b55b21cd stateid 00000003:00000001
nfsd_open: client 5f48a967:b55b21cd stateid 00000002:00000001
Suggested-by: Chuck Lever <[email protected]>
Signed-off-by: Hou Tao <[email protected]>
---
v1: https://marc.info/?l=linux-nfs&m=159851134513236&w=2
fs/nfsd/nfs4state.c | 4 ++--
fs/nfsd/trace.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c09a2a4281ec9..0525acfe31314 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5126,7 +5126,7 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
- trace_nfsd_deleg_open(&dp->dl_stid.sc_stateid);
+ trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
nfs4_put_stid(&dp->dl_stid);
return;
@@ -5243,7 +5243,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
nfs4_open_delegation(current_fh, open, stp);
nodeleg:
status = nfs_ok;
- trace_nfsd_deleg_none(&stp->st_stid.sc_stateid);
+ trace_nfsd_open(&stp->st_stid.sc_stateid);
out:
/* 4.1 client trying to upgrade/downgrade delegation? */
if (open->op_delegate_type == NFS4_OPEN_DELEGATE_NONE && dp &&
diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index 1861db1bdc670..99bf07800cd09 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -289,8 +289,8 @@ DEFINE_STATEID_EVENT(layout_recall_done);
DEFINE_STATEID_EVENT(layout_recall_fail);
DEFINE_STATEID_EVENT(layout_recall_release);
-DEFINE_STATEID_EVENT(deleg_open);
-DEFINE_STATEID_EVENT(deleg_none);
+DEFINE_STATEID_EVENT(open);
+DEFINE_STATEID_EVENT(deleg_read);
DEFINE_STATEID_EVENT(deleg_break);
DEFINE_STATEID_EVENT(deleg_recall);
--
2.25.0.4.g0ad7144999
> On Aug 28, 2020, at 3:02 AM, Hou Tao <[email protected]> wrote:
>
> Now when a read delegation is given, two delegation related traces
> will be printed:
>
> nfsd_deleg_open: client 5f45b854:e6058001 stateid 00000030:00000001
> nfsd_deleg_none: client 5f45b854:e6058001 stateid 0000002f:00000001
>
> Although the intention is to let developers know two stateid are
> returned, the traces are confusing about whether or not a read delegation
> is handled out. So renaming trace_nfsd_deleg_none() to trace_nfsd_open()
> and trace_nfsd_deleg_open() to trace_nfsd_deleg_read() to make
> the intension clearer.
>
> The patched traces will be:
>
> nfsd_deleg_read: client 5f48a967:b55b21cd stateid 00000003:00000001
> nfsd_open: client 5f48a967:b55b21cd stateid 00000002:00000001
>
> Suggested-by: Chuck Lever <[email protected]>
> Signed-off-by: Hou Tao <[email protected]>
LGTM. I assume Bruce is taking this for v5.10.
> ---
> v1: https://marc.info/?l=linux-nfs&m=159851134513236&w=2
>
> fs/nfsd/nfs4state.c | 4 ++--
> fs/nfsd/trace.h | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c09a2a4281ec9..0525acfe31314 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5126,7 +5126,7 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
>
> memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
>
> - trace_nfsd_deleg_open(&dp->dl_stid.sc_stateid);
> + trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
> open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
> nfs4_put_stid(&dp->dl_stid);
> return;
> @@ -5243,7 +5243,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> nfs4_open_delegation(current_fh, open, stp);
> nodeleg:
> status = nfs_ok;
> - trace_nfsd_deleg_none(&stp->st_stid.sc_stateid);
> + trace_nfsd_open(&stp->st_stid.sc_stateid);
> out:
> /* 4.1 client trying to upgrade/downgrade delegation? */
> if (open->op_delegate_type == NFS4_OPEN_DELEGATE_NONE && dp &&
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index 1861db1bdc670..99bf07800cd09 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -289,8 +289,8 @@ DEFINE_STATEID_EVENT(layout_recall_done);
> DEFINE_STATEID_EVENT(layout_recall_fail);
> DEFINE_STATEID_EVENT(layout_recall_release);
>
> -DEFINE_STATEID_EVENT(deleg_open);
> -DEFINE_STATEID_EVENT(deleg_none);
> +DEFINE_STATEID_EVENT(open);
> +DEFINE_STATEID_EVENT(deleg_read);
> DEFINE_STATEID_EVENT(deleg_break);
> DEFINE_STATEID_EVENT(deleg_recall);
>
> --
> 2.25.0.4.g0ad7144999
>
--
Chuck Lever
On Fri, Aug 28, 2020 at 09:21:55AM -0400, Chuck Lever wrote:
>
>
> > On Aug 28, 2020, at 3:02 AM, Hou Tao <[email protected]> wrote:
> >
> > Now when a read delegation is given, two delegation related traces
> > will be printed:
> >
> > nfsd_deleg_open: client 5f45b854:e6058001 stateid 00000030:00000001
> > nfsd_deleg_none: client 5f45b854:e6058001 stateid 0000002f:00000001
> >
> > Although the intention is to let developers know two stateid are
> > returned, the traces are confusing about whether or not a read delegation
> > is handled out. So renaming trace_nfsd_deleg_none() to trace_nfsd_open()
> > and trace_nfsd_deleg_open() to trace_nfsd_deleg_read() to make
> > the intension clearer.
> >
> > The patched traces will be:
> >
> > nfsd_deleg_read: client 5f48a967:b55b21cd stateid 00000003:00000001
> > nfsd_open: client 5f48a967:b55b21cd stateid 00000002:00000001
> >
> > Suggested-by: Chuck Lever <[email protected]>
> > Signed-off-by: Hou Tao <[email protected]>
>
> LGTM. I assume Bruce is taking this for v5.10.
Applying for 5.10, thanks.--b.
>
>
> > ---
> > v1: https://marc.info/?l=linux-nfs&m=159851134513236&w=2
> >
> > fs/nfsd/nfs4state.c | 4 ++--
> > fs/nfsd/trace.h | 4 ++--
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index c09a2a4281ec9..0525acfe31314 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5126,7 +5126,7 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
> >
> > memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
> >
> > - trace_nfsd_deleg_open(&dp->dl_stid.sc_stateid);
> > + trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
> > open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
> > nfs4_put_stid(&dp->dl_stid);
> > return;
> > @@ -5243,7 +5243,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> > nfs4_open_delegation(current_fh, open, stp);
> > nodeleg:
> > status = nfs_ok;
> > - trace_nfsd_deleg_none(&stp->st_stid.sc_stateid);
> > + trace_nfsd_open(&stp->st_stid.sc_stateid);
> > out:
> > /* 4.1 client trying to upgrade/downgrade delegation? */
> > if (open->op_delegate_type == NFS4_OPEN_DELEGATE_NONE && dp &&
> > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > index 1861db1bdc670..99bf07800cd09 100644
> > --- a/fs/nfsd/trace.h
> > +++ b/fs/nfsd/trace.h
> > @@ -289,8 +289,8 @@ DEFINE_STATEID_EVENT(layout_recall_done);
> > DEFINE_STATEID_EVENT(layout_recall_fail);
> > DEFINE_STATEID_EVENT(layout_recall_release);
> >
> > -DEFINE_STATEID_EVENT(deleg_open);
> > -DEFINE_STATEID_EVENT(deleg_none);
> > +DEFINE_STATEID_EVENT(open);
> > +DEFINE_STATEID_EVENT(deleg_read);
> > DEFINE_STATEID_EVENT(deleg_break);
> > DEFINE_STATEID_EVENT(deleg_recall);
> >
> > --
> > 2.25.0.4.g0ad7144999
> >
>
> --
> Chuck Lever
>
>