From patchwork Mon May 14 15:30:39 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 159035 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 756C0B702A for ; Tue, 15 May 2012 01:31:17 +1000 (EST) Received: from localhost ([::1]:36283 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1STxEx-0000Km-B1 for incoming@patchwork.ozlabs.org; Mon, 14 May 2012 11:31:15 -0400 Received: from eggs.gnu.org ([208.118.235.92]:52603) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1STxEf-0008MG-UZ for qemu-devel@nongnu.org; Mon, 14 May 2012 11:31:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1STxEU-0005rG-4b for qemu-devel@nongnu.org; Mon, 14 May 2012 11:30:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:22713) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1STxET-0005qp-PF for qemu-devel@nongnu.org; Mon, 14 May 2012 11:30:46 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q4EFUh1k013608 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 14 May 2012 11:30:43 -0400 Received: from yakj.usersys.redhat.com (ovpn-112-19.ams2.redhat.com [10.36.112.19]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q4EFUePY020257; Mon, 14 May 2012 11:30:41 -0400 Message-ID: <4FB1251F.1050708@redhat.com> Date: Mon, 14 May 2012 17:30:39 +0200 From: Paolo Bonzini User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 MIME-Version: 1.0 To: Ronnie Sahlberg References: <1336773877-20725-1-git-send-email-ronniesahlberg@gmail.com> <1336773877-20725-2-git-send-email-ronniesahlberg@gmail.com> In-Reply-To: <1336773877-20725-2-git-send-email-ronniesahlberg@gmail.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: kwolf@redhat.com, qemu-devel@nongnu.org Subject: 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. X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --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) {