| Message ID | 20240415172133.553441-1-pvorel@suse.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] proc01: Whitelist /proc/fs/nfsd/nfsv4recoverydir | expand |
> On Apr 15, 2024, at 1:21 PM, Petr Vorel <pvorel@suse.cz> wrote: > > /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL. > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > 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? > $ sudo cat /proc/fs/nfsd/nfsv4recoverydir > cat: /proc/fs/nfsd/nfsv4recoverydir: Invalid argument > > I'm asking because It worked fine in kernel 6.7: > > $ sudo cat /proc/fs/nfsd/nfsv4recoverydir > /var/lib/nfs/v4recovery > > I did not bisect but I suspect suspect 74fd48739d04 ("nfsd: new Kconfig > option for legacy client tracking") from v6.8-rc1. The system I test > (openSUSE Tumbleweed) has not CONFIG_NFSD_LEGACY_CLIENT_TRACKING set and > 74fd48739d04 wraps write_recoverydir setup, thus it's not set. > > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING > [NFSD_RecoveryDir] = write_recoverydir, > +#endif > > Kind regards, > Petr > > testcases/kernel/fs/proc/proc01.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/testcases/kernel/fs/proc/proc01.c b/testcases/kernel/fs/proc/proc01.c > index c90e509a3..08b9bbc75 100644 > --- a/testcases/kernel/fs/proc/proc01.c > +++ b/testcases/kernel/fs/proc/proc01.c > @@ -113,6 +113,7 @@ static const struct mapping known_issues[] = { > {"read", "/proc/fs/nfsd/filehandle", EINVAL}, > {"read", "/proc/fs/nfsd/.getfs", EINVAL}, > {"read", "/proc/fs/nfsd/.getfd", EINVAL}, > + {"read", "/proc/fs/nfsd/nfsv4recoverydir", EINVAL}, > {"read", "/proc/self/net/rpc/use-gss-proxy", EAGAIN}, > {"read", "/proc/sys/net/ipv6/conf/*/stable_secret", EIO}, > {"read", "/proc/sys/vm/nr_hugepages", EOPNOTSUPP}, > -- > 2.43.0 > > -- Chuck Lever
On Mon, 2024-04-15 at 17:27 +0000, Chuck Lever III wrote: > > > On Apr 15, 2024, at 1:21 PM, Petr Vorel <pvorel@suse.cz> wrote: > > > > /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL. > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > 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? > > > $ sudo cat /proc/fs/nfsd/nfsv4recoverydir > > cat: /proc/fs/nfsd/nfsv4recoverydir: Invalid argument > > > > I'm asking because It worked fine in kernel 6.7: > > > > $ sudo cat /proc/fs/nfsd/nfsv4recoverydir > > /var/lib/nfs/v4recovery > > > > I did not bisect but I suspect suspect 74fd48739d04 ("nfsd: new Kconfig > > option for legacy client tracking") from v6.8-rc1. The system I test > > (openSUSE Tumbleweed) has not CONFIG_NFSD_LEGACY_CLIENT_TRACKING set and > > 74fd48739d04 wraps write_recoverydir setup, thus it's not set. > > > > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING > > [NFSD_RecoveryDir] = write_recoverydir, > > +#endif > > > > Kind regards, > > Petr > > > > testcases/kernel/fs/proc/proc01.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/testcases/kernel/fs/proc/proc01.c b/testcases/kernel/fs/proc/proc01.c > > index c90e509a3..08b9bbc75 100644 > > --- a/testcases/kernel/fs/proc/proc01.c > > +++ b/testcases/kernel/fs/proc/proc01.c > > @@ -113,6 +113,7 @@ static const struct mapping known_issues[] = { > > {"read", "/proc/fs/nfsd/filehandle", EINVAL}, > > {"read", "/proc/fs/nfsd/.getfs", EINVAL}, > > {"read", "/proc/fs/nfsd/.getfd", EINVAL}, > > + {"read", "/proc/fs/nfsd/nfsv4recoverydir", EINVAL}, > > {"read", "/proc/self/net/rpc/use-gss-proxy", EAGAIN}, > > {"read", "/proc/sys/net/ipv6/conf/*/stable_secret", EIO}, > > {"read", "/proc/sys/vm/nr_hugepages", EOPNOTSUPP}, > > -- > > 2.43.0 > > > > > > -- > Chuck Lever > >
> On Apr 15, 2024, at 1:35 PM, Jeff Layton <jlayton@kernel.org> 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@suse.cz> wrote: >>> >>> /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL. >>> >>> Signed-off-by: Petr Vorel <pvorel@suse.cz> >>> --- >>> 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. -- Chuck Lever
On Mon, 2024-04-15 at 17:37 +0000, Chuck Lever III wrote: > > > On Apr 15, 2024, at 1:35 PM, Jeff Layton <jlayton@kernel.org> 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@suse.cz> wrote: > > > > > > > > /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL. > > > > > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > > --- > > > > 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?
> On Mon, 2024-04-15 at 17:37 +0000, Chuck Lever III wrote: > > > On Apr 15, 2024, at 1:35 PM, Jeff Layton <jlayton@kernel.org> 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@suse.cz> wrote: > > > > > /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL. > > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > > > --- > > > > > 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. First, thanks a lot for handling this. > Does it? Where are these rules written? I wonder myself as well. > 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? FYI current exceptions on /proc files in whole kernel have various errnos, e.g. EINVAL, EOPNOTSUPP: https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/fs/proc/proc01.c#L81 Kind regards, Petr > 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?
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@kernel.org> 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@suse.cz> wrote: > > > > > > > > > > /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL. > > > > > > > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > > > --- > > > > > 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.
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@kernel.org> 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@suse.cz> wrote: > > > > > > > > > > > > /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL. > > > > > > > > > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > > > > --- > > > > > > 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
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@kernel.org> 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@suse.cz> wrote: > > > > > > > > > > > > > > /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL. > > > > > > > > > > > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > > > > > --- > > > > > > > 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@kernel.org> > > 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.
On Tue, Apr 16, 2024 at 09:52:11AM +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@kernel.org> 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@suse.cz> wrote: > > > > > > > > > > > > > > /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL. > > > > > > > > > > > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > > > > > --- > > > > > > > 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: 'kay, no objection. Can you send an "official" patch with a description and SOB? > --- 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
Hi all,
> /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL.
Because Neil sent fix, I withdraw this patch.
Kind regards,
Petr
[1] https://lore.kernel.org/linux-nfs/171330258224.17212.9790424282163530018@noble.neil.brown.name/
diff --git a/testcases/kernel/fs/proc/proc01.c b/testcases/kernel/fs/proc/proc01.c index c90e509a3..08b9bbc75 100644 --- a/testcases/kernel/fs/proc/proc01.c +++ b/testcases/kernel/fs/proc/proc01.c @@ -113,6 +113,7 @@ static const struct mapping known_issues[] = { {"read", "/proc/fs/nfsd/filehandle", EINVAL}, {"read", "/proc/fs/nfsd/.getfs", EINVAL}, {"read", "/proc/fs/nfsd/.getfd", EINVAL}, + {"read", "/proc/fs/nfsd/nfsv4recoverydir", EINVAL}, {"read", "/proc/self/net/rpc/use-gss-proxy", EAGAIN}, {"read", "/proc/sys/net/ipv6/conf/*/stable_secret", EIO}, {"read", "/proc/sys/vm/nr_hugepages", EOPNOTSUPP},
/proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL. Signed-off-by: Petr Vorel <pvorel@suse.cz> --- 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: $ sudo cat /proc/fs/nfsd/nfsv4recoverydir cat: /proc/fs/nfsd/nfsv4recoverydir: Invalid argument I'm asking because It worked fine in kernel 6.7: $ sudo cat /proc/fs/nfsd/nfsv4recoverydir /var/lib/nfs/v4recovery I did not bisect but I suspect suspect 74fd48739d04 ("nfsd: new Kconfig option for legacy client tracking") from v6.8-rc1. The system I test (openSUSE Tumbleweed) has not CONFIG_NFSD_LEGACY_CLIENT_TRACKING set and 74fd48739d04 wraps write_recoverydir setup, thus it's not set. +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING [NFSD_RecoveryDir] = write_recoverydir, +#endif Kind regards, Petr testcases/kernel/fs/proc/proc01.c | 1 + 1 file changed, 1 insertion(+)