Re: [PATCH] nfsd: fix nfsd4_delegreturn to return correct error codes

[Bruce James Fields <bfields@fieldses.org>]

On Fri, Nov 06, 2015 at 09:19:47AM -0500, Jeff Layton wrote:
> On Fri, 6 Nov 2015 09:03:50 -0500
> Trond Myklebust <trond.myklebust@primarydata.com> wrote:
> 
> > On Fri, Nov 6, 2015 at 8:48 AM, Trond Myklebust
> > <trond.myklebust@primarydata.com> wrote:
> > > On Fri, Nov 6, 2015 at 8:08 AM, Andrew W Elble <aweits@rit.edu> wrote:
> > >>
> > >>> Umm... If the client is sending delegreturn, then why not destroy the
> > >>> delegation?

Yeah, good question, I didn't think about that.

The one thing that bothers me:

> Hmm...so is there any advantage to reporting NFS4ERR_DELEG_REVOKED
> there at all? I guess that could be a signal that it may not have held
> a delegation that it thought it had,

Yes, I'd like to think a little about this.  It does worry me that the
loss of a delegation could be completely silent.

Even in the absence of locks, userspace may now have gotten incorrect
lookup or stat results.  (Unless the client is careful not to depend on
delegations for any guarantees that go beyond close-to-open?)

So,

	client1		client2
	-------		-------

			make prog
					(client2 gets delegation on prog.c)

	vi prog.c
					(client2's deleg recalled, then
					revoked)

			wait for ac timeout
			make prog
			make: 'prog' is up to date

Hm, I think the client has to see a STATUS_RECALLABLE_STATE_REVOKED at
some point here, though.

In fact, is there any situation this could happen without the client
having seeing that flag on the SEQUENCE preceding this op?  I guess
there could be an extremely narrow window for a race between the two
ops.  Is that race is the only justification for having this error in
the protocol at all?  In the typical case it's going to see the sequence
flag at the same time as the error and have to do some kind of recovery
anyway.

> but it's probably too late to do anything about it if that occurs.

Yeah, I can't think what the client could do beyond log something scary.

> Some older clients may also not handle that error gracefully too,

Is it likely?  (How's the current client?)  Not sure what ungraceful
handling would be, though, it would be an odd client that would do
anything other than either FREE_STATEID or just give up.

I don't see any reason this case needs to be optimized, so if clients
handle the REVOKED error then maybe it's OK.

> so just returning NFS4_OK might be best...

Anyway, after all that, yes, maybe so, if only out of conservatism about
change.

--b.