Re: [Qemu-devel] [PATCH] ISCSI: iscsi_process_read callback for when the iscsi socket becomes readable may be invoked by qemu after the fd-is-readable event has cleared.

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

Il 12/05/2012 00:04, Ronnie Sahlberg ha scritto:
> Libiscsi treats a situation such as POLLIN was invoked and the socket is readable but ioctl(...FIONREAD...) returns that there are no bytes available to read as an error and that the socket is faulty or has been closed.
> which may trigger a slow process of closing down the socket completely and trying to reconnect to recover.
> 
> Update the iscsi fd-is-readable callback  iscsi_process_read to check for this condition explicitely.
> If are invoked and getsockopt tells us there is a real socket problem then we pass POLLIN onto libiscsi and let it try to handle the situation and/or recover.
> If there is no error, but ioctl(...FIONREAD...) still indicates that there were no bytes to read, then we treat this as just a false invokation from the eventsystem and do nothing.
> If there are bytes available to read, then we pass POLLIN into libiscsi and let it read and process the bytes.
> 
> regards
> ronnie sahlberg

I can apply this patch as a workaround, but I see that you already
applied the same thing upstream in libiscsi?

And indeed, I think this is a bug in libiscsi.  You make the socket
nonblocking (on POSIX only; but you can do the same ffor Win32, please
see the code in qemu to set the FIONBIO ioctls), but then you do not
handle EAGAIN.  Furthermore, you confuse read returning 0 (end-of-data)
with read returning EAGAIN (no data available).  If you fix this, all
the things you do with FIONREAD are not necessary.  See the attached
patch, compile-tested only.

Paolo

[-- Attachment #2: libiscsi-read.patch --]
[-- Type: text/x-patch, Size: 2878 bytes --]

diff --git a/lib/socket.c b/lib/socket.c
index 295cbf3..bbc5633 100644
--- a/lib/socket.c
+++ b/lib/socket.c
@@ -228,24 +228,8 @@ iscsi_read_from_socket(struct iscsi_context *iscsi)
 {
 	struct iscsi_in_pdu *in;
 	ssize_t data_size, count;
-	int socket_count = 0;
-
-	if (ioctl(iscsi->fd, FIONREAD, &socket_count) != 0) {
-		iscsi_set_error(iscsi, "Socket failure. Socket FIONREAD failed");
-		return -1;
-	}
-	if (socket_count == 0) {
-		int ret, err = 0;
-		socklen_t err_size = sizeof(err);
-
-		ret = getsockopt(iscsi->fd, SOL_SOCKET, SO_ERROR, &err, &err_size);
-		/* someone just called us without the socket being readable */
-		if (ret == 0 && err == 0) {
-			return 0;
-		}
-		iscsi_set_error(iscsi, "Socket failure. Socket is readable but no bytes available in FIONREAD");
-		return -1;
-	}
+	int ret, err = 0;
+	socklen_t err_size = sizeof(err);
 
 	if (iscsi->incoming == NULL) {
 		iscsi->incoming = malloc(sizeof(struct iscsi_in_pdu));
@@ -259,27 +243,23 @@ iscsi_read_from_socket(struct iscsi_context *iscsi)
 
 	/* first we must read the header, including any digests */
 	if (in->hdr_pos < ISCSI_HEADER_SIZE) {
-		/* try to only read the header, and make sure we don't
-		 * read more than is available in the socket;
+		/* try to only read the header, the socket is nonblocking, so
+		 * no need to limit the read to what is available in the socket
 		 */
 		count = ISCSI_HEADER_SIZE - in->hdr_pos;
-		if (socket_count < count) {
-			count = socket_count;
-		}
 		count = recv(iscsi->fd, &in->hdr[in->hdr_pos], count, 0);
+		if (count == 0) {
+			return -1;
+		}
 		if (count < 0) {
-			if (errno == EINTR) {
+			if (errno == EINTR || errno == EAGAIN) {
 				return 0;
 			}
 			iscsi_set_error(iscsi, "read from socket failed, "
 				"errno:%d", errno);
 			return -1;
 		}
-		if (count == 0) {
-			return 0;
-		}
 		in->hdr_pos  += count;
-		socket_count -= count;
 	}
 
 	if (in->hdr_pos < ISCSI_HEADER_SIZE) {
@@ -291,14 +271,7 @@ iscsi_read_from_socket(struct iscsi_context *iscsi)
 	if (data_size != 0) {
 		unsigned char *buf = NULL;
 
-		/* No more data right now */
-		if (socket_count == 0) {
-			return 0;
-		}
 		count = data_size - in->data_pos;
-		if (count > socket_count) {
-			count = socket_count;
-		}
 
 		/* first try to see if we already have a user buffer */
 		buf = iscsi_get_user_in_buffer(iscsi, in, in->data_pos, &count);
@@ -315,19 +288,18 @@ iscsi_read_from_socket(struct iscsi_context *iscsi)
 		}
 
 		count = recv(iscsi->fd, buf, count, 0);
+		if (count == 0) {
+			return -1;
+		}
 		if (count < 0) {
-			if (errno == EINTR) {
+			if (errno == EINTR || errno == EAGAIN) {
 				return 0;
 			}
 			iscsi_set_error(iscsi, "read from socket failed, "
 				"errno:%d", errno);
 			return -1;
 		}
-		if (count == 0) {
-			return 0;
-		}
 		in->data_pos += count;
-		socket_count -= count;
 	}
 
 	if (in->data_pos < data_size) {