[Qemu-devel] [PATCH 0/1] ISCSI: Dont call libiscsi for POLLIN if there are no bytes readable from the socket

[Qemu-devel] [PATCH 0/1] ISCSI: Dont call libiscsi for POLLIN if there are no bytes readable from the socket

[Ronnie Sahlberg]

Kevin, List, Paolo

Updated patch based on Paolo's suggestion to do these checks in iscsi_process_read  instead.
Additionally, since this means we can no remain setting a fd-is-readable event unconditionally we can handle target initiated NOPs better.
For example the case when the initiator is idle for a long time and has no i/o in flight a target may at this situation send NOPs to the initiator to ping that it is still alive, as an alternative to TCP-Keepalives.
Unconditionally setting fd-is-readable event means that we will trigger on such target driven NOPs and can respond to them even if the initiator itself is idle.


When iscsi_process_read is invoked we check if there is a socket error.
If there is we pass POLLIN down to libiscsi and let it handle it, and possibly also try to reconnect and recover.

If not, we check if there are any available bytes to read using ioctl(...FIONREAD...) and if there were no bytes avaialble, we assume that this was probably just a false invocation of the read event so we do nothing and just return.

If there were bytes available to read from the socket, we pass POLLIN down into libiscsi as usual and let it read and process them.


regards
ronnie sahlberg

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

[Ronnie Sahlberg]

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

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index d37c4ee..694f135 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -25,6 +25,9 @@
 #include "config-host.h"
 
 #include <poll.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/ioctl.h>
 #include "qemu-common.h"
 #include "qemu-error.h"
 #include "block_int.h"
@@ -116,6 +119,35 @@ iscsi_process_read(void *arg)
 {
     IscsiLun *iscsilun = arg;
     struct iscsi_context *iscsi = iscsilun->iscsi;
+    int socket_count = 0;
+    int err = 0;
+    socklen_t err_size = sizeof(err);
+
+    /* There are places where we might be invoked but the read-event
+       may not still be active.
+       Libiscsi treats POLLIN but socket having no bytes available to read
+       as a socket error.
+       So we have to check socket status and available bytes explicitely
+       before we invoke libiscsi.
+    */
+    if (getsockopt(iscsi_get_fd(iscsi), SOL_SOCKET, SO_ERROR, &err,
+                   &err_size) != 0 || err != 0) {
+        /* There is a socket error, call libicsi and let it try to handle
+           the error and maybe try reconnecting.
+         */
+        iscsi_service(iscsi, POLLIN);
+        iscsi_set_events(iscsilun);
+        return;
+    }
+
+    if (ioctl(iscsi_get_fd(iscsi), FIONREAD, &socket_count) == 0
+    && socket_count == 0) {
+        /* no bytes available to read from the socket, and there was no
+           error, just return without calling libiscsi
+         */
+        iscsi_set_events(iscsilun);
+        return;
+    }
 
     iscsi_service(iscsi, POLLIN);
     iscsi_set_events(iscsilun);
-- 
1.7.3.1

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]

[-- 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) {

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.

[ronnie sahlberg]

The idea was that by being very caredul with FIONREAD  I can guarantee
non-blocking behaviour without relying on making the socket
non-blocking.
The only purpose of making the socket non-blocking was to ensure that
writes dont block due to the lack of a FIONWRITE ioctl.


But, the code becomes simpler.

Ill apply your patch to libiscsi.  Thanks. Ignore this patch to qemu.

regards
ronnie sahlberg


On Tue, May 15, 2012 at 1:30 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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