trace_nfsd_ctl_filehandle in write_filehandle has
some similar infos.
write_filehandle is the only caller of exp_rootfh,
so just remove the dprintk parts.
Signed-off-by: Chen Hanxiao <[email protected]>
---
fs/nfsd/export.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 7b641095a665..e7acd820758d 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -1027,9 +1027,6 @@ exp_rootfh(struct net *net, struct auth_domain *clp, char *name,
}
inode = d_inode(path.dentry);
- dprintk("nfsd: exp_rootfh(%s [%p] %s:%s/%ld)\n",
- name, path.dentry, clp->name,
- inode->i_sb->s_id, inode->i_ino);
exp = exp_parent(cd, clp, &path);
if (IS_ERR(exp)) {
err = PTR_ERR(exp);
--
2.39.1
On Mon, 2024-04-08 at 23:06 +0800, Chen Hanxiao wrote:
> trace_nfsd_ctl_filehandle in write_filehandle has
> some similar infos.
>
> write_filehandle is the only caller of exp_rootfh,
> so just remove the dprintk parts.
>
> Signed-off-by: Chen Hanxiao <[email protected]>
> ---
> fs/nfsd/export.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 7b641095a665..e7acd820758d 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1027,9 +1027,6 @@ exp_rootfh(struct net *net, struct auth_domain *clp, char *name,
> }
> inode = d_inode(path.dentry);
>
> - dprintk("nfsd: exp_rootfh(%s [%p] %s:%s/%ld)\n",
> - name, path.dentry, clp->name,
> - inode->i_sb->s_id, inode->i_ino);
> exp = exp_parent(cd, clp, &path);
> if (IS_ERR(exp)) {
> err = PTR_ERR(exp);
Reviewed-by: Jeff Layton <[email protected]>
On Tue, 09 Apr 2024, Chen Hanxiao wrote:
> trace_nfsd_ctl_filehandle in write_filehandle has
> some similar infos.
Not all that similar. The dprintk you are removing includes the inode
number and sb->s_id which the trace point don't include.
Why do you think that information isn't needed?
NeilBrown
>
> write_filehandle is the only caller of exp_rootfh,
> so just remove the dprintk parts.
>
> Signed-off-by: Chen Hanxiao <[email protected]>
> ---
> fs/nfsd/export.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 7b641095a665..e7acd820758d 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1027,9 +1027,6 @@ exp_rootfh(struct net *net, struct auth_domain *clp, char *name,
> }
> inode = d_inode(path.dentry);
>
> - dprintk("nfsd: exp_rootfh(%s [%p] %s:%s/%ld)\n",
> - name, path.dentry, clp->name,
> - inode->i_sb->s_id, inode->i_ino);
> exp = exp_parent(cd, clp, &path);
> if (IS_ERR(exp)) {
> err = PTR_ERR(exp);
> --
> 2.39.1
>
>
>
On Tue, Apr 09, 2024 at 09:21:54AM +1000, NeilBrown wrote:
> On Tue, 09 Apr 2024, Chen Hanxiao wrote:
> > trace_nfsd_ctl_filehandle in write_filehandle has
> > some similar infos.
>
> Not all that similar. The dprintk you are removing includes the inode
> number and sb->s_id which the trace point don't include.
>
> Why do you think that information isn't needed?
I asked him to remove that dprintk.
Can you say why you think that information is useful to provide
via a dprintk? It doesn't seem useful to system administrators,
IMO.
> > write_filehandle is the only caller of exp_rootfh,
> > so just remove the dprintk parts.
> >
> > Signed-off-by: Chen Hanxiao <[email protected]>
> > ---
> > fs/nfsd/export.c | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > index 7b641095a665..e7acd820758d 100644
> > --- a/fs/nfsd/export.c
> > +++ b/fs/nfsd/export.c
> > @@ -1027,9 +1027,6 @@ exp_rootfh(struct net *net, struct auth_domain *clp, char *name,
> > }
> > inode = d_inode(path.dentry);
> >
> > - dprintk("nfsd: exp_rootfh(%s [%p] %s:%s/%ld)\n",
> > - name, path.dentry, clp->name,
> > - inode->i_sb->s_id, inode->i_ino);
> > exp = exp_parent(cd, clp, &path);
> > if (IS_ERR(exp)) {
> > err = PTR_ERR(exp);
> > --
> > 2.39.1
> >
> >
> >
>
--
Chuck Lever
On Tue, 09 Apr 2024, Chuck Lever wrote:
> On Tue, Apr 09, 2024 at 09:21:54AM +1000, NeilBrown wrote:
> > On Tue, 09 Apr 2024, Chen Hanxiao wrote:
> > > trace_nfsd_ctl_filehandle in write_filehandle has
> > > some similar infos.
> >
> > Not all that similar. The dprintk you are removing includes the inode
> > number and sb->s_id which the trace point don't include.
> >
> > Why do you think that information isn't needed?
>
> I asked him to remove that dprintk.
>
> Can you say why you think that information is useful to provide
> via a dprintk? It doesn't seem useful to system administrators,
> IMO.
I'm not saying it is useful, but I don't think the onus is on me.
When removing tracing information, the commit message should at least
acknowledge what is being removed and make some attempt to justify it.
In this case the commit message claimed that nothing was being removed,
which is clearly false. Maybe just the commit message needs to be
fixed.
I don't think these tracepoints are just for system administrators.
They are also for tech support when they are trying to remotely diagnose
a customer problem. It is really hard to know what will actually be
useful. Many times I have found that the particular information that I
want isn't available in any tracing. I expect this is inevitable. We
cannot trace EVERYTHING so there will always be gaps. But removing some
information that was previously generated seems like a regression that
should at least be justified.
In the case of write_filehandle() we are now tracing the request, but
not the result. Is this generally sensible? Would it not make sense to
trace both after the core of the function (exp_rootfh) has completed?
At lease the knfsd_fh_hash() of the generated filehandle could be
reported.
NeilBrown
>
>
> > > write_filehandle is the only caller of exp_rootfh,
> > > so just remove the dprintk parts.
> > >
> > > Signed-off-by: Chen Hanxiao <[email protected]>
> > > ---
> > > fs/nfsd/export.c | 3 ---
> > > 1 file changed, 3 deletions(-)
> > >
> > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > > index 7b641095a665..e7acd820758d 100644
> > > --- a/fs/nfsd/export.c
> > > +++ b/fs/nfsd/export.c
> > > @@ -1027,9 +1027,6 @@ exp_rootfh(struct net *net, struct auth_domain *clp, char *name,
> > > }
> > > inode = d_inode(path.dentry);
> > >
> > > - dprintk("nfsd: exp_rootfh(%s [%p] %s:%s/%ld)\n",
> > > - name, path.dentry, clp->name,
> > > - inode->i_sb->s_id, inode->i_ino);
> > > exp = exp_parent(cd, clp, &path);
> > > if (IS_ERR(exp)) {
> > > err = PTR_ERR(exp);
> > > --
> > > 2.39.1
> > >
> > >
> > >
> >
>
> --
> Chuck Lever
>
On Tue, Apr 09, 2024 at 11:12:12AM +1000, NeilBrown wrote:
> On Tue, 09 Apr 2024, Chuck Lever wrote:
> > On Tue, Apr 09, 2024 at 09:21:54AM +1000, NeilBrown wrote:
> > > On Tue, 09 Apr 2024, Chen Hanxiao wrote:
> > > > trace_nfsd_ctl_filehandle in write_filehandle has
> > > > some similar infos.
> > >
> > > Not all that similar. The dprintk you are removing includes the inode
> > > number and sb->s_id which the trace point don't include.
> > >
> > > Why do you think that information isn't needed?
> >
> > I asked him to remove that dprintk.
> >
> > Can you say why you think that information is useful to provide
> > via a dprintk? It doesn't seem useful to system administrators,
> > IMO.
>
> I'm not saying it is useful, but I don't think the onus is on me.
No the onus isn't on you. I'm merely asking for feedback (and I
explain why below).
> When removing tracing information, the commit message should at least
> acknowledge what is being removed and make some attempt to justify it.
> In this case the commit message claimed that nothing was being removed,
> which is clearly false. Maybe just the commit message needs to be
> fixed.
Agreed, the patch description could have more detail and a proper
justification.
> I don't think these tracepoints are just for system administrators.
> They are also for tech support when they are trying to remotely diagnose
> a customer problem. It is really hard to know what will actually be
> useful. Many times I have found that the particular information that I
> want isn't available in any tracing. I expect this is inevitable. We
> cannot trace EVERYTHING so there will always be gaps.
When there is no in-code tracepoint available, the usual course of
action these days is to wheel out BPF, systemtap, or drgn. Distro
support engineers know how to do that. Server administrators might
not be so well trained, so I consider dprintk() of primary
importance to them.
Here the dprintk() is reporting information that seems useful only
to kernel developers. That's a code smell IMO. And the guidance in
these cases, historically, has been to remove such observability
either before a patch is merged, or after the fact, as we're doing
here.
> But removing some information that was previously generated seems
> like a regression that should at least be justified.
> In the case of write_filehandle() we are now tracing the request, but
> not the result. Is this generally sensible?
Let's instead look at the specific situation. The purpose of the
nfsd_ctl_* tracepoints is to record in the trace log when
configuration changes are made, in order to juxtapose those with
other server operation.
So, here, it's quite sensible. We want to observe the information
that was passed from user space, and the starting timestamp.
> Would it not make sense to
> trace both after the core of the function (exp_rootfh) has completed?
In some cases there are already tracepoints that would report or
infer the new state, so reporting a result would be redundant in
those cases.
> At lease the knfsd_fh_hash() of the generated filehandle could be
> reported.
Chen's original patch replaced the dprintk with a tracepoint. So I
asked, a week or so ago, for exactly this kind of feedback. There
have been no responses until now. Therefore it seemed logical to
assume no-one had a use for this info.
The folks whom this information would serve have been silent to date
with any specific suggestions, and usually we hear from someone
quickly when removing observability that is depended upon.
We could record the FH hash, but what would it be used for? User
space requested the FH, which can be reported by the requesting
program.
I'm not hearing a convincing specific justification for maintaining
observability here.... but we have a few more weeks before making a
more permanent decision.
--
Chuck Lever
On Tue, 09 Apr 2024, Chuck Lever wrote:
> On Tue, Apr 09, 2024 at 11:12:12AM +1000, NeilBrown wrote:
> > On Tue, 09 Apr 2024, Chuck Lever wrote:
> > > On Tue, Apr 09, 2024 at 09:21:54AM +1000, NeilBrown wrote:
> > > > On Tue, 09 Apr 2024, Chen Hanxiao wrote:
> > > > > trace_nfsd_ctl_filehandle in write_filehandle has
> > > > > some similar infos.
> > > >
> > > > Not all that similar. The dprintk you are removing includes the inode
> > > > number and sb->s_id which the trace point don't include.
> > > >
> > > > Why do you think that information isn't needed?
> > >
> > > I asked him to remove that dprintk.
> > >
> > > Can you say why you think that information is useful to provide
> > > via a dprintk? It doesn't seem useful to system administrators,
> > > IMO.
> >
> > I'm not saying it is useful, but I don't think the onus is on me.
>
> No the onus isn't on you. I'm merely asking for feedback (and I
> explain why below).
>
>
> > When removing tracing information, the commit message should at least
> > acknowledge what is being removed and make some attempt to justify it.
> > In this case the commit message claimed that nothing was being removed,
> > which is clearly false. Maybe just the commit message needs to be
> > fixed.
>
> Agreed, the patch description could have more detail and a proper
> justification.
>
>
> > I don't think these tracepoints are just for system administrators.
> > They are also for tech support when they are trying to remotely diagnose
> > a customer problem. It is really hard to know what will actually be
> > useful. Many times I have found that the particular information that I
> > want isn't available in any tracing. I expect this is inevitable. We
> > cannot trace EVERYTHING so there will always be gaps.
>
> When there is no in-code tracepoint available, the usual course of
> action these days is to wheel out BPF, systemtap, or drgn. Distro
> support engineers know how to do that. Server administrators might
> not be so well trained, so I consider dprintk() of primary
> importance to them.
Point for clarification: do you see tracepoints and dprintk as having
different audiences, or do you see them as serving the same purpose with
dprintk being a legacy implementation which is slowly being transitioned
to tracepoints? I had assumed the latter but your language above makes
me wonder.
Certainly systemtap and other are invaluable and should be understood by
support engineers, but its a whole lot easier (and quicker) if the
useful information is easily available. Our first-level support can
easily ask for a rpcdebug or trace-cmd trace and if we can get all the
useful information from there, that is a big win.
(It'll be nice when we can stop using rpcdebug and only ask for
trace-cmd output).
>
> Here the dprintk() is reporting information that seems useful only
> to kernel developers. That's a code smell IMO. And the guidance in
> these cases, historically, has been to remove such observability
> either before a patch is merged, or after the fact, as we're doing
> here.
Interesting... I had always assumed that dprintk/trace was largely
useless without some understanding of the inner workings of the kernel.
I certainly need to dig around before I can work out how to interpret
the trace information. So I think of all of it as "useful only to
kernel developers" ... or potential developers.
>
>
> > But removing some information that was previously generated seems
> > like a regression that should at least be justified.
> > In the case of write_filehandle() we are now tracing the request, but
> > not the result. Is this generally sensible?
>
> Let's instead look at the specific situation. The purpose of the
> nfsd_ctl_* tracepoints is to record in the trace log when
> configuration changes are made, in order to juxtapose those with
> other server operation.
configuration changes? write_filehandle is used by mountd to get a
filehandle to return to a v3 MOUNT request. I guess that is a config
change on the client, but not on the server.
>
> So, here, it's quite sensible. We want to observe the information
> that was passed from user space, and the starting timestamp.
>
>
> > Would it not make sense to
> > trace both after the core of the function (exp_rootfh) has completed?
>
> In some cases there are already tracepoints that would report or
> infer the new state, so reporting a result would be redundant in
> those cases.
Well there was already a dprintk, but that is being removed... I don't
know what other tracepoint might already provide this info. Maybe I'm
confused about your meaning.
>
>
> > At lease the knfsd_fh_hash() of the generated filehandle could be
> > reported.
>
> Chen's original patch replaced the dprintk with a tracepoint. So I
> asked, a week or so ago, for exactly this kind of feedback. There
> have been no responses until now. Therefore it seemed logical to
> assume no-one had a use for this info.
Sorry - I don't pay as close attention to nfs traffic as I might like.
(In this case I was on leave a week ago...)
>
> The folks whom this information would serve have been silent to date
> with any specific suggestions, and usually we hear from someone
> quickly when removing observability that is depended upon.
>
> We could record the FH hash, but what would it be used for? User
> space requested the FH, which can be reported by the requesting
> program.
What is the FH hash ever used for? Presumably for tracking a particular
object through multiple tracepoints. Maybe we need to see when the dir
that the client mounted gets accessed later?
>
> I'm not hearing a convincing specific justification for maintaining
> observability here.... but we have a few more weeks before making a
> more permanent decision.
>
I'm not seeing a significant cost in maintaining observability. But I
don't know that I care quite enough to write a patch. So I'll leave it
up to you.
Thanks,
NeilBrown
>
> --
> Chuck Lever
>
On Tue, Apr 09, 2024 at 01:06:00PM +1000, NeilBrown wrote:
> On Tue, 09 Apr 2024, Chuck Lever wrote:
> > On Tue, Apr 09, 2024 at 11:12:12AM +1000, NeilBrown wrote:
> > > On Tue, 09 Apr 2024, Chuck Lever wrote:
> > > > On Tue, Apr 09, 2024 at 09:21:54AM +1000, NeilBrown wrote:
> > > > > On Tue, 09 Apr 2024, Chen Hanxiao wrote:
> > > > > > trace_nfsd_ctl_filehandle in write_filehandle has
> > > > > > some similar infos.
> > > > >
> > > > > Not all that similar. The dprintk you are removing includes the inode
> > > > > number and sb->s_id which the trace point don't include.
> > > > >
> > > > > Why do you think that information isn't needed?
> > > >
> > > > I asked him to remove that dprintk.
> > > >
> > > > Can you say why you think that information is useful to provide
> > > > via a dprintk? It doesn't seem useful to system administrators,
> > > > IMO.
> > >
> > > I'm not saying it is useful, but I don't think the onus is on me.
> >
> > No the onus isn't on you. I'm merely asking for feedback (and I
> > explain why below).
> >
> >
> > > When removing tracing information, the commit message should at least
> > > acknowledge what is being removed and make some attempt to justify it.
> > > In this case the commit message claimed that nothing was being removed,
> > > which is clearly false. Maybe just the commit message needs to be
> > > fixed.
> >
> > Agreed, the patch description could have more detail and a proper
> > justification.
> >
> >
> > > I don't think these tracepoints are just for system administrators.
> > > They are also for tech support when they are trying to remotely diagnose
> > > a customer problem. It is really hard to know what will actually be
> > > useful. Many times I have found that the particular information that I
> > > want isn't available in any tracing. I expect this is inevitable. We
> > > cannot trace EVERYTHING so there will always be gaps.
> >
> > When there is no in-code tracepoint available, the usual course of
> > action these days is to wheel out BPF, systemtap, or drgn. Distro
> > support engineers know how to do that. Server administrators might
> > not be so well trained, so I consider dprintk() of primary
> > importance to them.
>
> Point for clarification: do you see tracepoints and dprintk as having
> different audiences, or do you see them as serving the same purpose with
> dprintk being a legacy implementation which is slowly being transitioned
> to tracepoints? I had assumed the latter but your language above makes
> me wonder.
I'm leaving some dprintk call sites in place when they can report
information that system administrators can use to troubleshoot
local configuration or network issues, say.
tracepoints can serve a similar purpose, but can also be used for
deeper introspection because they can be left persistently enabled,
can handle a much higher volume of traffic, and have a higher signal
to noise ratio.
> Certainly systemtap and other are invaluable and should be understood by
> support engineers, but its a whole lot easier (and quicker) if the
> useful information is easily available. Our first-level support can
> easily ask for a rpcdebug or trace-cmd trace and if we can get all the
> useful information from there, that is a big win.
> (It'll be nice when we can stop using rpcdebug and only ask for
> trace-cmd output).
>
> > Here the dprintk() is reporting information that seems useful only
> > to kernel developers. That's a code smell IMO. And the guidance in
> > these cases, historically, has been to remove such observability
> > either before a patch is merged, or after the fact, as we're doing
> > here.
>
> Interesting... I had always assumed that dprintk/trace was largely
> useless without some understanding of the inner workings of the kernel.
> I certainly need to dig around before I can work out how to interpret
> the trace information. So I think of all of it as "useful only to
> kernel developers" ... or potential developers.
In the past, dprintk call sites were a mix of information for kernel
developers and local administrators. The trend on the client side
has been to remove or replace the dprintk call sites that were used
to develop code and are not helpful to system administrators.
That is probably due to the coarse granularity of dprintk: when you
enable a class, you get a whole bunch at the same time.
Now that the system journal is rate-throttled, the amount of noise
this can generate makes dprintk pretty useless for more than one or
two messages.
> > I'm not hearing a convincing specific justification for maintaining
> > observability here.... but we have a few more weeks before making a
> > more permanent decision.
> >
>
> I'm not seeing a significant cost in maintaining observability. But I
> don't know that I care quite enough to write a patch. So I'll leave it
> up to you.
The only intention was code clean up.
--
Chuck Lever