Message ID | 171330258224.17212.9790424282163530018@noble.neil.brown.name (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | nfsd: don't create nfsv4recoverydir in nfsdfs when not used. | expand |
Hi Neil, all, > When CONFIG_NFSD_LEGACY_CLIENT_TRACKING is not set, the virtual file > /proc/fs/nfsd/nfsv4recoverydir > is created but responds EINVAL to any access. > This is not useful, is somewhat surprising, and it causes ltp to > complain. > The only known user of this file is in nfs-utils, which handles > non-existence and read-failure equally well. So there is nothing to > gain from leaving the file present but inaccessible. > So this patch removes the file when its content is not available - i.e. > when that config option is not selected. > Also remove the #ifdef which hides some of the enum values when > CONFIG_NFSD_V$ not selection. simple_fill_super() quietly ignores array > entries that are not present, so having slots in the array that don't > get used is perfectly acceptable. So there is no value in this #ifdef. > Reported-by: Petr Vorel <pvorel@suse.cz> > Reviewed-by: Jeff Layton <jlayton@kernel.org> > Fixes: 74fd48739d04 ("nfsd: new Kconfig option for legacy client tracking") > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfsctl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > index 93c87587e646..340c5d61f199 100644 > --- a/fs/nfsd/nfsctl.c > +++ b/fs/nfsd/nfsctl.c > @@ -48,12 +48,10 @@ enum { > NFSD_MaxBlkSize, > NFSD_MaxConnections, > NFSD_Filecache, > -#ifdef CONFIG_NFSD_V4 > NFSD_Leasetime, > NFSD_Gracetime, > NFSD_RecoveryDir, > NFSD_V4EndGrace, > -#endif > NFSD_MaxReserved > }; > @@ -1360,7 +1358,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 LGTM. Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr > [NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO}, > #endif > /* last one */ {""}
On Wed, Apr 17, 2024 at 07:25:16AM +0200, Petr Vorel wrote: > Hi Neil, all, > > > When CONFIG_NFSD_LEGACY_CLIENT_TRACKING is not set, the virtual file > > /proc/fs/nfsd/nfsv4recoverydir > > is created but responds EINVAL to any access. > > This is not useful, is somewhat surprising, and it causes ltp to > > complain. > > > The only known user of this file is in nfs-utils, which handles > > non-existence and read-failure equally well. So there is nothing to > > gain from leaving the file present but inaccessible. > > > So this patch removes the file when its content is not available - i.e. > > when that config option is not selected. > > > Also remove the #ifdef which hides some of the enum values when > > CONFIG_NFSD_V$ not selection. simple_fill_super() quietly ignores array > > entries that are not present, so having slots in the array that don't > > get used is perfectly acceptable. So there is no value in this #ifdef. > > > Reported-by: Petr Vorel <pvorel@suse.cz> > > Reviewed-by: Jeff Layton <jlayton@kernel.org> > > Fixes: 74fd48739d04 ("nfsd: new Kconfig option for legacy client tracking") > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > fs/nfsd/nfsctl.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > > index 93c87587e646..340c5d61f199 100644 > > --- a/fs/nfsd/nfsctl.c > > +++ b/fs/nfsd/nfsctl.c > > @@ -48,12 +48,10 @@ enum { > > NFSD_MaxBlkSize, > > NFSD_MaxConnections, > > NFSD_Filecache, > > -#ifdef CONFIG_NFSD_V4 > > NFSD_Leasetime, > > NFSD_Gracetime, > > NFSD_RecoveryDir, > > NFSD_V4EndGrace, > > -#endif > > NFSD_MaxReserved > > }; > > > @@ -1360,7 +1358,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 > > LGTM. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Kind regards, > Petr > > > [NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO}, > > #endif > > /* last one */ {""} Thanks to you both! Applied to nfsd-next (for v6.10), unless you'd like this to go in sooner.
> On Wed, Apr 17, 2024 at 07:25:16AM +0200, Petr Vorel wrote: > > Hi Neil, all, > > > When CONFIG_NFSD_LEGACY_CLIENT_TRACKING is not set, the virtual file > > > /proc/fs/nfsd/nfsv4recoverydir > > > is created but responds EINVAL to any access. > > > This is not useful, is somewhat surprising, and it causes ltp to > > > complain. > > > The only known user of this file is in nfs-utils, which handles > > > non-existence and read-failure equally well. So there is nothing to > > > gain from leaving the file present but inaccessible. > > > So this patch removes the file when its content is not available - i.e. > > > when that config option is not selected. > > > Also remove the #ifdef which hides some of the enum values when > > > CONFIG_NFSD_V$ not selection. simple_fill_super() quietly ignores array > > > entries that are not present, so having slots in the array that don't > > > get used is perfectly acceptable. So there is no value in this #ifdef. > > > Reported-by: Petr Vorel <pvorel@suse.cz> > > > Reviewed-by: Jeff Layton <jlayton@kernel.org> > > > Fixes: 74fd48739d04 ("nfsd: new Kconfig option for legacy client tracking") > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > --- > > > fs/nfsd/nfsctl.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > > > index 93c87587e646..340c5d61f199 100644 > > > --- a/fs/nfsd/nfsctl.c > > > +++ b/fs/nfsd/nfsctl.c > > > @@ -48,12 +48,10 @@ enum { > > > NFSD_MaxBlkSize, > > > NFSD_MaxConnections, > > > NFSD_Filecache, > > > -#ifdef CONFIG_NFSD_V4 > > > NFSD_Leasetime, > > > NFSD_Gracetime, > > > NFSD_RecoveryDir, > > > NFSD_V4EndGrace, > > > -#endif > > > NFSD_MaxReserved > > > }; > > > @@ -1360,7 +1358,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 > > LGTM. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Kind regards, > > Petr > > > [NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO}, > > > #endif > > > /* last one */ {""} > Thanks to you both! Applied to nfsd-next (for v6.10), unless you'd > like this to go in sooner. OK for me, I guess LTP testers can wait. Kind regards, Petr
> On Wed, Apr 17, 2024 at 07:25:16AM +0200, Petr Vorel wrote: > > Hi Neil, all, > > > When CONFIG_NFSD_LEGACY_CLIENT_TRACKING is not set, the virtual file > > > /proc/fs/nfsd/nfsv4recoverydir > > > is created but responds EINVAL to any access. > > > This is not useful, is somewhat surprising, and it causes ltp to > > > complain. > > > The only known user of this file is in nfs-utils, which handles > > > non-existence and read-failure equally well. So there is nothing to > > > gain from leaving the file present but inaccessible. > > > So this patch removes the file when its content is not available - i.e. > > > when that config option is not selected. > > > Also remove the #ifdef which hides some of the enum values when > > > CONFIG_NFSD_V$ not selection. simple_fill_super() quietly ignores array > > > entries that are not present, so having slots in the array that don't > > > get used is perfectly acceptable. So there is no value in this #ifdef. > > > Reported-by: Petr Vorel <pvorel@suse.cz> > > > Reviewed-by: Jeff Layton <jlayton@kernel.org> > > > Fixes: 74fd48739d04 ("nfsd: new Kconfig option for legacy client tracking") > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > --- > > > fs/nfsd/nfsctl.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > > > index 93c87587e646..340c5d61f199 100644 > > > --- a/fs/nfsd/nfsctl.c > > > +++ b/fs/nfsd/nfsctl.c > > > @@ -48,12 +48,10 @@ enum { > > > NFSD_MaxBlkSize, > > > NFSD_MaxConnections, > > > NFSD_Filecache, > > > -#ifdef CONFIG_NFSD_V4 > > > NFSD_Leasetime, > > > NFSD_Gracetime, > > > NFSD_RecoveryDir, > > > NFSD_V4EndGrace, > > > -#endif > > > NFSD_MaxReserved > > > }; > > > @@ -1360,7 +1358,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 > > LGTM. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Kind regards, > > Petr > > > [NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO}, > > > #endif > > > /* last one */ {""} > Thanks to you both! Applied to nfsd-next (for v6.10), unless you'd > like this to go in sooner. Also thanks to all for a very quick fix. Kind regards, Petr
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c index 93c87587e646..340c5d61f199 100644 --- a/fs/nfsd/nfsctl.c +++ b/fs/nfsd/nfsctl.c @@ -48,12 +48,10 @@ enum { NFSD_MaxBlkSize, NFSD_MaxConnections, NFSD_Filecache, -#ifdef CONFIG_NFSD_V4 NFSD_Leasetime, NFSD_Gracetime, NFSD_RecoveryDir, NFSD_V4EndGrace, -#endif NFSD_MaxReserved }; @@ -1360,7 +1358,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 */ {""}