Received-SPF: pass (google.com: domain of linux-nfs+bounces-2731-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 9 Apr 2024 10:09:12 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: NeilBrown <neilb@suse.de>
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
Message-ID: <ZhVMCHHIUn8pobje@tissot.1015granger.net>
References: <>
 <ZhSjEGEMyVuyApha@tissot.1015granger.net>
 <171263196068.17212.17916891599918470772@noble.neil.brown.name>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <171263196068.17212.17916891599918470772@noble.neil.brown.name>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?hVmehWseXVzpubBgjG7aLKsZUkeFpyu7/k4P2w8IOC/0RePGx8f4PRXge5Zh?=
 =?us-ascii?Q?o6eOAin98Pb5hOW4sQz6/ftDiV9WfNb/w21vrlrYt5JnBX1TM95Ducv/ty75?=
 =?us-ascii?Q?94UVrOykRoNEXIAF63uuEMGhbGDxbaNI3pePh4v91WmrdFyf2G+TI0xZGNfB?=
 =?us-ascii?Q?NoXKqh770qqJzbn9CNoL4osvK1HEOt7pfwxaHssrcjtesHxKHmt6JnO++fux?=
 =?us-ascii?Q?L16u4a1XNR7U9BwGQAJgQI4UPKFCwZSuJ1+L/uNZWzQCwG3BdyipyvPE3MmY?=
 =?us-ascii?Q?WZGbVEwWlIG/VeLZpNMammIymI/DjXz+hnbtRcOH1Ksyhu2aGqeb/PlhUi+s?=
 =?us-ascii?Q?84Zfg2vyoen9abgdzmNOsUCqSQeAxAL7WWWZOvgGFmuDtsnXoXIcEx/GzB+l?=
 =?us-ascii?Q?LVtK4MubS8x35xZ6MQxl/niHORYRZoSV6eO1cQnmA56sjc+RbIk3K0iJf/Hr?=
 =?us-ascii?Q?fKZklV5krWFLHEhytSBXyt5rUajXZxdo+HdkfD7c5OmnaBLyOCgOnGP7bh/2?=
 =?us-ascii?Q?5nXQbC8lYPL3VstHsYRPOvYy4QJa3DSKlJU6lS8BiOyzqN8mVOWHxuyJjSGg?=
 =?us-ascii?Q?doe9O8phmWhZHf9l4HgKUF/VW3DVFN4htbV9YUccUyDqibhn3ohe+vIgBwwg?=
 =?us-ascii?Q?9NvxA8EJv3/aeJv2kOLSrZHhSo2USiaiquNapciGQ5bsuO2WiCa4D9pRKaeB?=
 =?us-ascii?Q?xRBJxEJI+6RxY0ALWl9i52Pl3Cw5YOg3/Re6zWGF6W5WcufiaBOTaNRA6bct?=
 =?us-ascii?Q?UwMD+n6hP7eufxq8prEwKLjq3Rv/Xocg4A62rUgC+H4VIR8h6IHEW1/SsZ/X?=
 =?us-ascii?Q?rr0n0fnCC68iwa1EVOgLVcwkJQYLQQbKpaybFcFqk0xx3lNYifQrrszSkcHj?=
 =?us-ascii?Q?QkJkRaSjTQzVpQ14ACRMzTP86HTM4t9IEO2jfGIjJDyS7zr+OmLiV3GUJekj?=
 =?us-ascii?Q?i0KkWpETKtkkfDR4/7vd+1UKTJUiEClK9HrFBu5jzNkhm7NwpBaTNQO4qJAf?=
 =?us-ascii?Q?bI0J6GRnHSakO8N3Pz1V9b6s7uPRYfB7YKzQLiwWYEVYyTGhgOH6gT0a6Vj9?=
 =?us-ascii?Q?bKUKQhrkNJ1yPlN3DI6stT+WtzEfYMEhJ56ryE4Rh6U1noNmYWl59ZTTKxuw?=
 =?us-ascii?Q?BDJ4UtDDEuHuPuKaqH0xYxkiMGqliVjlgqjPXlNCh4Q4FOje7xlmU0MB5Vy3?=
 =?us-ascii?Q?PhxLDlQ5oMhT0mHr01LZ59C3bMvuJ5wOcuzZhEHr4UwSESnW0cfMQ3HwS1JU?=
 =?us-ascii?Q?m4izL7yWWMO+pJ1+x7EJoRnyLfYwNVeYPyr8W4Uai01ZHWbR2d0xkIdqarub?=
 =?us-ascii?Q?ch8INjVGucKUUJl1EYi6igF9wLpeyK+L9fGvO/n+w2ybuoYO/XcCmV6JF4LC?=
 =?us-ascii?Q?i4NCKWThNRf//WICVUl5E4s6MlFqITMWR27AmGQ9oVAMKWIkVzpkpWx0AxTj?=
 =?us-ascii?Q?TJk1LKgYJsPF7/B5b88ksAGjY3H67+PvLdZiFm9g59BkX+WSF+GM1yCCODt/?=
 =?us-ascii?Q?+1eWeYmygE3CCmoabOsJClEz8S8Zed//0bVDwv6PK3s8jYtxFraH3fsgGXl1?=
 =?us-ascii?Q?+4EsY27nsd/lt1R6ilqniPO+jW0VgGJeJzl/KgCi/9BYzE7urXV6uK5mxke5?=
 =?us-ascii?Q?dQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	tiADcnH9RqMWXTgDGHShfojog7t3/GaMFsxww4XyB/6x4KQBFy+M+8ch7OVUmaTeWK5c0P0n/RqtqCTsyfzp1ItqGPLPkZxeEl9eybiuOaKWzl81BHC2I7PsdZGRw7G/fRpaBpUDRCmqp179uN617WBye0mdd8tLnmtBcZCgPYKfMiLYCGOP2/iYetxxCC2Vd6DRTmdcnK2iPWoFDVt3Nocjfn0+tysQ+doRi7a45nA/81k1bmdOAgADNqBhG1zm76LOAeFWZQ/Su11ZQsrSDmanklNtlbh7cdnQp/nvYynu0b7rGC0HPiUHMTtzQtRWgna3YNkkm/dnTfukluqvOkzklnpCpk/wMrFWL40F9c9Q4Zm9M6j02yvkGXrxVOT23zSmoLGj8Edb6WmARkeZRea4JiXt0pB98pODuPyOVPTPzhmSAf+EEfEpYy70o1b567LPzkibyMov3l+K8PHPNyC4ujbUnCXtcYeZ1IxqhkIVayOHZWSyWOGq5g9Si5Ns5+mgRKGY+Uem1TXUfUiV4n3aJRRbCz098+jijoCqAZWxxbEJzG9SvMpdjB/j7WC1IwrMhhZp8Oz1hOjd1XJ9qe5Id0ssFS2+oMTZ0wocTwk=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: de438ea0-7894-49a6-26da-08dc589ea3aa
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Apr 2024 14:09:15.5228
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: H2BTALjqIqcyKbaBd+VGNw60kX/8OXpd1WvNqAq4JpFFfeuBauPRsPJ0UrE7TRFYIQhgpiIgjTUniac3jLDYEA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR10MB7045
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-04-09_10,2024-04-09_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 suspectscore=0
 bulkscore=0 mlxlogscore=999 mlxscore=0 adultscore=0 phishscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2404090092
X-Proofpoint-ORIG-GUID: GFPQTeNaC11Lmk_5WFH-5fhsMZIeniRC
X-Proofpoint-GUID: GFPQTeNaC11Lmk_5WFH-5fhsMZIeniRC

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