Re: [PATCH - v2] NFSv4: handle EINVAL from EXCHANGE_ID better.

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 3481 bytes --]

On Fri, Mar 16 2018, Mkrtchyan, Tigran wrote:

> Hi Neil,
>
> according to rfc5661, NFS4ERR_INVAL is returned by the server if it
> thinks that client sends an invalid request (e.g. points to a client bug)
> or server misinterpret it (broken server).

Agreed.

>
> With your change instead of failing the mount, client will silently go for
> v4.0, even v4.1 mount was requested and produce undesirable behavior, e.g.
> proxy-io instead of pnfs. I fill prefer fail-fast instead of long debug
> sessions.

I don't quite agree.  With my change, the client will behave exactly the
same way as if the server explicitly didn't support v4.1
So if you explicitly ask for v4.1, you will get a fail-fast.
If you ask for "mount with whatever protocol seems to work" (the
default), then you will get a protocol that works - not 4.1 in this
case.

>
> On the other hand, I understand, that it's not always possible to fix server
> or clients in production environment and time-to-time workarounds are
> necessary.

Yes.  We are not overly eager to work-around broken implementations, but
my memory is that we do it when it brings measurable benefits without
unreasonable compromises.

In this can I am suggesting that change that results in a user-visible
error of EPROTONOSUPPORT instead of EIO - in a case where the server
doesn't respond to out v4.1 in the way we think it should.  I don't
think there is any compromise there.

Thanks,
NeilBrown


>
>
> Tigran.
>
>
> ----- Original Message -----
>> From: "NeilBrown" <neilb@suse.com>
>> To: "Trond Myklebust" <trond.myklebust@primarydata.com>, "Anna Schumaker" <anna.schumaker@netapp.com>
>> Cc: "linux-nfs" <linux-nfs@vger.kernel.org>
>> Sent: Friday, March 16, 2018 12:44:23 AM
>> Subject: [PATCH - v2] NFSv4: handle EINVAL from EXCHANGE_ID better.
>
>> nfs4_proc_exchange_id() can return -EINVAL if the server
>> reported NFS4INVAL (which I have seen in a packet trace),
>> or nfs4_check_cl_exchange_flags() exchange flags detects
>> a problem.
>> Each of these mean that NFSv4.1 later cannot be use, but
>> they should not prevent fallback to NFSv4.0.
>> 
>> Currently this EINVAL error is returned by nfs4_proc_exchange_id() to
>> nfs41_discover_server_trunking(), and thence to
>> nfs4_discover_server_trunking().
>> nfs4_discover_server_trunking() doesn't understand EINVAL, so converts
>> it to EIO which causes mount.nfs to think something is horribly wrong
>> and to give up.
>> 
>> It would be a more graceful failure if nfs4_discover_server_trunking()
>> mapped -EINVAL to -EPROTONOSUPPORT - a failure to negotiate a client
>> ID clearly shows that NFSv4.1 cannot be supported, but isn't as
>> general a failure as EIO.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>> 
>> Sorry - a bit too trigger-happy with that first version of the patch.
>> 
>> NeilBrown
>> 
>> fs/nfs/nfs4state.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index 91a4d4eeb235..b988e460553d 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -2219,6 +2219,8 @@ int nfs4_discover_server_trunking(struct nfs_client *clp,
>> 		clnt = clp->cl_rpcclient;
>> 		goto again;
>> 
>> +	case -NFS4ERR_INVAL:
>> +		/* Server confused - assume this minor isn't supported */
>> 	case -NFS4ERR_MINOR_VERS_MISMATCH:
>> 		status = -EPROTONOSUPPORT;
>> 		break;
>> --
>> 2.14.0.rc0.dirty

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]