Received-SPF: pass (google.com: domain of linux-nfs+bounces-2833-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
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: "Jeff Layton" <jlayton@kernel.org>, "Petr Vorel" <pvorel@suse.cz>,
 "ltp@lists.linux.it" <ltp@lists.linux.it>, "Cyril Hrubis" <chrubis@suse.cz>,
 "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 1/1] proc01: Whitelist /proc/fs/nfsd/nfsv4recoverydir
In-reply-to: <Zh2XBV/sW67dx+wp@tissot.1015granger.net>
References: <>, <Zh2XBV/sW67dx+wp@tissot.1015granger.net>
Date: Tue, 16 Apr 2024 09:52:11 +1000
Message-id: <171322513118.17212.2981486436520645718@noble.neil.brown.name>

On Tue, 16 Apr 2024, Chuck Lever wrote:
> On Mon, Apr 15, 2024 at 01:43:37PM -0400, Jeff Layton wrote:
> > On Mon, 2024-04-15 at 17:37 +0000, Chuck Lever III wrote:
> > >=20
> > > > On Apr 15, 2024, at 1:35=E2=80=AFPM, Jeff Layton <jlayton@kernel.org>=
 wrote:
> > > >=20
> > > > On Mon, 2024-04-15 at 17:27 +0000, Chuck Lever III wrote:
> > > > >=20
> > > > > > On Apr 15, 2024, at 1:21=E2=80=AFPM, Petr Vorel <pvorel@suse.cz> =
wrote:
> > > > > >=20
> > > > > > /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EIN=
VAL.
> > > > > >=20
> > > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > > > > ---
> > > > > > Hi,
> > > > > >=20
> > > > > > @ Jeff, Chuck, Neil, NFS devs: The patch itself whitelist reading
> > > > > > /proc/fs/nfsd/nfsv4recoverydir in LTP test. I suspect reading fai=
led
> > > > > > with EINVAL in 6.8 was a deliberate change and expected behavior =
when
> > > > > > CONFIG_NFSD_LEGACY_CLIENT_TRACKING is not set:
> > > > >=20
> > > > > I'm not sure it was deliberate. This seems like a behavior
> > > > > regression. Jeff?
> > > > >=20
> > > >=20
> > > > I don't think I intended to make it return -EINVAL. I guess that's wh=
at
> > > > happens when there is no entry for it in the write_op array.
> > > >=20
> > > > With CONFIG_NFSD_LEGACY_CLIENT_TRACKING disabled, that file has no
> > > > meaning or value at all anymore. Maybe we should just remove the dent=
ry
> > > > altogether when CONFIG_NFSD_LEGACY_CLIENT_TRACKING is disabled?
> > >=20
> > > My understanding of the rules about modifying this part of
> > > the kernel-user interface is that the file has to stay, even
> > > though it's now a no-op.
> > >=20
> >=20
> > Does it?  Where are these rules written?=20
> >=20
> > What should we have it do now when read and written? Maybe EOPNOTSUPP
> > would be better, if we can make it just return an error?
> >=20
> > We could also make it just discard written data, and present a blank
> > string when read. What do the rules say we are required to do here?
>=20
> The best I could find was Documentation/process/stable-api-nonsense.rst.
>=20
> Tell you what, you and Petr work out what you'd like to do, let's
> figure out the right set of folks to review changes in /proc, and
> we'll go from there. If no-one has a problem removing the file, I'm
> not going to stand in the way.

I don't think we need any external review for this.  While the file is
in /proc, it is not in procfs but in nfsdfs.  So people out side the
nfsd community are unlikely to care.  And this isn't a hard removal.  It
is just a new config option that allows a file to be removed.

I think we do want to completely remove the file, not just let it return
an error:
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -51,7 +51,9 @@ enum {
 #ifdef CONFIG_NFSD_V4
 	NFSD_Leasetime,
 	NFSD_Gracetime,
+#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
 	NFSD_RecoveryDir,
+#endif
 	NFSD_V4EndGrace,
 #endif
 	NFSD_MaxReserved
@@ -1360,7 +1362,9 @@ static int nfsd_fill_super(struct super_block *sb, stru=
ct fs_context *fc)
 #ifdef CONFIG_NFSD_V4
 		[NFSD_Leasetime] =3D {"nfsv4leasetime", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Gracetime] =3D {"nfsv4gracetime", &transaction_ops, S_IWUSR|S_IRUSR},
+#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
 		[NFSD_RecoveryDir] =3D {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IR=
USR},
+#endif
 		[NFSD_V4EndGrace] =3D {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO},
 #endif
 		/* last one */ {""}


My understand of the stability rule is "if Linus doesn't hear about it,
then it isn't a regression".  Also known as "no harm, no foul".

So if we manage the change to everyone's satisfaction, then it is
perfectly OK to make the change.  nfs-utils already handles a missing
file fairly well - you get a D_GENERAL log message, but that is all.
Petr's fix for ltp should allow it to work.  I would be greatly
surprised if anything else (except possibly other testing code) would
care.

NeilBrown