Re: [PATCH/RFC] NFS: handle NFSv4.1 server that doesn't support NFS4_OPEN_CLAIM_DELEG_CUR_FH

[Trond Myklebust <trondmy@hammerspace.com>]

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
trond.myklebust@hammerspace.com