Re: What's cooking in git.git (Jul 2016, #03; Fri, 8)

From

Jeff King <peff@peff.net>

Date

Jul 10, 2016, 02:52 UTC

Message-ID

<20160710025232.GA4666@sigill.intra.peff.net>

In-Reply-To

<20160709234518.GA3702@dcvr.yhbt.net>

On Sat, Jul 09, 2016 at 11:45:18PM +0000, Eric Wong wrote:

> Junio C Hamano <gitster@pobox.com> wrote:
> > * sb/submodule-parallel-fetch (2016-06-27) 2 commits
> >   (merged to 'next' on 2016-07-06 at de5fd35)
> >  + xwrite: poll on non-blocking FDs
> >  + xread: retry after poll on EAGAIN/EWOULDBLOCK
> > 
> >  Fix a recently introduced codepaths that are involved in parallel
> >  submodule operations, which gave up on reading too early, and
> >  could have wasted CPU while attempting to write under a corner case
> >  condition.
> > 
> >  Will merge to 'master'.
> 
> Any thoughts on my cleanup 3/2 patch for this series?
> ("hoist out io_wait function for xread and xwrite")
> https://public-inbox.org/git/20160627201311.GA7039@dcvr.yhbt.net/raw
> 
> Jeff liked it:
> https://public-inbox.org/git/20160627214947.GA17149@sigill.intra.peff.net/

[1] https://public-inbox.org/git/20160627222238.GA23645@sigill.intra.peff.net

[2] https://public-inbox.org/git/20160627214947.GA17149@sigill.intra.peff.net

-- >8 --
From: Eric Wong <e@80x24.org>
Subject: [PATCH] hoist out io_wait function for xread and xwrite

Signed-off-by: Eric Wong <e@80x24.org>
Signed-off-by: Jeff King <peff@peff.net>
---
Since both conditionals just call "continue", you could actually fold
them into a single if() in each caller, but I think it's easier to
follow as two separate ones.

You could actually fold the t
 wrapper.c | 48 ++++++++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index f7ea6c4..0a966d6 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -224,6 +224,24 @@ int xopen(const char *path, int oflag, ...)
 	}
 }
 
+static int handle_nonblock(int fd, short poll_events)
+{
+	struct pollfd pfd;
+
+	if (errno != EAGAIN && errno != EWOULDBLOCK)
+		return 0;
+
+	pfd.fd = fd;
+	pfd.events = poll_events;
+
+	/*
+	 * no need to check for errors, here;
+	 * a subsequent read/write will detect unrecoverable errors
+	 */
+	poll(&pfd, 1, -1);
+	return 1;
+}
+
 /*
  * xread() is the same a read(), but it automatically restarts read()
  * operations with a recoverable error (EAGAIN and EINTR). xread()
@@ -239,21 +257,8 @@ ssize_t xread(int fd, void *buf, size_t len)
 		if (nr < 0) {
 			if (errno == EINTR)
 				continue;
-			if (errno == EAGAIN || errno == EWOULDBLOCK) {
-				struct pollfd pfd;
-				pfd.events = POLLIN;
-				pfd.fd = fd;
-				/*
-				 * it is OK if this poll() failed; we
-				 * want to leave this infinite loop
-				 * only when read() returns with
-				 * success, or an expected failure,
-				 * which would be checked by the next
-				 * call to read(2).
-				 */
-				poll(&pfd, 1, -1);
+			if (handle_nonblock(fd, POLLIN))
 				continue;
-			}
 		}
 		return nr;
 	}
@@ -274,21 +279,8 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
 		if (nr < 0) {
 			if (errno == EINTR)
 				continue;
-			if (errno == EAGAIN || errno == EWOULDBLOCK) {
-				struct pollfd pfd;
-				pfd.events = POLLOUT;
-				pfd.fd = fd;
-				/*
-				 * it is OK if this poll() failed; we
-				 * want to leave this infinite loop
-				 * only when write() returns with
-				 * success, or an expected failure,
-				 * which would be checked by the next
-				 * call to write(2).
-				 */
-				poll(&pfd, 1, -1);
+			if (handle_nonblock(fd, POLLOUT))
 				continue;
-			}
 		}
 
 		return nr;

-- 
2.9.0.406.g77f030d