Subject: [nfsv4] what error should be returned to an impatient client?

From: Benny Halevy <[email-masked]>
Date: Wed, 11 Apr 2007 17:19:22 +0300

while reviewing the sessions text in draft-10 I realized that
it is not clear what error the server should return if the
client sent a new request on a slot that's still in use.

In the currently linux implementation (in Andy's latest patches)
nfsd4_sequence() just allows that
	status = nfs_ok;
	if (nfs41_get_slot_state(slot) != NFS4_SLOT_AVAILABLE)
		goto out;
...
out:
	dprintk("%s returns %d\n", __FUNCTION__, status);
	nfs4_unlock_state();
	return status;

I suggest returning NFS4ERR_BADSLOT or add a new error code NFS4ERR_BUSYSLOT
(other alternatives could be NFS4ERR_INVALID or NFS4ERR_SEQ_MISORDERED).

Benny

_______________________________________________
nfsv4 mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/nfsv4

---

From: "Talpey, Thomas" <[email-masked]>
Date: Wed, 11 Apr 2007 10:26:35 -0400

Well, since the client violated the no-retransmit principle, the
server could simply close the connection. In fact on an RDMA
transport, this may happen from the hardware itself. In my opinion
this is what the server should do. But, if the server is going to
return an error, it should be NFS4ERR_SEQ_MISORDERED.

Remember, in this kind of situation, the slot and sequence are
completely invalid, and the exchange is basically outside the
session. There is no replay protection, and the client will have
to use XID matching to detect the server's reply. Basically,
it's an invalid protocol request, almost as if the client had sent
RPC garbage.

Tom.

At 10:19 AM 4/11/2007, Benny Halevy wrote:
>pNFS mailing list
>[email-masked]
>http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs

---

From: "Noveck, Dave" <[email-masked]>
Date: Wed, 11 Apr 2007 10:46:01 -0400

> There is no replay protection, and the client will have
> to use XID matching to detect the server's reply.

This is a very interesting point.  I think it relates to
the discussion at the review meeting abut what the spec
should say about the XID.  Requiring the server at least
to propagate it on the reply so that if the client screws
up, he can at least see the error he gets, seems reasonable
to me.

Although a server may close the connection, I would hope that
as we try to debug prototypes of this stuff, we try to give
as meaningful a response to the client as possible, even 
when he send garbage.  Any help on the type of garbage can
save a lot debugging time, and that's a good thing.

I think the concept that we are closed to .x changes should 
not apply to the addition of errors.  We know the per-op 
error lists are not complete in the spec anyway, so clients
will not be depending on specific limits as to errors.

I like the idea of a separate error to tell the client he
sent unbelievably stinky rotten garbage, which this would
be.

-----Original Message-----
From: Talpey, Thomas 
Sent: Wednesday, April 11, 2007 10:27 AM
To: Benny Halevy
Cc: [email-masked]; [email-masked]
Subject: [nfsv4] Re: [pnfs] what error should be returned to an
impatient client?

Tom.

---

From: "Talpey, Thomas" <[email-masked]>
Date: Wed, 11 Apr 2007 11:00:56 -0400

I'm fine with sending the error, then closing the connection.
The client has been proven insane, so there is no reason to
attempt recovery since it almost certainly won't help.

We should not introduce a new error return for this however,
especially not BUSYSLOT which would be a contradiction in
terms. SEQ_MISORDERED is more than enough to indicate the
error. Is carving a protocol distinction between one kind of
garbage and another kind at all worth it? Not to me.

Tom.

---

From: Mike Eisler <[email-masked]>
Date: Wed, 11 Apr 2007 08:10:41 -0700 (PDT)

> return an error, it should be NFS4ERR_SEQ_MISORDERED.

_MISORDERED is for when the client sends a sequenceid that is
less than the sequencedid in the reply cache or greater (by two or
more)
than the sequenceid in reply cache.

I think there are four approaches:

- disconnect. One can look at my 2006 connectathon presentation as
  to why that is a bad idea. Even though the spec allows it, don't
  do it.

- The server just processes the request as if it it a new one. This
  is the Rick Macklem approach (see his presentation from 2006
  connectathon). So there are two or more requests outstanding 
  on the same slot, sequenceid pair. The first request
  that completes on the server side gets to update the cache.
  The client gets what it deserves. However I suspect this is
  more complex logic for the v4.1 server than for the v4.0
  server.

- Return a new error, say: NFS4ERR_INPROGRESS. I don't really like
  this because it is accomodating a broken client. 

- Return NFS4ERR_DELAY. I like this because it uses an existing
  error and so is not providing accomodation to brokeness.

> to use XID matching to detect the server's reply. Basically,

To make it clear to everyone, there is no slot table replay
protection because SEQUENCE is returning an error, versus a sequenceid
and slotid. Hence that is why the xid is needed.

---

From: "Noveck, Dave" <[email-masked]>
Date: Wed, 11 Apr 2007 11:44:52 -0400

>   this because it is accomodating a broken client. 

The accommodation consists of giving him an error so he can note
that he has done something wrong and fix it.  I'm not against
that sort of accommodation.

>   error and so is not providing accomodation to brokeness.

So the broken client will not get fixed.  DELAY is a normal and
expected error which indicates the server is not ready to do
something, and that it isn't the client's fault.  So the problem
won't get fixed as easily.

Why is this better?

Would you still object if the error were NFS4ERR_BROKEN_CLIENT 
or somethng else that made it absolutely clear the client is 
not supposed to do this and we are only providing the error in 
the interests of getting the client fixed.  

-----Original Message-----
From: Mike Eisler [mailto:[email-masked]] 
Sent: Wednesday, April 11, 2007 11:11 AM
To: [email-masked]
Subject: Re: [nfsv4] Re: [pnfs] what error should be returned to an
impatientclient?

---

From: "Halevy, Benny" <[email-masked]>
Date: Wed, 11 Apr 2007 15:01:42 -0400

> the interests of getting the client fixed.

I agree with Dave on all points above.  First, we better return an error
before tearing down the session.  If the bug on the client
implementation is rare enough it could still be useful for end users and
returning an error, logging it, and handling it properly (by closing and
recreating the session on the client side) will give developers a better
chance to fix the bug.

Errors must be unique per client error recovery procedure. I.e.
different error conditions can return the same error code if the client
is expected to recover for these conditions in the same way.  Recovering
from this particular case is unique therefore it needs a different error
code from the ones already defined for SEQUENCE.  To avoid introducing a
new error code, how about returning NFS4ERR_INVAL in this case?
(although NFS4ERR_DAMN_YOU_CLIENTS does seem nicer :)

---

From: Mike Eisler <[email-masked]>
Date: Wed, 11 Apr 2007 14:42:12 -0700 (PDT)

> that sort of accommodation.

How is the client going to fix it? Is there a new trend in
artificial intelligence that causes NFSv4 clients to fix
themselves? Can we get this AI agent on NFSv3 clients? :-)

> won't get fixed as easily.

NFS4ERR_DELAY at least is an explicit indication that the
client needs to back off.

> the interests of getting the client fixed.  

I'd like to avoid punishing customers for choosing bad clients.
I used to do that, and lost business. I fear that while
NFS4ERR_INPROGRESS will just result in client implementors exploiting
it routinely, and possibly immediately re-sending,
NFS4ERR_BROKEN_CLIENT
is likely to be mapped to EIO and returned immediately to the app
as a fatal error.

If the consensus is to put either error into the spec, as
editor I'll do the job, under protest. But, no way will I ever
write a server that uses either error for this case.

---

From: "Noveck, Dave" <[email-masked]>
Date: Wed, 11 Apr 2007 21:56:33 -0400

By he, I meant the implementor.  I suppose I should have said
"he or she".  The client itself would be "it".  No AI. 

-----Original Message-----
From: Mike Eisler [mailto:[email-masked]] 
Sent: Wednesday, April 11, 2007 5:42 PM
To: [email-masked]
Subject: RE: [nfsv4] Re: [pnfs] what error should be returned to
animpatientclient?

--- "Noveck, Dave" <[email-masked]> wrote:

> The accommodation consists of giving him an error so he can note that 
> he has done something wrong and fix it.  I'm not against that sort of 
> accommodation.

How is the client going to fix it? Is there a new trend in artificial
intelligence that causes NFSv4 clients to fix themselves? Can we get
this AI agent on NFSv3 clients? :-)

> something, and that it isn't the client's fault.  So the problem won't

> get fixed as easily.

NFS4ERR_DELAY at least is an explicit indication that the client needs
to back off.

> 
> Would you still object if the error were NFS4ERR_BROKEN_CLIENT or 
> somethng else that made it absolutely clear the client is not supposed

> to do this and we are only providing the error in the interests of 
> getting the client fixed.

I'd like to avoid punishing customers for choosing bad clients.
I used to do that, and lost business. I fear that while
NFS4ERR_INPROGRESS will just result in client implementors exploiting it
routinely, and possibly immediately re-sending, NFS4ERR_BROKEN_CLIENT is
likely to be mapped to EIO and returned immediately to the app as a
fatal error.

If the consensus is to put either error into the spec, as editor I'll do
the job, under protest. But, no way will I ever write a server that uses
either error for this case. 
> _MISORDERED is for when the client sends a sequenceid that is less 
> than the sequencedid in the reply cache or greater (by two or
> more)
> > session. There is no replay protection, and the client will have to 
> > use XID matching to detect the server's reply. Basically,
> 
> To make it clear to everyone, there is no slot table replay protection

> because SEQUENCE is returning an error, versus a sequenceid and 
> slotid. Hence that is why the xid is needed.
> 
> > >while reviewing the sessions text in draft-10 I realized that it is

> > >not clear what error the server should return if the client sent a 
> > >new request on a slot that's still in use.
> > >

---

From: "Iyer, Rahul" <[email-masked]>
Date: Wed, 11 Apr 2007 14:23:40 -0700

> From: Halevy, Benny [mailto:[email-masked]] 
> Sent: Wednesday, April 11, 2007 12:02 PM
> To: Noveck, Dave; [email-masked]; [email-masked]
> Subject: RE: [nfsv4] Re: [pnfs] what error should be returned 
> toanimpatientclient?
> 
> Dave Noveck said:
> > 
> I agree with Dave on all points above.  First, we better 
> return an error before tearing down the session.  If the bug 
> on the client implementation is rare enough it could still be 
> useful for end users and returning an error, logging it, and 
> handling it properly (by closing and recreating the session 
> on the client side) will give developers a better chance to 
> fix the bug.
> 
> different error conditions can return the same error code if 
> the client is expected to recover for these conditions in the 
> same way.  Recovering from this particular case is unique 
> therefore it needs a different error code from the ones 
> already defined for SEQUENCE.  To avoid introducing a new 
> error code, how about returning NFS4ERR_INVAL in this case?

Just to add to Benny's original question, our current implementation has
a curious side effect of "allowing" the slot to be used by 2 RPCs
simultaneously. In the current implementation, if the aforementioned
scenario does occur, the sequenceid on the slot isn't bumped up and the
client eventually sees NFSERR_SEQ_MISORDERED errors on the next request
in that slot. So, despite the server "allowing" it, from a pure client
side view, NFSERR_SEQ_MISORDERED is what the client currently sees.
Thanks
Regards
Rahul

> protection

---

From: "Iyer, Rahul" <[email-masked]>
Date: Wed, 11 Apr 2007 14:54:02 -0700

> Sent: Wednesday, April 11, 2007 2:42 PM
> To: [email-masked]
> to animpatientclient?

How about the client tearing down the session, re-establishing it and
retrying the operation? 

> NFS4ERR_INPROGRESS will just result in client implementors 
> exploiting it routinely, and possibly immediately re-sending, 
> NFS4ERR_BROKEN_CLIENT is likely to be mapped to EIO and 
> returned immediately to the app as a fatal error.

What if this were treated akin to a stale clientid error? In which case
it would mean tear down state, re-establish state and retry the
operation. It could also mean "mark this slot useless, use other slots
to clean up and re-establish state ASAP".

Regards
Rahul

> editor I'll do the job, under protest. But, no way will I 
> ever write a server that uses either error for this case. 
> >

---

From: Spencer Shepler <[email-masked]>
Date: Thu, 12 Apr 2007 00:39:48 -0500

>   error and so is not providing accomodation to brokeness.

This is my choice.  Even though DELAY is an old error, in the
context of the new operation SEQUENCE, it means something
new (ERR_MISORDERED covers other sequence issues as Mike stated).

The return of DELAY for the SEQUENCE will be clearly explained
in the SEQUENCE operation and is sufficient to deal with this
situation.

Spencer