Re: [Xen-devel] [PATCH] libxenstore: Use poll() with a non-blocking read()

[Jonathan Creekmore <jonathan.creekmore@xxxxxxxxx>]

> 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?
> 
> 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