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 */ {""}
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
