Received-SPF: pass (google.com: domain of linux-nfs+bounces-2724-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 8 Apr 2024 22:08:16 -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: <ZhSjEGEMyVuyApha@tissot.1015granger.net>
References: <>
 <ZhSPRTPtGCm4InJ8@tissot.1015granger.net>
 <171262513282.17212.15154633882557010394@noble.neil.brown.name>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <171262513282.17212.15154633882557010394@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?YoCYIQd+5/WY1d0fQK62UtBsmzDgdkH+r8PCesHK+jR8z+3SmvC1bC+2GXFU?=
 =?us-ascii?Q?L+JH7ULOhaIjJQe5jr/nacVW5HBbAtULS09el1aqQ5sIYUhsPLPW5tpZzDPq?=
 =?us-ascii?Q?6VfFR0BXTlQU1JRd0/Oyb/ZYOKQqbefpdC6z8jVR18umFvSd0Z2a4+4pDaEV?=
 =?us-ascii?Q?CF32OF6w6KYoD4PEAdoeGfDyrOsrKMato1sysgqVNRZHnmVAUnfupHfNCvsk?=
 =?us-ascii?Q?HDkNGtyKhcmOUKnqBS7GasubCe/mKZ6vvpnyZ/S0IN8PnWguF9yTjo1jLon3?=
 =?us-ascii?Q?puajQYn80aP1Os20WcAdlxMFgXkZkMv2QEPVGhaF1EHVG4V6JaHAVoK8obsp?=
 =?us-ascii?Q?DMU+8mPc5uVwZ83VxNXdOx0E3gSwTlqoBpSbsjeeKZpZ4w1OYseKUSNT2xD8?=
 =?us-ascii?Q?sScw7XWknti/CtlWmw5cYzHz7rfzPihZeYfzjLTWhroD+LcwYVmXZj75pBuN?=
 =?us-ascii?Q?M/D3mW1PP6S0FrsfNcMORhyzkKgJJo6OfXeF6qBsdBIA5YLxkDsH2H7b8k4k?=
 =?us-ascii?Q?cmQgATd0JOa7FcrE3npcrJeaZGcO2EQRZSoJAT4sT9J84kojF5qpVwoM7KrK?=
 =?us-ascii?Q?UskddDw4g2krs0WSPktGJNhwdH/lPADqsb2PvhLTxF5X3wJydgpb+PEDpBNg?=
 =?us-ascii?Q?cwDjHttnGNu1jw0eM4Y/odcrZrQB4JDtHjNhUev3KaQ+RCZlgLs0HQ9sVVot?=
 =?us-ascii?Q?zdMWcAXm4IxmRieKtPn1ga1XYcoJC0y+mjVQdtIauLj6ifuIovcvVEpSeSm3?=
 =?us-ascii?Q?Uvt8NUS0Zlcvq4fI8NcrSwMB1m2PJJ9QHgXiTA+4+OQfZwWrS8NFpLYxIar6?=
 =?us-ascii?Q?V0xLMvasDW9QzATx0HHhy9C95qhzIsLD1CMT9f8ewamdKcIMbsmMpiZhzP1f?=
 =?us-ascii?Q?z6XE7IKlCE7oVxHAIeCJF2e2G69SiEE3lxufPKmbQvrTD1G36xsxSdQUrS3d?=
 =?us-ascii?Q?k0dFCQYP354sWNAhyP2p483JwGmO/qgFswDZb8JSNtRDaSV3rTL8nILQngHT?=
 =?us-ascii?Q?HJwsGhWo6BT7nvOvQdQUgSKQixTAQUFed4wp8uhXHPSp6TSLEZ7nrc36ZcCA?=
 =?us-ascii?Q?IpvA+eICN1bhxWZA8OzaJA+oMVaLrbWaZT3wAAhdYaggbib6rrcF2czi36JF?=
 =?us-ascii?Q?KFSfAS8BffjY8NvhNx6g1GDoC0HY0Mhq+wckExvJ3Msv8or5SJQEZTbzYMmb?=
 =?us-ascii?Q?0FPvvMqLrVTcopyCGjtjKF3P3ofqdS/hetWC7xBGmPc7uWbXbpIfR4s+f7am?=
 =?us-ascii?Q?wkw2PJ7CgMB4ZQUAbcpRktGVrzMyOYTrI6M7Byn4yb7kL5Z8dAFZA//NBsdd?=
 =?us-ascii?Q?Dkqpe8Ror5tEEFXUJz+Z0Ha8fF8FI9iIy85UOzxUAKCj8aC6fmSwKtlUc9s2?=
 =?us-ascii?Q?6/K1pu+MQRhgTugl2i3TkZ+SWveTNgzDW8WUhacopnaavussrzVKu9JgKH/8?=
 =?us-ascii?Q?OvywSIUB5eq5HEMmUgGNlvV7oaO7dHwNAiI+95Oj8+zZsd9A6FTkcCzkuUoi?=
 =?us-ascii?Q?WxG1gsgYc7tfxtMjnGZMetBxuN/OdwTASnjRIm5x+MV2ItTqlwjkbv2bKXeN?=
 =?us-ascii?Q?HmN9CrcnONHnJmVUNHeGMaGjxNEsaigkydILvzUBr2lBS/TahhmzTy+W6w+7?=
 =?us-ascii?Q?fQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	SKTnfwMEZBbnMHRdA906ExbVwzw5cB/9wjRj69oSAHVcp53QOjqvFoxGhfwfV59M7cGWwhZnAqrQ2rkrQQIuk+jL6+Yl1GXJX2hxUjud4cvk945H8g73MswSS7NJOlCmXdYq+LQkifUm3T3JyLx9TEFzWhoYIwP4t3b7HdzbTieISZ2fCwDxYNQz3Yu+9ceJp6kCROfK4F2rZ9i42tR8O75UwmfasoEmvwh1dDHNAT55UXmS3R+4JYI+Q8yc2B+8+y2hg0vFJAwQPojrc+gdUUHanWtYuhN/5cNhrtYERcakkheyLTWleIT/Ga+GFC8jcbFXJtIssM3SmqyJXwkimm5gXEsOQgN4S04epY+ZXGEx77SFgKLFHlBAvXn3VJUZydrGo1H2QT9y//tMZX2K+WaiuZlP5W9i9pm/L43zwBV02bIXODEFhCmFMayIjT3FSrllkJ9SRJEy5k3ZIX8QFo/kVcvJjkjWX57PdA44ZbxJTBFZBHCvfZBkAi62qXa36iEZUex0F2oO+++35hNO3Q+56AaDo+i98oQQNsu4Pg3IWm27TNxySkW2UZTS+zeFZfiRRn8qNZFt6UnItzUclR/2EyP84oT1rhmlVdPwmys=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b0a3dab6-2061-4d9c-9df6-08dc5839ed3c
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Apr 2024 02:08:19.7328
 (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: c743S8AUrcigVyF4RSKloG1aX1KfB9AB5gL5bct2VCfWu/U5eHOmQXUEd/aX9bCKk/1mob/IgxnSy0whHFji7w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB7439
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-08_19,2024-04-05_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 phishscore=0
 adultscore=0 malwarescore=0 bulkscore=0 suspectscore=0 mlxscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2404090011
X-Proofpoint-ORIG-GUID: r8DVV9ZnhjciUGBQApd4kniDRoDhHR3Q
X-Proofpoint-GUID: r8DVV9ZnhjciUGBQApd4kniDRoDhHR3Q

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