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

[Ian Campbell <ian.campbell@citrix.com>]

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@gmail.com>
> ---
>  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