[Xen-devel] [PATCH] libxenstore: Use poll() with a non-blocking read()
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 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
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] libxenstore: Use poll() with a non-blocking read()
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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] libxenstore: Use poll() with a non-blocking read()
> On Aug 16, 2015, at 1:59 AM, Ian Campbell 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?
>
> 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
>> ---
>> 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_
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 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
---
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))
-
Re: [Xen-devel] [PATCH] libxenstore: Use poll() with a non-blocking read()
On 16/08/15 09:59, Ian Campbell 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. I think you should make libxenstore open /dev/xen/xenbus instead. Since this is a character device it should work correctly. It may be necessary to try /dev/xen/xenbus first and fallback to /proc/xen/xenbus. > This sounds like you are describing a kernel bug. Shouldn't this be > fixed in the kernel? /proc/xen/xenbus should never have existed (it's a character device masquerading as a regular file), but I guess we're stuck with it now. The correct kernel fix is to make /proc/xen/xenbus a character device identical to /dev/xen/xenbus or a symlink to /dev/xen/xenbus. David ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] libxenstore: Use poll() with a non-blocking read()
David Vrabel writes: > On 16/08/15 09:59, Ian Campbell 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. > > I think you should make libxenstore open /dev/xen/xenbus instead. Since > this is a character device it should work correctly. > So, I did a quick test and verified that 1) /dev/xen/xenbus did exist on my system and 2) that opening /dev/xen/xenbus instead of /proc/xen/xenbus fixed the problem I was seeing as well. I did think that it was a little weird that libxenstore was treating a proc file as a character device. It makes more sense to me to open the actual character device instead. > It may be necessary to try /dev/xen/xenbus first and fallback to > /proc/xen/xenbus. > When did /dev/xen/xenbus get created? It is a fairly straightforward change to just switch to /dev/xen/xenbus and is a bit larger refactor to probe for its existence first before falling back to /proc/xen/xenbus. Either way, I could work up a patch to do this if people are ok with that user-space change. That said, I still think that the original patch I submitted is worthwhile to prevent a subtle, but probably benign, race condition with changing the blocking flag on a file descriptor that is shared between two threads. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
