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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 08/16/2015 08:46 PM, Jonathan Creekmore wrote:
>> On Aug 16, 2015, at 1:59 AM, Ian Campbell <ian.campbell@citrix.com> 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@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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel