If an NFSv4.1 server doesn't support NFS4_OPEN_CLAIM_DELEG_CUR_FH
(e.g. Linux 3.0), and a newer NFS client tries to use it to claim
an open before returning a delegation, the server might return
NFS4ERR_BADXDR.
That is what Linux 3.0 does, though the RFC doesn't seem to be explicit
on which flags must be supported, and what error can be returned for
unsupported flags.
When NFS_CAP_ATOMIC_OPEN_V1 support was added in Commit 49f9a0fafd84
("NFSv4.1: Enable open-by-filehandle"), fall-back for non-supporting
servers was added for various open types, but not for delegation recall.
The code pattern for delegation recall is a little different to the
other open types, so I cannot simply copy the same approach.
I think the below patch should do the right thing, but I haven't tested
yet.
Does this look reasonable? Is there a cleaner way to do it? Should
we check other errors?
Thanks,
NeilBrown
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index caacf5e7f5e1..14f958d16648 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2174,6 +2174,13 @@ static int nfs4_open_reclaim(struct nfs4_state_owner *sp, struct nfs4_state *sta
static int nfs4_handle_delegation_recall_error(struct nfs_server *server, struct nfs4_state *state, const nfs4_stateid *stateid, struct file_lock *fl, int err)
{
switch (err) {
+ case -NFS4ERR_BADXDR: {
+ struct nfs4_exception exception;
+ if (nfs4_clear_cap_atomic_open_v1(server, -EINVAL,
+ &exception))
+ return -EAGAIN;
+ }
+ /* fallthrough */
default:
printk(KERN_ERR "NFS: %s: unhandled error "
"%d.\n", __func__, err);
On Thu, 2019-12-19 at 09:47 +1100, NeilBrown wrote:
> If an NFSv4.1 server doesn't support NFS4_OPEN_CLAIM_DELEG_CUR_FH
> (e.g. Linux 3.0), and a newer NFS client tries to use it to claim
> an open before returning a delegation, the server might return
> NFS4ERR_BADXDR.
> That is what Linux 3.0 does, though the RFC doesn't seem to be
> explicit
> on which flags must be supported, and what error can be returned for
> unsupported flags.
NFS4ERR_BADXDR is defined in RFC5661, section 15.1.1.1 as meaning
"The arguments for this operation do not match those specified in the
XDR definition."
That's clearly not the case here, so I'd chalk this down to a fairly
blatant server bug, at which point it makes no sense to fix it in the
client.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Wed, Dec 18 2019, Trond Myklebust wrote:
> On Thu, 2019-12-19 at 09:47 +1100, NeilBrown wrote:
>> If an NFSv4.1 server doesn't support NFS4_OPEN_CLAIM_DELEG_CUR_FH
>> (e.g. Linux 3.0), and a newer NFS client tries to use it to claim
>> an open before returning a delegation, the server might return
>> NFS4ERR_BADXDR.
>> That is what Linux 3.0 does, though the RFC doesn't seem to be
>> explicit
>> on which flags must be supported, and what error can be returned for
>> unsupported flags.
>
> NFS4ERR_BADXDR is defined in RFC5661, section 15.1.1.1 as meaning
>
> "The arguments for this operation do not match those specified in the
> XDR definition."
>
> That's clearly not the case here, so I'd chalk this down to a fairly
> blatant server bug, at which point it makes no sense to fix it in the
> client.
Ok, but the RFC seems to suggest it is OK to not support this flag, so
suppose I fixed the server to return NFS4ERR_NOTSUPP instead.
The client still wouldn't handle this response gracefully.
Thanks,
NeilBrown
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index caacf5e7f5e1..14f958d16648 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2174,6 +2174,13 @@ static int nfs4_open_reclaim(struct nfs4_state_owner *sp, struct nfs4_state *sta
static int nfs4_handle_delegation_recall_error(struct nfs_server *server, struct nfs4_state *state, const
nfs4_stateid *stateid, struct file_lock *fl, int err)
{
switch (err) {
+ case -NFS4ERR_NOTSUPP: {
+ struct nfs4_exception exception;
+ if (nfs4_clear_cap_atomic_open_v1(server, -EINVAL,
+ &exception))
+ return -EAGAIN;
+ }
+ /* fallthrough */
default:
printk(KERN_ERR "NFS: %s: unhandled error "
"%d.\n", __func__, err);
On Thu, 2019-12-19 at 13:56 +1100, NeilBrown wrote:
> On Wed, Dec 18 2019, Trond Myklebust wrote:
>
> > On Thu, 2019-12-19 at 09:47 +1100, NeilBrown wrote:
> > > If an NFSv4.1 server doesn't support NFS4_OPEN_CLAIM_DELEG_CUR_FH
> > > (e.g. Linux 3.0), and a newer NFS client tries to use it to claim
> > > an open before returning a delegation, the server might return
> > > NFS4ERR_BADXDR.
> > > That is what Linux 3.0 does, though the RFC doesn't seem to be
> > > explicit
> > > on which flags must be supported, and what error can be returned
> > > for
> > > unsupported flags.
> >
> > NFS4ERR_BADXDR is defined in RFC5661, section 15.1.1.1 as meaning
> >
> > "The arguments for this operation do not match those specified in
> > the
> > XDR definition."
> >
> > That's clearly not the case here, so I'd chalk this down to a
> > fairly
> > blatant server bug, at which point it makes no sense to fix it in
> > the
> > client.
>
> Ok, but the RFC seems to suggest it is OK to not support this flag,
> so
> suppose I fixed the server to return NFS4ERR_NOTSUPP instead.
> The client still wouldn't handle this response gracefully.
>
NFS4ERR_NOTSUPP is wrong too as the OPEN operation is clearly
supported. The only error that might make sense is NFS4ERR_INVAL:
"15.1.1.4. NFS4ERR_INVAL (Error Code 22)
The arguments for this operation are not valid for some reason, even
though they do match those specified in the XDR definition for the
request."
That said, why do we care about supporting NFSv4.1 on this server? It
is clearly broken.
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index caacf5e7f5e1..14f958d16648 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2174,6 +2174,13 @@ static int nfs4_open_reclaim(struct
> nfs4_state_owner *sp, struct nfs4_state *sta
> static int nfs4_handle_delegation_recall_error(struct nfs_server
> *server, struct nfs4_state *state, const
> nfs4_stateid *stateid, struct file_lock *fl, int err)
> {
> switch (err) {
> + case -NFS4ERR_NOTSUPP: {
> + struct nfs4_exception exception;
> + if (nfs4_clear_cap_atomic_open_v1(server,
> -EINVAL,
> + &exception))
> + return -EAGAIN;
> + }
> + /* fallthrough */
> default:
> printk(KERN_ERR "NFS: %s: unhandled error "
> "%d.\n", __func__, err);
>
--
Trond Myklebust
CTO, Hammerspace Inc
4300 El Camino Real, Suite 105
Los Altos, CA 94022
http://www.hammer.space
On Thu, Dec 19 2019, Trond Myklebust wrote:
> On Thu, 2019-12-19 at 13:56 +1100, NeilBrown wrote:
>> On Wed, Dec 18 2019, Trond Myklebust wrote:
>>
>> > On Thu, 2019-12-19 at 09:47 +1100, NeilBrown wrote:
>> > > If an NFSv4.1 server doesn't support NFS4_OPEN_CLAIM_DELEG_CUR_FH
>> > > (e.g. Linux 3.0), and a newer NFS client tries to use it to claim
>> > > an open before returning a delegation, the server might return
>> > > NFS4ERR_BADXDR.
>> > > That is what Linux 3.0 does, though the RFC doesn't seem to be
>> > > explicit
>> > > on which flags must be supported, and what error can be returned
>> > > for
>> > > unsupported flags.
>> >
>> > NFS4ERR_BADXDR is defined in RFC5661, section 15.1.1.1 as meaning
>> >
>> > "The arguments for this operation do not match those specified in
>> > the
>> > XDR definition."
>> >
>> > That's clearly not the case here, so I'd chalk this down to a
>> > fairly
>> > blatant server bug, at which point it makes no sense to fix it in
>> > the
>> > client.
>>
>> Ok, but the RFC seems to suggest it is OK to not support this flag,
>> so
>> suppose I fixed the server to return NFS4ERR_NOTSUPP instead.
>> The client still wouldn't handle this response gracefully.
>>
>
> NFS4ERR_NOTSUPP is wrong too as the OPEN operation is clearly
> supported. The only error that might make sense is NFS4ERR_INVAL:
>
> "15.1.1.4. NFS4ERR_INVAL (Error Code 22)
>
> The arguments for this operation are not valid for some reason, even
> though they do match those specified in the XDR definition for the
> request."
>
> That said, why do we care about supporting NFSv4.1 on this server? It
> is clearly broken.
I care about it because a customer has a support contract, but that
isn't your problem.
I would think "we" care about it because we want to support the spec,
and the spec (RFC 5661 section 2.4) says:
where the server
supports neither the CLAIM_DELEGATE_PREV nor CLAIM_DELEG_CUR_FH claim
types
Also you have code in the client to handle the possibility that an
NFSv4.1 or later server might not handle some features of OPEN.
Three separate features are grouped under "NFS_CAP_ATOMIC_OPEN_V1":
If this isn't set, we fall back:
case NFS4_OPEN_CLAIM_FH:
return NFS4_OPEN_CLAIM_NULL;
case NFS4_OPEN_CLAIM_DELEG_CUR_FH:
return NFS4_OPEN_CLAIM_DELEGATE_CUR;
case NFS4_OPEN_CLAIM_DELEG_PREV_FH:
return NFS4_OPEN_CLAIM_DELEGATE_PREV;
However nfs4_map_atomic_open_claim() is not called when
NFS4_OPEN_CLAIM_DELEG_CUR_FH is tried, and fails. This appears
to be an omission in the code.
Thanks,
NeilBrown
On Thu, 2019-12-19 at 16:39 +1100, NeilBrown wrote:
> On Thu, Dec 19 2019, Trond Myklebust wrote:
>
> > On Thu, 2019-12-19 at 13:56 +1100, NeilBrown wrote:
> > > On Wed, Dec 18 2019, Trond Myklebust wrote:
> > >
> > > > On Thu, 2019-12-19 at 09:47 +1100, NeilBrown wrote:
> > > > > If an NFSv4.1 server doesn't support
> > > > > NFS4_OPEN_CLAIM_DELEG_CUR_FH
> > > > > (e.g. Linux 3.0), and a newer NFS client tries to use it to
> > > > > claim
> > > > > an open before returning a delegation, the server might
> > > > > return
> > > > > NFS4ERR_BADXDR.
> > > > > That is what Linux 3.0 does, though the RFC doesn't seem to
> > > > > be
> > > > > explicit
> > > > > on which flags must be supported, and what error can be
> > > > > returned
> > > > > for
> > > > > unsupported flags.
> > > >
> > > > NFS4ERR_BADXDR is defined in RFC5661, section 15.1.1.1 as
> > > > meaning
> > > >
> > > > "The arguments for this operation do not match those specified
> > > > in
> > > > the
> > > > XDR definition."
> > > >
> > > > That's clearly not the case here, so I'd chalk this down to a
> > > > fairly
> > > > blatant server bug, at which point it makes no sense to fix it
> > > > in
> > > > the
> > > > client.
> > >
> > > Ok, but the RFC seems to suggest it is OK to not support this
> > > flag,
> > > so
> > > suppose I fixed the server to return NFS4ERR_NOTSUPP instead.
> > > The client still wouldn't handle this response gracefully.
> > >
> >
> > NFS4ERR_NOTSUPP is wrong too as the OPEN operation is clearly
> > supported. The only error that might make sense is NFS4ERR_INVAL:
> >
> > "15.1.1.4. NFS4ERR_INVAL (Error Code 22)
> >
> > The arguments for this operation are not valid for some reason,
> > even
> > though they do match those specified in the XDR definition for
> > the
> > request."
> >
> > That said, why do we care about supporting NFSv4.1 on this server?
> > It
> > is clearly broken.
>
> I care about it because a customer has a support contract, but that
> isn't your problem.
>
> I would think "we" care about it because we want to support the spec,
> and the spec (RFC 5661 section 2.4) says:
>
> where the
> server
> supports neither the CLAIM_DELEGATE_PREV nor CLAIM_DELEG_CUR_FH
> claim
> types
Given the context, I think that is actually a typo. It looks to me like
it is talking about CLAIM_DELEGATE_PREV and CLAIM_DELEG_PREV_FH, since
otherwise the talk about releasing delegation state when establishing a
new lease makes no sense.
> Also you have code in the client to handle the possibility that an
> NFSv4.1 or later server might not handle some features of OPEN.
> Three separate features are grouped under "NFS_CAP_ATOMIC_OPEN_V1":
> If this isn't set, we fall back:
>
> case NFS4_OPEN_CLAIM_FH:
> return NFS4_OPEN_CLAIM_NULL;
> case NFS4_OPEN_CLAIM_DELEG_CUR_FH:
> return NFS4_OPEN_CLAIM_DELEGATE_CUR;
> case NFS4_OPEN_CLAIM_DELEG_PREV_FH:
> return NFS4_OPEN_CLAIM_DELEGATE_PREV;
>
Right. That's a convenience for downgrading NFSv4.1 service to what is
supported by NFSv4.0.
> However nfs4_map_atomic_open_claim() is not called when
> NFS4_OPEN_CLAIM_DELEG_CUR_FH is tried, and fails. This appears
> to be an omission in the code.
>
It is deliberate. There really isn't anything that describes what is
and isn't mandatory to implement in NFSv4.1, but if we have to make
everything optional, then we're going to have to add a lot of mostly
unnecessary complexity to the client.
At what point do we then stop? Do we support a NFSv4.1 server that
implements no NFSv4.1 features? Why not just let the client downgrade
to NFSv4.0 in that case?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Thu, Dec 19 2019, Trond Myklebust wrote:
> On Thu, 2019-12-19 at 16:39 +1100, NeilBrown wrote:
>> On Thu, Dec 19 2019, Trond Myklebust wrote:
>>
>> > On Thu, 2019-12-19 at 13:56 +1100, NeilBrown wrote:
>> > > On Wed, Dec 18 2019, Trond Myklebust wrote:
>> > >
>> > > > On Thu, 2019-12-19 at 09:47 +1100, NeilBrown wrote:
>> > > > > If an NFSv4.1 server doesn't support
>> > > > > NFS4_OPEN_CLAIM_DELEG_CUR_FH
>> > > > > (e.g. Linux 3.0), and a newer NFS client tries to use it to
>> > > > > claim
>> > > > > an open before returning a delegation, the server might
>> > > > > return
>> > > > > NFS4ERR_BADXDR.
>> > > > > That is what Linux 3.0 does, though the RFC doesn't seem to
>> > > > > be
>> > > > > explicit
>> > > > > on which flags must be supported, and what error can be
>> > > > > returned
>> > > > > for
>> > > > > unsupported flags.
>> > > >
>> > > > NFS4ERR_BADXDR is defined in RFC5661, section 15.1.1.1 as
>> > > > meaning
>> > > >
>> > > > "The arguments for this operation do not match those specified
>> > > > in
>> > > > the
>> > > > XDR definition."
>> > > >
>> > > > That's clearly not the case here, so I'd chalk this down to a
>> > > > fairly
>> > > > blatant server bug, at which point it makes no sense to fix it
>> > > > in
>> > > > the
>> > > > client.
>> > >
>> > > Ok, but the RFC seems to suggest it is OK to not support this
>> > > flag,
>> > > so
>> > > suppose I fixed the server to return NFS4ERR_NOTSUPP instead.
>> > > The client still wouldn't handle this response gracefully.
>> > >
>> >
>> > NFS4ERR_NOTSUPP is wrong too as the OPEN operation is clearly
>> > supported. The only error that might make sense is NFS4ERR_INVAL:
>> >
>> > "15.1.1.4. NFS4ERR_INVAL (Error Code 22)
>> >
>> > The arguments for this operation are not valid for some reason,
>> > even
>> > though they do match those specified in the XDR definition for
>> > the
>> > request."
>> >
>> > That said, why do we care about supporting NFSv4.1 on this server?
>> > It
>> > is clearly broken.
>>
>> I care about it because a customer has a support contract, but that
>> isn't your problem.
>>
>> I would think "we" care about it because we want to support the spec,
>> and the spec (RFC 5661 section 2.4) says:
>>
>> where the
>> server
>> supports neither the CLAIM_DELEGATE_PREV nor CLAIM_DELEG_CUR_FH
>> claim
>> types
>
> Given the context, I think that is actually a typo. It looks to me like
> it is talking about CLAIM_DELEGATE_PREV and CLAIM_DELEG_PREV_FH, since
> otherwise the talk about releasing delegation state when establishing a
> new lease makes no sense.
Hmmm.. Yes, that's believable.
>
>
>> Also you have code in the client to handle the possibility that an
>> NFSv4.1 or later server might not handle some features of OPEN.
>> Three separate features are grouped under "NFS_CAP_ATOMIC_OPEN_V1":
>> If this isn't set, we fall back:
>>
>> case NFS4_OPEN_CLAIM_FH:
>> return NFS4_OPEN_CLAIM_NULL;
>> case NFS4_OPEN_CLAIM_DELEG_CUR_FH:
>> return NFS4_OPEN_CLAIM_DELEGATE_CUR;
>> case NFS4_OPEN_CLAIM_DELEG_PREV_FH:
>> return NFS4_OPEN_CLAIM_DELEGATE_PREV;
>>
>
> Right. That's a convenience for downgrading NFSv4.1 service to what is
> supported by NFSv4.0.
>
>> However nfs4_map_atomic_open_claim() is not called when
>> NFS4_OPEN_CLAIM_DELEG_CUR_FH is tried, and fails. This appears
>> to be an omission in the code.
>>
>
> It is deliberate. There really isn't anything that describes what is
> and isn't mandatory to implement in NFSv4.1, but if we have to make
> everything optional, then we're going to have to add a lot of mostly
> unnecessary complexity to the client.
> At what point do we then stop? Do we support a NFSv4.1 server that
> implements no NFSv4.1 features? Why not just let the client downgrade
> to NFSv4.0 in that case?
I was a bit surprised that nfs4_map_atomic_open_claim() exists at all,
but given that it did, I assumed it would be used more uniformly.
So this all implies that Linux NFS server claimed to support NFSv4.1
before it actually did - which seems odd. This is just a bug (which are
expected), but a clear ommission.
Oh well, it probably won't be too hard to backport the
NFS4_OPEN_CLAIM_DELEG_CUR_FH support if it turns out to be really
needed.
Thanks a lot for your time,
NeilBrown
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> [email protected]
On Fri, Dec 20, 2019 at 04:19:56PM +1100, NeilBrown wrote:
> I was a bit surprised that nfs4_map_atomic_open_claim() exists at all,
> but given that it did, I assumed it would be used more uniformly.
>
> So this all implies that Linux NFS server claimed to support NFSv4.1
> before it actually did - which seems odd. This is just a bug (which are
> expected), but a clear ommission.
For what it's worth, I did make some attempt to keep 4.1 by default
until 3.11 (see d109148111cd "nfsd4: support minorversion 1 by default")
but probably could have communicated that better. This isn't the only
blatant known issue in older code.
--b.
On Tue, Jan 07, 2020 at 11:15:36AM -0500, bfields wrote:
> On Fri, Dec 20, 2019 at 04:19:56PM +1100, NeilBrown wrote:
> > I was a bit surprised that nfs4_map_atomic_open_claim() exists at all,
> > but given that it did, I assumed it would be used more uniformly.
> >
> > So this all implies that Linux NFS server claimed to support NFSv4.1
> > before it actually did - which seems odd. This is just a bug (which are
> > expected), but a clear ommission.
>
> For what it's worth, I did make some attempt to keep 4.1 by default
(I meant to say: "keep 4.1 off by default".)
> until 3.11 (see d109148111cd "nfsd4: support minorversion 1 by default")
> but probably could have communicated that better. This isn't the only
> blatant known issue in older code.
>
> --b.
On Tue, Jan 07 2020, J. Bruce Fields wrote:
> On Fri, Dec 20, 2019 at 04:19:56PM +1100, NeilBrown wrote:
>> I was a bit surprised that nfs4_map_atomic_open_claim() exists at all,
>> but given that it did, I assumed it would be used more uniformly.
>>
>> So this all implies that Linux NFS server claimed to support NFSv4.1
>> before it actually did - which seems odd. This is just a bug (which are
>> expected), but a clear ommission.
>
> For what it's worth, I did make some attempt to keep 4.1 by default
> until 3.11 (see d109148111cd "nfsd4: support minorversion 1 by default")
> but probably could have communicated that better. This isn't the only
> blatant known issue in older code.
Ahh... thanks for that.
Looking more deeply, I see that we (SUSE) left it that way, but there is
a sysconfig option to explicitly enable NFSv4.1 service, and the
customer has explicitly enabled that.
So it is sort-of there fault.
Maybe we shouldn't have given them the option.
Anyway, it is all clear now. Thanks.
NeilBrown