From: Jeff King <peff@peff.net>
To: Eric Wong <e@80x24.org>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: What's cooking in git.git (Jul 2016, #03; Fri, 8)
Date: Sat, 9 Jul 2016 22:52:32 -0400 [thread overview]
Message-ID: <20160710025232.GA4666@sigill.intra.peff.net> (raw)
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/
I did (and do) like it. I think it may have simply gotten lost in the
mass of "should we handle EAGAIN from getdelim()" patches I sent (to
which I concluded "probably not").
There was one minor improvement I suggested[1] (and which you seemed to
like), which is to push the errno check into the function. That wasn't
expressed in patch form, though, so I've included below a version of
your patch with my suggestion squashed in.
I didn't include the test I mentioned in [2]. I think the potential for
portability and raciness problems make it probably not worth the
trouble.
[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
At least for me, this improves the readability of xread and
xwrite; hopefully allowing missing "continue" statements to
be spotted more easily.
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
next prev parent reply other threads:[~2016-07-10 2:52 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-08 22:59 What's cooking in git.git (Jul 2016, #03; Fri, 8) Junio C Hamano
2016-07-09 23:25 ` Eric Wong
2016-07-11 17:51 ` Junio C Hamano
2016-07-09 23:45 ` Eric Wong
2016-07-10 2:52 ` Jeff King [this message]
2016-07-10 3:47 ` Eric Wong
2016-07-10 4:45 ` Jeff King
2016-07-10 8:20 ` [PATCH 3/2] hoist out handle_nonblock function for xread and xwrite Eric Wong
2016-07-11 6:24 ` What's cooking in git.git (Jul 2016, #03; Fri, 8) Johannes Schindelin
2016-07-11 17:13 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160710025232.GA4666@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=e@80x24.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).