Received-SPF: pass (google.com: domain of linux-nfs+bounces-2723-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Chuck Lever" <chuck.lever@oracle.com>
Cc: "Chen Hanxiao" <chenhx.fnst@fujitsu.com>,
 "Jeff Layton" <jlayton@kernel.org>, "Olga Kornievskaia" <kolga@netapp.com>,
 "Dai Ngo" <Dai.Ngo@oracle.com>, "Tom Talpey" <tom@talpey.com>,
 linux-nfs@vger.kernel.org
Subject: Re: [PATCH] NFSD: remove redundant dprintk in exp_rootfh
In-reply-to: <ZhSPRTPtGCm4InJ8@tissot.1015granger.net>
References: <>, <ZhSPRTPtGCm4InJ8@tissot.1015granger.net>
Date: Tue, 09 Apr 2024 11:12:12 +1000
Message-id: <171262513282.17212.15154633882557010394@noble.neil.brown.name>

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.
> >=20
> > Not all that similar.  The dprintk you are removing includes the inode
> > number and sb->s_id which the trace point don't include.
> >=20
> > Why do you think that information isn't needed?
>=20
> I asked him to remove that dprintk.
>=20
> 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


>=20
>=20
> > > write_filehandle is the only caller of exp_rootfh,
> > > so just remove the dprintk parts.
> > >=20
> > > Signed-off-by: Chen Hanxiao <chenhx.fnst@fujitsu.com>
> > > ---
> > >  fs/nfsd/export.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > >=20
> > > 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 *c=
lp, char *name,
> > >  	}
> > >  	inode =3D d_inode(path.dentry);
> > > =20
> > > -	dprintk("nfsd: exp_rootfh(%s [%p] %s:%s/%ld)\n",
> > > -		 name, path.dentry, clp->name,
> > > -		 inode->i_sb->s_id, inode->i_ino);
> > >  	exp =3D exp_parent(cd, clp, &path);
> > >  	if (IS_ERR(exp)) {
> > >  		err =3D PTR_ERR(exp);
> > > --=20
> > > 2.39.1
> > >=20
> > >=20
> > >=20
> >=20
>=20
> --=20
> Chuck Lever
>=20