| Message ID | 20240920160551.52759-1-okorniev@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] nfsd: fix handling of WANT_DELEG_TIMESTAMPS on open | expand |
On Fri, 2024-09-20 at 12:05 -0400, Olga Kornievskaia wrote: > Current, the server returns that it supports NFS4_SHARE_WANT_DELEG_TIMESTAMPS > but when the client sends that on the open, knfsd returns back with > bad_xdr error. > > Fixes: d0eab73d48a0 ("nfsd: add support for delegated timestamps") > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > --- > fs/nfsd/nfs4xdr.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index c0bad580ab6d..adda8b489175 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -1109,6 +1109,7 @@ static __be32 nfsd4_decode_share_access(struct nfsd4_compoundargs *argp, u32 *sh > case NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED: > case (NFS4_SHARE_SIGNAL_DELEG_WHEN_RESRC_AVAIL | > NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED): > + case NFS4_SHARE_WANT_DELEG_TIMESTAMPS: > return nfs_ok; > } > return nfserr_bad_xdr; Ouch. The problem here is that we had to drop the patch that added OPEN_XOR_DELEGATION support. That patch reworked the flag handling in this function in a way that allowed the new delstid flags to be properly supported. I think we probably want to resurrect the parts of this patch that alter nfsd4_decode_share_access: https://lore.kernel.org/linux-nfs/20240905-delstid-v4-8-d3e5fd34d107@kernel.org/ Olga, would you be OK with that approach instead?
> On Sep 20, 2024, at 12:24 PM, Jeff Layton <jlayton@kernel.org> wrote: > > On Fri, 2024-09-20 at 12:05 -0400, Olga Kornievskaia wrote: >> Current, the server returns that it supports NFS4_SHARE_WANT_DELEG_TIMESTAMPS >> but when the client sends that on the open, knfsd returns back with >> bad_xdr error. >> >> Fixes: d0eab73d48a0 ("nfsd: add support for delegated timestamps") >> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> >> --- >> fs/nfsd/nfs4xdr.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c >> index c0bad580ab6d..adda8b489175 100644 >> --- a/fs/nfsd/nfs4xdr.c >> +++ b/fs/nfsd/nfs4xdr.c >> @@ -1109,6 +1109,7 @@ static __be32 nfsd4_decode_share_access(struct nfsd4_compoundargs *argp, u32 *sh >> case NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED: >> case (NFS4_SHARE_SIGNAL_DELEG_WHEN_RESRC_AVAIL | >> NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED): >> + case NFS4_SHARE_WANT_DELEG_TIMESTAMPS: >> return nfs_ok; >> } >> return nfserr_bad_xdr; > > Ouch. > > The problem here is that we had to drop the patch that added > OPEN_XOR_DELEGATION support. That patch reworked the flag handling in > this function in a way that allowed the new delstid flags to be > properly supported. > > I think we probably want to resurrect the parts of this patch that > alter nfsd4_decode_share_access: > > https://lore.kernel.org/linux-nfs/20240905-delstid-v4-8-d3e5fd34d107@kernel.org/ > > Olga, would you be OK with that approach instead? At this point, I'd like to consider postponing the delstid patches until v6.13. They haven't gotten enough testing in their current form... -- Chuck Lever
On Fri, 2024-09-20 at 16:30 +0000, Chuck Lever III wrote: > > > On Sep 20, 2024, at 12:24 PM, Jeff Layton <jlayton@kernel.org> wrote: > > > > On Fri, 2024-09-20 at 12:05 -0400, Olga Kornievskaia wrote: > > > Current, the server returns that it supports NFS4_SHARE_WANT_DELEG_TIMESTAMPS > > > but when the client sends that on the open, knfsd returns back with > > > bad_xdr error. > > > > > > Fixes: d0eab73d48a0 ("nfsd: add support for delegated timestamps") > > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > > > --- > > > fs/nfsd/nfs4xdr.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > > index c0bad580ab6d..adda8b489175 100644 > > > --- a/fs/nfsd/nfs4xdr.c > > > +++ b/fs/nfsd/nfs4xdr.c > > > @@ -1109,6 +1109,7 @@ static __be32 nfsd4_decode_share_access(struct nfsd4_compoundargs *argp, u32 *sh > > > case NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED: > > > case (NFS4_SHARE_SIGNAL_DELEG_WHEN_RESRC_AVAIL | > > > NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED): > > > + case NFS4_SHARE_WANT_DELEG_TIMESTAMPS: > > > return nfs_ok; > > > } > > > return nfserr_bad_xdr; > > > > Ouch. > > > > The problem here is that we had to drop the patch that added > > OPEN_XOR_DELEGATION support. That patch reworked the flag handling in > > this function in a way that allowed the new delstid flags to be > > properly supported. > > > > I think we probably want to resurrect the parts of this patch that > > alter nfsd4_decode_share_access: > > > > https://lore.kernel.org/linux-nfs/20240905-delstid-v4-8-d3e5fd34d107@kernel.org/ > > > > Olga, would you be OK with that approach instead? > > At this point, I'd like to consider postponing the delstid patches > until v6.13. They haven't gotten enough testing in their current > form... > It's your call, but that seems like an extreme reaction to a flag handling fix, given that we have several weeks of -rc's ahead of us.
On Fri, Sep 20, 2024 at 12:34 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Fri, 2024-09-20 at 16:30 +0000, Chuck Lever III wrote: > > > > > On Sep 20, 2024, at 12:24 PM, Jeff Layton <jlayton@kernel.org> wrote: > > > > > > On Fri, 2024-09-20 at 12:05 -0400, Olga Kornievskaia wrote: > > > > Current, the server returns that it supports NFS4_SHARE_WANT_DELEG_TIMESTAMPS > > > > but when the client sends that on the open, knfsd returns back with > > > > bad_xdr error. > > > > > > > > Fixes: d0eab73d48a0 ("nfsd: add support for delegated timestamps") > > > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > > > > --- > > > > fs/nfsd/nfs4xdr.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > > > index c0bad580ab6d..adda8b489175 100644 > > > > --- a/fs/nfsd/nfs4xdr.c > > > > +++ b/fs/nfsd/nfs4xdr.c > > > > @@ -1109,6 +1109,7 @@ static __be32 nfsd4_decode_share_access(struct nfsd4_compoundargs *argp, u32 *sh > > > > case NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED: > > > > case (NFS4_SHARE_SIGNAL_DELEG_WHEN_RESRC_AVAIL | > > > > NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED): > > > > + case NFS4_SHARE_WANT_DELEG_TIMESTAMPS: > > > > return nfs_ok; > > > > } > > > > return nfserr_bad_xdr; > > > > > > Ouch. > > > > > > The problem here is that we had to drop the patch that added > > > OPEN_XOR_DELEGATION support. That patch reworked the flag handling in > > > this function in a way that allowed the new delstid flags to be > > > properly supported. > > > > > > I think we probably want to resurrect the parts of this patch that > > > alter nfsd4_decode_share_access: > > > > > > https://lore.kernel.org/linux-nfs/20240905-delstid-v4-8-d3e5fd34d107@kernel.org/ > > > > > > Olga, would you be OK with that approach instead? > > > > At this point, I'd like to consider postponing the delstid patches > > until v6.13. They haven't gotten enough testing in their current > > form... > > > > It's your call, but that seems like an extreme reaction to a flag > handling fix, given that we have several weeks of -rc's ahead of us. I'm OK with whatever fix/approach it's going to go (Jeff if you have a patch to test that would go on top of what's in Chuck's nfsd-next and fixes the issue do post it) . I'm not ok with the code that's at the moment in nfsd-next :). > -- > Jeff Layton <jlayton@kernel.org> >
> On Sep 20, 2024, at 12:34 PM, Jeff Layton <jlayton@kernel.org> wrote: > > On Fri, 2024-09-20 at 16:30 +0000, Chuck Lever III wrote: >> >>> On Sep 20, 2024, at 12:24 PM, Jeff Layton <jlayton@kernel.org> wrote: >>> >>> On Fri, 2024-09-20 at 12:05 -0400, Olga Kornievskaia wrote: >>>> Current, the server returns that it supports NFS4_SHARE_WANT_DELEG_TIMESTAMPS >>>> but when the client sends that on the open, knfsd returns back with >>>> bad_xdr error. >>>> >>>> Fixes: d0eab73d48a0 ("nfsd: add support for delegated timestamps") >>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> >>>> --- >>>> fs/nfsd/nfs4xdr.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c >>>> index c0bad580ab6d..adda8b489175 100644 >>>> --- a/fs/nfsd/nfs4xdr.c >>>> +++ b/fs/nfsd/nfs4xdr.c >>>> @@ -1109,6 +1109,7 @@ static __be32 nfsd4_decode_share_access(struct nfsd4_compoundargs *argp, u32 *sh >>>> case NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED: >>>> case (NFS4_SHARE_SIGNAL_DELEG_WHEN_RESRC_AVAIL | >>>> NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED): >>>> + case NFS4_SHARE_WANT_DELEG_TIMESTAMPS: >>>> return nfs_ok; >>>> } >>>> return nfserr_bad_xdr; >>> >>> Ouch. >>> >>> The problem here is that we had to drop the patch that added >>> OPEN_XOR_DELEGATION support. That patch reworked the flag handling in >>> this function in a way that allowed the new delstid flags to be >>> properly supported. >>> >>> I think we probably want to resurrect the parts of this patch that >>> alter nfsd4_decode_share_access: >>> >>> https://lore.kernel.org/linux-nfs/20240905-delstid-v4-8-d3e5fd34d107@kernel.org/ >>> >>> Olga, would you be OK with that approach instead? >> >> At this point, I'd like to consider postponing the delstid patches >> until v6.13. They haven't gotten enough testing in their current >> form... > > It's your call, but that seems like an extreme reaction to a flag > handling fix, given that we have several weeks of -rc's ahead of us. It's not just this small problem that worries me. I pulled in v4 of the delstid patches pretty much at the last moment for v6.12 development. So far, there's been an 80% performance regression, and the surgery to work around that issue introduced another bug that invalidates all NFSv4.2 test results since that surgery, just a week ago. The delstid patches impact an already heavily-used code path (OPEN). You've already found one or two bugs in the client side as well. This isn't something the can be disabled by distros until it is ready -- it's baked into a popular code path as soon as it goes upstream, and NFSv4.2 is the current default. So I can't say with confidence that this code is stable and ready to merge. If we had another week or two before v6.11 final, there wouldn't be a question, but it's already a week into the v6.12 merge window, and I need to submit a pull request in the next few days. A 6.11-rc8 would have been extremely helpful, but we didn't get one. Exposing this change set to the cauldron that is bake-a-thon would be valuable, IMO, for both the client and the server. -- Chuck Lever
On Fri, 2024-09-20 at 18:18 +0000, Chuck Lever III wrote: > > > On Sep 20, 2024, at 12:34 PM, Jeff Layton <jlayton@kernel.org> wrote: > > > > On Fri, 2024-09-20 at 16:30 +0000, Chuck Lever III wrote: > > > > > > > On Sep 20, 2024, at 12:24 PM, Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > > On Fri, 2024-09-20 at 12:05 -0400, Olga Kornievskaia wrote: > > > > > Current, the server returns that it supports NFS4_SHARE_WANT_DELEG_TIMESTAMPS > > > > > but when the client sends that on the open, knfsd returns back with > > > > > bad_xdr error. > > > > > > > > > > Fixes: d0eab73d48a0 ("nfsd: add support for delegated timestamps") > > > > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > > > > > --- > > > > > fs/nfsd/nfs4xdr.c | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > > > > index c0bad580ab6d..adda8b489175 100644 > > > > > --- a/fs/nfsd/nfs4xdr.c > > > > > +++ b/fs/nfsd/nfs4xdr.c > > > > > @@ -1109,6 +1109,7 @@ static __be32 nfsd4_decode_share_access(struct nfsd4_compoundargs *argp, u32 *sh > > > > > case NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED: > > > > > case (NFS4_SHARE_SIGNAL_DELEG_WHEN_RESRC_AVAIL | > > > > > NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED): > > > > > + case NFS4_SHARE_WANT_DELEG_TIMESTAMPS: > > > > > return nfs_ok; > > > > > } > > > > > return nfserr_bad_xdr; > > > > > > > > Ouch. > > > > > > > > The problem here is that we had to drop the patch that added > > > > OPEN_XOR_DELEGATION support. That patch reworked the flag handling in > > > > this function in a way that allowed the new delstid flags to be > > > > properly supported. > > > > > > > > I think we probably want to resurrect the parts of this patch that > > > > alter nfsd4_decode_share_access: > > > > > > > > https://lore.kernel.org/linux-nfs/20240905-delstid-v4-8-d3e5fd34d107@kernel.org/ > > > > > > > > Olga, would you be OK with that approach instead? > > > > > > At this point, I'd like to consider postponing the delstid patches > > > until v6.13. They haven't gotten enough testing in their current > > > form... > > > > It's your call, but that seems like an extreme reaction to a flag > > handling fix, given that we have several weeks of -rc's ahead of us. > > It's not just this small problem that worries me. > > I pulled in v4 of the delstid patches pretty much at the last > moment for v6.12 development. So far, there's been an 80% > performance regression, and the surgery to work around that > issue introduced another bug that invalidates all NFSv4.2 test > results since that surgery, just a week ago. > > The delstid patches impact an already heavily-used code path > (OPEN). You've already found one or two bugs in the client > side as well. This isn't something the can be disabled by > distros until it is ready -- it's baked into a popular code > path as soon as it goes upstream, and NFSv4.2 is the current > default. > > So I can't say with confidence that this code is stable and > ready to merge. If we had another week or two before v6.11 > final, there wouldn't be a question, but it's already a week > into the v6.12 merge window, and I need to submit a pull > request in the next few days. A 6.11-rc8 would have been > extremely helpful, but we didn't get one. > > Exposing this change set to the cauldron that is bake-a-thon > would be valuable, IMO, for both the client and the server. > Fair enough. These are not features that anyone is particularly clamoring for, so dropping them for now is fine. I'll plan to spin up a new set once I'm back from travel (another week or so).
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index c0bad580ab6d..adda8b489175 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -1109,6 +1109,7 @@ static __be32 nfsd4_decode_share_access(struct nfsd4_compoundargs *argp, u32 *sh case NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED: case (NFS4_SHARE_SIGNAL_DELEG_WHEN_RESRC_AVAIL | NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED): + case NFS4_SHARE_WANT_DELEG_TIMESTAMPS: return nfs_ok; } return nfserr_bad_xdr;
Current, the server returns that it supports NFS4_SHARE_WANT_DELEG_TIMESTAMPS but when the client sends that on the open, knfsd returns back with bad_xdr error. Fixes: d0eab73d48a0 ("nfsd: add support for delegated timestamps") Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> --- fs/nfsd/nfs4xdr.c | 1 + 1 file changed, 1 insertion(+)