Re: [PATCHv2 2/7] xread: poll on non blocking fds

From: Jeff King <peff@peff.net>
Date: Dec 18, 2015, 03:07 UTC
Message-ID: <20151218030752.GA8432@sigill.intra.peff.net>
In-Reply-To: <1450224252-16833-3-git-send-email-sbeller@google.com>

On Tue, Dec 15, 2015 at 04:04:07PM -0800, Stefan Beller wrote:

Show 5 quoted lines

> If we get an EAGAIN or EWOULDBLOCK the fd must have set O_NONBLOCK.
> As the intent of xread is to read as much as possible either until the
> fd is EOF or an actual error occurs, we can ease the feeder of the fd
> by not spinning the whole time, but rather wait for it politely by not
> busy waiting.

I'm not sure this second sentence is true. The point of xread() is to do a single read that makes forward progress (or returns a real error), not to read as much as we can.

The rationale seems more to me like:

  The point of xread() is to perform read() until we either get some
  data, or encounter a "real" error. We do not count EINTR or EAGAIN as
  "real" errors, as a reader trying to make forward progress would
  generally just repeat the read, so we loop for them as a convenience.

  In the case of EINTR, trying the read() again immediately is fine; a
  signal interrupted us, but there is no reason to think the read()
  would not succeed if we tried it again.

  For EAGAIN, however, we know that the fd must have set O_NONBLOCK. In
  this case reading again immediately is not likely to produce results,
  and we will chew up CPU spinning on read() calls. Instead, let's
  insert a poll() to block until we actually get data.

I also don't know that this patch necessarily needs to be part of the parallel-fetch series anymore. We are not setting O_NONBLOCK ourselves, and I don't think the correctness of the new strbuf_read_once() is impacted by this. I.e., I think this optimization applies equally well to the hundreds of existing xread() calls, should they unexpectedly be fed an O_NONBLOCK descriptor.

That said, I do not mind it here, but I think it could be its own separate topic if you wanted to reduce the complexity of the parallel-fetch topic.

> We should not care if the call to poll failed, as we're in an infinite
> loop and can only get out with the correct read().

You might want to expand on "correct" here. I think you mean "can only get out with a read() that returns data or a "real" error" (I am reusing the terminology I introduced above; I am fine with other terminology, depending on how you word the rest of it).

-Peff