Re: [PATCHv3 02/13] xread: poll on non blocking fds

From: Stefan Beller <sbeller@google.com>
Date: Sep 23, 2015, 00:14 UTC
Message-ID: <CAGZ79kYKxNAXhbmXEtMeBYeGqu8i4bcUW_hpjLaPSF4p5O=uBg@mail.gmail.com>
In-Reply-To: <xmqqlhby2pj0.fsf@gitster.mtv.corp.google.com>

On Tue, Sep 22, 2015 at 1:00 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 27 quoted lines

> Jeff King <peff@peff.net> writes:
>
>> On Tue, Sep 22, 2015 at 12:45:51PM -0700, Junio C Hamano wrote:
>>
>>> One caveat is that the caller may not know in the first place.
>>>
>>> The last time I checked the existing callers of xread(), there were
>>> a few that read from a file descriptor they did not open themselves
>>> (e.g. unpack-objects that read from standard input).  The invoker of
>>> these processes is free to do O_NONBLOCK their input stream for
>>> whatever reason.
>>
>> Yeah. I do not think this is a bug at all; the user might have their
>> reasons for handing off an O_NONBLOCK pipe. If we take xread() to mean
>> "try to read from fd until we get a real error, some data, or an EOF",
>> then it is perfectly reasonable to replace spinning on read() (which we
>> do now) with a poll() for efficiency. The caller (and the user) does not
>> have to care, and should not notice; the outcome will be the same.
>
> I think we are in agreement, and that answers the question/guidance
> Stefan asked earlier in $gmane/278414, which was:
>
>> So rather a combination of both, with the warning only spewing every
>> 5 seconds or such?
>
> and the answer obviously is "No warning, do a poll() without timeout
> to block".

Ok. Expect that in a reroll.

To answer the first question upthread:

> I can sort of see EINTR but why is ENOMEM any special than other errors?

So we have 4 kinds of additional errors, when adding the poll. When we want to keep the behavior as close to the original as possible, we should not error out for things we had under control previously.

       EFAULT The array given as argument was not contained in the
calling program's address space.

       EINTR  A signal occurred before any requested event; see signal(7).

       EINVAL The nfds value exceeds the RLIMIT_NOFILE value.

       ENOMEM There was no space to allocate file descriptor tables.

I don't see a way EFAULT can be returned as we have the array hard coded on the stack. (No tricks with the heap or anything, we'll have it on the stack)

EINTR is possible while waiting. Actually it doesn't matter if we retry the poll or read first and then poll again, so we can easily just continue and do the read.

EINVAL is highly unlikely (as nfds == 1 in the case here), but can happen. ENOMEM is similar to EINVAL.

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(..). So maybe an implementation like this would already suffice:

ssize_t xread(int fd, void *buf, size_t len)
{
    ssize_t nr;
    if (len > MAX_IO_SIZE)
        len = MAX_IO_SIZE;
    while (1) {
        nr = read(fd, buf, len);
        if (nr < 0) {
            if (errno == EINTR)
                continue;
            if (errno == EAGAIN || errno == EWOULDBLOCK) {
                struct pollfd pfd;
                pfd.events = POLLIN;
                pfd.fd = fd;
                /* We deliberately ignore the return value of poll. */
                poll(&pfd, 1, -1);
                continue;
            }
        }
        return nr;
    }
}

In the resend I'll only check for EINTR and in other cases of early poll failure, we just die(..).