From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH] libxenstore: Use poll() with a non-blocking read() Date: Sun, 16 Aug 2015 09:59:32 +0100 Message-ID: <1439715572.3480.19.camel@citrix.com> References: <1439502287-5520-1-git-send-email-jonathan.creekmore@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZQtn7-00040a-Bs for xen-devel@lists.xenproject.org; Sun, 16 Aug 2015 08:59:45 +0000 In-Reply-To: <1439502287-5520-1-git-send-email-jonathan.creekmore@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jonathan Creekmore , xen-devel@lists.xenproject.org, David Vrabel , Konrad Rzeszutek Wilk , BorisOstrovsky Cc: ian.jackson@eu.citrix.com, wei.liu@citrix.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On Thu, 2015-08-13 at 16:44 -0500, Jonathan Creekmore wrote: > With the addition of FMODE_ATOMIC_POS in the Linux 3.14 kernel, > concurrent blocking file accesses to a single open file descriptor can > cause a deadlock trying to grab the file position lock. If a watch has > been set up, causing a read_thread to blocking read on the file > descriptor, then future writes that would cause the background read to > complete will block waiting on the file position lock before they can > execute. This sounds like you are describing a kernel bug. Shouldn't this be fixed in the kernel? In fact it even sounds a bit familiar, I wonder if it is fixed in some version of Linux >> 3.14? (CCing a few relevant maintainers) > This race condition only occurs when libxenstore is accessing > the xenstore daemon through the /proc/xen/xenbus file and not through > the unix domain socket, which is the case when the xenstore daemon is > running as a stub domain or when oxenstored is passed --disable-socket. > > Arguably, since the /proc/xen/xenbus file is declared nonseekable, then > the file position lock need not be grabbed, since the file cannot be > seeked. However, that is not how the kernel API works. On the other > hand, using the poll() API to implement the blocking for the read_all() > function prevents the file descriptor from being switched back and forth > between blocking and non-blocking modes between two threads. > > Signed-off-by: Jonathan Creekmore > --- > tools/xenstore/xs.c | 52 ++++++++++++++++--------------------------- > --------- > 1 file changed, 16 insertions(+), 36 deletions(-) > > diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c > index d1e01ba..9b75493 100644 > --- a/tools/xenstore/xs.c > +++ b/tools/xenstore/xs.c > @@ -31,6 +31,7 @@ > #include > #include > #include > +#include > #include "xenstore.h" > #include "list.h" > #include "utils.h" > @@ -145,22 +146,6 @@ struct xs_handle { > > static int read_message(struct xs_handle *h, int nonblocking); > > -static bool setnonblock(int fd, int nonblock) { > - int flags = fcntl(fd, F_GETFL); > - if (flags == -1) > - return false; > - > - if (nonblock) > - flags |= O_NONBLOCK; > - else > - flags &= ~O_NONBLOCK; > - > - if (fcntl(fd, F_SETFL, flags) == -1) > - return false; > - > - return true; > -} > - > int xs_fileno(struct xs_handle *h) > { > char c = 0; > @@ -216,7 +201,7 @@ error: > static int get_dev(const char *connect_to) > { > /* We cannot open read-only because requests are writes */ > - return open(connect_to, O_RDWR); > + return open(connect_to, O_RDWR | O_NONBLOCK); > } > > static struct xs_handle *get_handle(const char *connect_to) > @@ -365,42 +350,37 @@ static bool read_all(int fd, void *data, > unsigned int len, int nonblocking) > /* With nonblocking, either reads either everything > requested, > * or nothing. */ > { > - if (!len) > - return true; > - > - if (nonblocking && !setnonblock(fd, 1)) > - return false; > + int done; > + struct pollfd fds[] = { > + { > + .fd = fd, > + .events = POLLIN > + } > + }; > > while (len) { > - int done; > + if (!nonblocking) { > + if (poll(fds, 1, -1) < 1) { > + return false; > + } > + } > > done = read(fd, data, len); > if (done < 0) { > if (errno == EINTR) > continue; > - goto out_false; > + return false; > } > if (done == 0) { > /* It closed fd on us? EBADF is > appropriate. */ > errno = EBADF; > - goto out_false; > + return false; > } > data += done; > len -= done; > - > - if (nonblocking) { > - nonblocking = 0; > - if (!setnonblock(fd, 0)) > - goto out_false; > - } > } > > return true; > - > -out_false: > - if (nonblocking) > - setnonblock(fd, 0); > - return false; > } > > #ifdef XSTEST