[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH] libxenstore: Use poll() with a non-blocking read()
On 08/16/2015 08:46 PM, Jonathan Creekmore wrote:
On Aug 16, 2015, at 1:59 AM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
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?
It is a bug in the sense that addition of FMODE_ATOMIC_POS changed
user-visible behavior. And a fix was suggested in
http://permalink.gmane.org/gmane.linux.file-systems/93969 .
Copying Vitalii who sent an early version of that patch.
-boris
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)
So, the latest I saw on the LKML, the problem still existed as of 3.17 and, looking
forward through 4.2, the problem still exists there as well. I saw a few posts back
in March 2015 talking about the deadlock, but it appeared to have gotten no
traction. It isnât a deadlock in the kernel, but rather a deadlock between the
two blocking reads or a blocking read or write. The slight rewrite I applied in my
patch effectively works around the problem and prevents the library from
flip-flopping the nonblocking flag on the file descriptor between two threads.
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 <jonathan.creekmore@xxxxxxxxx>
---
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 <signal.h>
#include <stdint.h>
#include <errno.h>
+#include <poll.h>
#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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
