* [PATCH] nfsd: don't call trace_nfsd_deleg_none() if read delegation is given
@ 2020-08-27 7:02 Hou Tao
2020-08-27 14:43 ` Chuck Lever
0 siblings, 1 reply; 6+ messages in thread
From: Hou Tao @ 2020-08-27 7:02 UTC (permalink / raw)
To: J. Bruce Fields, Chuck Lever; +Cc: linux-nfs
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] nfsd: don't call trace_nfsd_deleg_none() if read delegation is given
2020-08-27 7:02 [PATCH] nfsd: don't call trace_nfsd_deleg_none() if read delegation is given Hou Tao
@ 2020-08-27 14:43 ` Chuck Lever
2020-08-28 2:23 ` Hou Tao
2020-08-28 7:02 ` [PATCH v2] nfsd: rename delegation related tracepoints to make them less confusing Hou Tao
0 siblings, 2 replies; 6+ messages in thread
From: Chuck Lever @ 2020-08-27 14:43 UTC (permalink / raw)
To: Hou Tao; +Cc: Bruce Fields, Linux NFS Mailing List
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nfsd: don't call trace_nfsd_deleg_none() if read delegation is given
2020-08-27 14:43 ` Chuck Lever
@ 2020-08-28 2:23 ` Hou Tao
2020-08-28 7:02 ` [PATCH v2] nfsd: rename delegation related tracepoints to make them less confusing Hou Tao
1 sibling, 0 replies; 6+ messages in thread
From: Hou Tao @ 2020-08-28 2:23 UTC (permalink / raw)
To: Chuck Lever; +Cc: Bruce Fields, Linux NFS Mailing List
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
>
>
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] nfsd: rename delegation related tracepoints to make them less confusing
2020-08-27 14:43 ` Chuck Lever
2020-08-28 2:23 ` Hou Tao
@ 2020-08-28 7:02 ` Hou Tao
2020-08-28 13:21 ` Chuck Lever
1 sibling, 1 reply; 6+ messages in thread
From: Hou Tao @ 2020-08-28 7:02 UTC (permalink / raw)
To: J. Bruce Fields, Chuck Lever; +Cc: linux-nfs
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 <chuck.lever@oracle.com>
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] nfsd: rename delegation related tracepoints to make them less confusing
2020-08-28 7:02 ` [PATCH v2] nfsd: rename delegation related tracepoints to make them less confusing Hou Tao
@ 2020-08-28 13:21 ` Chuck Lever
2020-08-29 15:20 ` Bruce Fields
0 siblings, 1 reply; 6+ messages in thread
From: Chuck Lever @ 2020-08-28 13:21 UTC (permalink / raw)
To: Hou Tao, Bruce Fields; +Cc: Linux NFS Mailing List
> On Aug 28, 2020, at 3:02 AM, Hou Tao <houtao1@huawei.com> 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 <chuck.lever@oracle.com>
> Signed-off-by: Hou Tao <houtao1@huawei.com>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] nfsd: rename delegation related tracepoints to make them less confusing
2020-08-28 13:21 ` Chuck Lever
@ 2020-08-29 15:20 ` Bruce Fields
0 siblings, 0 replies; 6+ messages in thread
From: Bruce Fields @ 2020-08-29 15:20 UTC (permalink / raw)
To: Chuck Lever; +Cc: Hou Tao, Linux NFS Mailing List
On Fri, Aug 28, 2020 at 09:21:55AM -0400, Chuck Lever wrote:
>
>
> > On Aug 28, 2020, at 3:02 AM, Hou Tao <houtao1@huawei.com> 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 <chuck.lever@oracle.com>
> > Signed-off-by: Hou Tao <houtao1@huawei.com>
>
> 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-29 15:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-27 7:02 [PATCH] nfsd: don't call trace_nfsd_deleg_none() if read delegation is given Hou Tao
2020-08-27 14:43 ` Chuck Lever
2020-08-28 2:23 ` Hou Tao
2020-08-28 7:02 ` [PATCH v2] nfsd: rename delegation related tracepoints to make them less confusing Hou Tao
2020-08-28 13:21 ` Chuck Lever
2020-08-29 15:20 ` Bruce Fields
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.