Subject: RE: Stateid and seqid

From: "Noveck, Dave" <[email-masked]>
Date: Wed, 31 Jan 2001 10:28:00 -0800

Sergei Klyushin wrote:
> Kendrick Smith wrote:
> >
> > In our server implementation, we can't determine the lockowner
> > corresponding to an old stateid without retaining an entry in
> > our stateid table.  (We found doing this to be impractical using 64-bit
> > stateids.)  So it seems that retaining the last superseded stateid may
or
> > may not be an issue, depending on the server implementation.  I do think
> > that this would be a useful remark for the implementor's guidelines (but
> > probably not
> > needed in the spec).
> >
> 
> According to spec, if you can't locate nfs_lockowner by stateid,
> it means BAD_STATEID or STALE_STATEID, but not OLD_STATEID

I don't see the problem recognizing OLD stateid's as OLD (and not BAD).
The spec says you are supposed to do it and I don't see why it would
be difficult.
 
> 
> >
> > > > So, you don't store stateid (or even full request) for reply
message,
> > > > but you are based only on seqid = stored_seqid? How you could be
sure,
> > > > that it's real replay, but not logical error? I want to be sure at
> > > > this point, that I understand replay correctly: for me it's real
> > > > retransmission of exactly the same package. Is it true?
> > >
> Kendrick Smith wrote:
> > > We only retransmit the response if the old request and new request are
> > > byte-by-byte identical, including rpc headers.
> >
> Noveck Dave wrote:
> >The entire request???
> >
> >If I do LOCK followed by WRITE of 64K, are you going to save the 64K
> >I write so that on a subsequent replay you can compare the entire
> >request including all that data?  Seems like a lot of work.
> > .....
> 
> According to spec., NFS Server must store and replay last response to any
> request with seqid only, but not for all requests. Looks like processing
> of replay is implementation depended. It must be based on seqid, but spec.
> says nothing more. Probably, I would compare full request also (at least
> for the first v.4 implementation) to avoid logical bugs.

There is a real problem with the way the spec describes this, which
has led to two divergent interpretations.  I think we need to resolve
this.  For example the spec says (section 8.1.5):

   If a request with a previous sequence number (r < L) is received, it
   is rejected with the return of error NFS4ERR_BAD_SEQID.  Given a
   properly-functioning client, the response to (r) must have been
   received before the last request (L) was sent.  If a duplicate of
   last request (r == L) is received, the stored response is returned.

The problem is such phrases as "request with a previous sequence number".
Requests don't really have sequence numbers, LOCK/UNLOCK, etc. operations
do.  I suspect that this text was written before COMPOUND became the
all-encompassing way to do things and was never rewritten to reflect
the fact that operations and requests are no longer one-to-one. 

So what do you implement?  I've implemented my seqid stuff to be
operation oriented; I save the response for the operation and replay that.
The other interpretation is to take "request" literally and try to save
the entire response.  For such requests as READ+UNLOCK, this can 
take a lot of space.  It would be quite easy to drive a server to
its knees, saving the contents of  many long READ's for many different
lockowner's.  This could happen as a denial-of-service attack but
might even happen with no malicious clients.

I think there are other problems with the whole-request approach.
Suppose you issue a request that has an operation with seqid X
and the entire request successfully executed on the server.  Now
suppose the reply is lost and the client resends the request.
Now the spec says "If a duplicate of the last request (r == L) 
is received, the stored response is returned."  The problem 
with this is the "is received".  Is the server supposed to check
this when the request is recieved (i.e. before it executes any
of the operations)?  I would expect that it would only notice
r=L when it gets to the operation that has the seqid.  So if
it starts executing the initial sequence of operations and
encounters an error (transient IO error, expired fh,, whatever), 
what then?  The server will think that the last seqid accted on 
was X while the client will think X-1.  The client will do
something to recover but when operation that includes seqid
X is issued, it will be in the context of a new rqeuest and
it won't match.  If you strictly embrace whole-request
interpretation you can say this is a logical error, but
how is the client to re-issue the operation (e.g. UNLOCK)
that may or may not have been executed on the server if
the original request is completed (in this case with an 
error).  On the operation-only interpretation, the client
can re-issue the UNLOCK as part of a new request with the
same seqid and it will be honored.

Also note that the paragraph (partially) quoted above 
lists the cases in terms of of the relationships between
r and L and then just assumes that that completely characterizes
the relationships between the two requests (or operations).
So when it says "If a duplicate of last request (r == L) is 
received", it ignores the case of r == L when it is duplicate
of the last operation but it not part of a duplicate request
or when it is r == L but is not a duplicate of the previous
operation.  Are these errors?  If so, what errors? 

> 
> >
> > > BTW, do you check lease time before reply?
> >
> Kendrick Smith wrote:
> > The client's lease is checked and renewed only if the seqid is correct.
> Is this
> > what everyone else is doing?
> >
> Noveck Dave wrote:
> >Right now my lease expiration code isn't there yet.  My inclination
> >is to treat replay as merely retransmitting what already happened,
> >so I wouldn't check it.
> 
> I would not check and renew lease on replay also.
> It would be consistent with RPC layer cashing.
> Not renewing lease on replay could cause some problems: if NFS client
> finally
> receives reply, the next request may get ERR_EXPIRED, but using RENEW
could
> help,
> and my feeling, that it's better, than keeping file opened (locked,...)
> forever, plus
> it will be consistent with RPC layer cashing.

I agree.  When a client first issues a request at time X and receives a 
response at time Y, all it can assume is that the server renewed the
lease at some time between X and Y, and he should schedule his RENEWAL 
logic accordingly.  (The case of relativistic server-client relative
motion is left as an exercise for the (not-yet-but-soon-to-be) truly 
deranged :-).

> 
> Another point to check lease at OPEN, if clientid OK -> nfs_lockowner
> found -> seqid OK.
> I would not check lease here also, but I would renew one.
> OPEN is the main point where lease starts and lease is one for all
> nfs_lockowners
> from the same client. If lease has expired before, but NFS Server still
has
> nfs_lockowner in his table, checking lease at this point will never allow
to
> OPEN a file.

Right.  You check a lease if you are using a lock that it covers.  For
OPEN, you are creating a new lease (and renewing any that still exist).
Section 8.4 talks about this.  It doesn't specifically exclude the
case of replay, but it does speak of "events" that trigger renewal
of which OPEN is one, but it is unclear whether the "event" is 
execution of an OPEN or simply responding to one.

> 
> In case clientid OK -> nfs_lockowner NOT found, I would NOT check lease
> also, bump seqid,
> renew lease and ask for OPEN_CONFIRM.

Agree.

> 
> One more note: good stateid and replay_seqid should never happened. It's
> logical error.
> I think BAD_SEQ_NUM should be returned.

I think the spec needs to be more explicit about error situations like that.

> 
> As far as I remember, there was agreement not to bump seqid, if any error
> returned
> (including ERR_EXPIRED).

Yes, that was what we all agreed at the bake-off.

> What about lease? I would NOT renew lease on ANY error also.

That seems troublesome to me.  The purpose of the renewal mechanism
is so that the server can know the client is still up and responsive.
There may be situations in which many requests will get errors 
because of server conditions.  It seems wrong to me to say that
in such a situation, the server must give up the ability to keep
track of the leases of clients.  Of course that means that we
have to define what errors will renew and those that won't.  Sigh.

---

From: "Sergey Klyushin" <[email-masked]>
Date: Wed, 31 Jan 2001 14:37:03 -0500

> operation oriented; I save the response for the operation and
Sorry for misunderstanding. I've very ease used "request".
My implementation is operation oriented also. As I understand idea of seqid
is to guarantee sequence of OPEN,LOCK,UNLOCK,CLOSE, in another words
OPERATIONS
that take seqid, and reply must be stored for such OPERATIONS, but not for
the whole request
    > have to define what errors will renew and those that won't.  Sigh.
Agree.
We need to look more close on what error renew lease.
Right now I am trying to review Andy's diagrams, and will post
my changes shortly. (Adny, please send SETCLIENTID and CONFIRM_CLIENTID
diagrams)