diff mbox

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.

Message ID 4FB1251F.1050708@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini May 14, 2012, 3:30 p.m. UTC 
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

Comments

ronnie sahlberg May 15, 2012, 10:44 a.m. UTC | #1 
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
diff mbox

Patch

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