[PATCH 00/16] nfsd patches: bugfixes, deferral, compound processing

[J. Bruce Fields]

On Tue, Dec 05, 2006 at 04:57:33PM +1100, Neil Brown wrote:
> Thanks.  Looks good.  I did make a few cosmetic changes though.  I
> removed a few trailing white spaces and wrapped a whole lot of >80 char
> lines.

Thanks.  I should update my tools to warn me about that kind of
thing....

> I also changed
> 	if (rv == -EAGAIN)
> 		if (cache_defer_req(rqstp, h))
> 			rv = -ETIMEDOUT;
> to
> 	if (rv == -EAGAIN)
> 		if (cache_defer_req(rqstp, h) < 0)
> 			rv = -ETIMEDOUT;
> 
> so it is more obviously correct (to me).

That always suggests to me one of these interfaces that returns some
positive number (e.g. a length) on success and a -errno on failure, so
I'd probably go for != 0.  OK, whatever.

> I also note that if ip_map_lookup returns ETIMEDOUT we still drop the
> request.... we aren't really in a position to return and NFS error at
> that point.
> This should happen very rarely as the ipm is cached per TCP connection,
> but it is still possible at first connect and if the cache gets
> updated.  I presume we should break the TCP connection in that
> very-rare case ??

Right.

> > For now the following patches just deal with the simplest cases of
> > silent drops in the deferral code.  (One tricky case remains--the case
> > where we decide to drop the oldest request in cache_defer_req.  I'm
> > tempted just to force a revisit of the old request in that case, under
> > the assumption that when we loop back around and try to defer again
> > it'll probably result in an error we can return to the client.  But that
> > seems suboptimal.)
> 
> Do we need "optimal" in this case.  Maybe "obvious if slow" is more
> appropriate of rare edge cases such as this?

Sure, probably so.  (Though if possible maybe you want to err on the
side of something that doesn't attempt more memory allocations.)

--b.