All of lore.kernel.org
 help / color / mirror / Atom feed
From: NeilBrown <neilb@suse.com>
To: "Mkrtchyan\, Tigran" <tigran.mkrtchyan@desy.de>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>,
	Anna Schumaker <anna.schumaker@netapp.com>,
	linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH - v2] NFSv4: handle EINVAL from EXCHANGE_ID better.
Date: Tue, 20 Mar 2018 11:09:52 +1100	[thread overview]
Message-ID: <87r2ofa9pr.fsf@notabene.neil.brown.name> (raw)
In-Reply-To: <584484878.12780264.1521192712528.JavaMail.zimbra@desy.de>

[-- 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 --]

  parent reply	other threads:[~2018-03-20  0:10 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-15 23:29 [PATCH] " NeilBrown
2018-03-15 23:44 ` [PATCH - v2] " NeilBrown
2018-03-16  9:31   ` Mkrtchyan, Tigran
2018-03-16 12:10     ` Trond Myklebust
2018-03-20  0:32       ` NeilBrown
2018-03-20 14:14         ` Trond Myklebust
2018-03-20 18:44           ` NeilBrown
2018-03-20 19:43             ` Trond Myklebust
2018-03-20 19:58               ` NeilBrown
2018-03-20 20:13                 ` Trond Myklebust
2018-03-20 21:20                   ` NeilBrown
2018-03-20  0:09     ` NeilBrown [this message]
2018-03-20 21:48   ` J. Bruce Fields
2018-03-20 22:12     ` NeilBrown
2018-04-03  0:41     ` NeilBrown
2018-04-03 16:02       ` J. Bruce Fields

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r2ofa9pr.fsf@notabene.neil.brown.name \
    --to=neilb@suse.com \
    --cc=anna.schumaker@netapp.com \
    --cc=linux-nfs@vger.kernel.org \
    --cc=tigran.mkrtchyan@desy.de \
    --cc=trond.myklebust@primarydata.com \
    --subject='Re: [PATCH - v2] NFSv4: handle EINVAL from EXCHANGE_ID better.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.