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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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