Re: [PATCH RFC] nfsd: serialize layout stateid morphing operations

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Dec 07, 2015 at 11:12:26AM -0500, Jeff Layton wrote:
> On Mon, 7 Dec 2015 15:17:01 +0100
> Christoph Hellwig <hch@lst.de> wrote:
> 
> > On Mon, Dec 07, 2015 at 08:28:03AM -0500, Jeff Layton wrote:
> > > My understanding is that you need to increment the seqid when prior to
> > > sending the callback. The basic idea there is that you want to ensure
> > > that any LAYOUTGETs that were sent before the CB_LAYOUTRECALL get back
> > > an OLD_STATEID error. RFC5661, Section 12.5.3:
> > > 
> > >     After the layout stateid is established, the server increments by
> > >     one the value of the "seqid" in each subsequent LAYOUTGET and
> > >     LAYOUTRETURN response, and in each CB_LAYOUTRECALL request.
> > 
> > True.  Although I really don't see any way to make layoutrecall processing
> > race free that way.  I guess your patch to just drop the mutex is the
> > best we could do.
> > 
> > Note that it doesn't really matter for the current server, as we will
> > always recall the whole file, and thus leave no layout state for it on
> > the client.  But for finger grained recalls this could become a
> > problem.
> 
> 
> Just thinking out loud...
> 
> So we inc the seqid when sending the recall, and then the
> layoutreturn(s) will inc it again, so that should cover those cases.
> 
> Let's suppose that the client returns 0 for the CB though, but never
> actually returns the layouts. At that point I guess we need to revoke
> the whole stateid, similarly to how we revoke delegations that aren't
> returned.
> 
> The catch here is that the revocation timer is the same as the RPC
> timeout period. It doesn't seem like there ought to be any relationship
> there. It ought to be 1-2 lease periods, IIRC?

Currently it's retrying indefinitely, with a comment that it should cap
the number of retries somehow.  So we'd just do the arithmetic to pick a
number of retries that works.

I think current client behavior should prevent any of this being a
problem in practice?

But people who read the spec should have at least a fighting chance of
writing a working client.  I don't like the idea that somebody's going
to make a perfectly legal client change and suddenly the client will
fail.

As Christoph says we can end up in that situation just as easily by
writing more complicated code and not testing those cases.  And there's
probably some of that in the current sessions backchannel code.

Argh.  I guess I'll take a shot at some pynfs tests....

--b.