On Tue, 2024-04-16 at 09:52 +1000, NeilBrown wrote:
> 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:
> > > >
> > > > > On Apr 15, 2024, at 1:35 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, 2024-04-15 at 17:27 +0000, Chuck Lever III wrote:
> > > > > >
> > > > > > > On Apr 15, 2024, at 1:21 PM, Petr Vorel <pvorel@xxxxxxx> wrote:
> > > > > > >
> > > > > > > /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL.
> > > > > > >
> > > > > > > Signed-off-by: Petr Vorel <pvorel@xxxxxxx>
> > > > > > > ---
> > > > > > > Hi,
> > > > > > >
> > > > > > > @ Jeff, Chuck, Neil, NFS devs: The patch itself whitelist reading
> > > > > > > /proc/fs/nfsd/nfsv4recoverydir in LTP test. I suspect reading failed
> > > > > > > with EINVAL in 6.8 was a deliberate change and expected behavior when
> > > > > > > CONFIG_NFSD_LEGACY_CLIENT_TRACKING is not set:
> > > > > >
> > > > > > I'm not sure it was deliberate. This seems like a behavior
> > > > > > regression. Jeff?
> > > > > >
> > > > >
> > > > > I don't think I intended to make it return -EINVAL. I guess that's what
> > > > > happens when there is no entry for it in the write_op array.
> > > > >
> > > > > With CONFIG_NFSD_LEGACY_CLIENT_TRACKING disabled, that file has no
> > > > > meaning or value at all anymore. Maybe we should just remove the dentry
> > > > > altogether when CONFIG_NFSD_LEGACY_CLIENT_TRACKING is disabled?
> > > >
> > > > 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.
> > > >
> > >
> > > Does it? Where are these rules written?
> > >
> > > 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?
> > >
> > > 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?
> >
> > The best I could find was Documentation/process/stable-api-nonsense.rst.
> >
> > 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, struct fs_context *fc)
> #ifdef CONFIG_NFSD_V4
> [NFSD_Leasetime] = {"nfsv4leasetime", &transaction_ops, S_IWUSR|S_IRUSR},
> [NFSD_Gracetime] = {"nfsv4gracetime", &transaction_ops, S_IWUSR|S_IRUSR},
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
> [NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
> +#endif
> [NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO},
> #endif
> /* last one */ {""}
>
I'm fine with this patch if you want to propose it formally.
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
>
> 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.
That was my thinking too. nfs-utils should handle the lack of this file
gracefully, and nothing else should really care. The LTP test is just
accessing all of the files under /proc so if that file goes missing, it
shouldn't care either.
We can update nfs-utils to silence the log message in later versions
too. In fact, it's probably a good time to think about removing the code
that accesses that file, since it's only used by nfsdcld to convert
"legacy" setups.
--
Jeff Layton <jlayton@xxxxxxxxxx>
