[{"id":1770559,"web_url":"http://patchwork.ozlabs.org/comment/1770559/","msgid":"<0f9ffc3d-02e1-fde8-67ac-0f62101f1204@redhat.com>","list_archive_url":null,"date":"2017-09-18T22:36:30","subject":"Re: [Qemu-devel] [PATCH v2 7/7] block/nbd-client: do not yield from\n\tnbd_read_reply_entry","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/18/2017 08:59 AM, Vladimir Sementsov-Ogievskiy wrote:\n> Refactor nbd client to not yield from nbd_read_reply_entry. It's\n> possible now as all reading is done in nbd_read_reply_entry and\n> all request-related data is stored in per-request s->requests[i].\n> \n> We need here some additional work with s->requests[i].ret and\n> s->quit to not fail requests on normal disconnet and to not report\n> reading errors on normal disconnect.\n> \n> Signed-off-by: Vladimir Sementsov-Ogievskiy \n> ---\n> block/nbd-client.c | 29 ++++++++++-------------------\n> 1 file changed, 10 insertions(+), 19 deletions(-)\n\nThe diffstat may have merit, but I'm wondering:\n\n> \n> diff --git a/block/nbd-client.c b/block/nbd-client.c\n> index f80a4c5564..bf20d5d5e6 100644\n> --- a/block/nbd-client.c\n> +++ b/block/nbd-client.c\n> @@ -79,7 +79,11 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)\n> while (!s->quit) {\n> ret = nbd_receive_reply(s->ioc, &reply, &local_err);\n> if (ret < 0) {\n> - error_report_err(local_err);\n> + if (s->quit) {\n> + error_free(local_err);\n\nThis discards errors on all remaining coroutines after we detect an\nearly exit. Are we sure the coroutine that set s->quit will have\nreported an appropriate error, and thus explaining why we can discard\nall secondary errors? A comment in the code would be helpful.\n\n> + } else {\n> + error_report_err(local_err);\n> + }\n> }\n\n> @@ -210,7 +204,7 @@ static int nbd_co_receive_reply(NBDClientSession *s, NBDRequest *request)\n> s->requests[i].receiving = true;\n> qemu_coroutine_yield();\n> s->requests[i].receiving = false;\n> - if (!s->ioc || s->quit) {\n> + if (!s->ioc) {\n> ret = -EIO;\n\nWhy don't we need to check s->quit any more? That was just added\nearlier in the series; why the churn?\n\n> } else {\n> ret = s->requests[i].ret;\n> @@ -218,11 +212,6 @@ static int nbd_co_receive_reply(NBDClientSession *s, NBDRequest *request)\n> \n> s->requests[i].coroutine = NULL;\n> \n> - /* Kick the read_reply_co to get the next reply. */\n> - if (s->read_reply_co) {\n> - aio_co_wake(s->read_reply_co);\n> - }\n> -\n> qemu_co_mutex_lock(&s->send_mutex);\n> s->in_flight--;\n> qemu_co_queue_next(&s->free_sema);\n> @@ -364,6 +353,8 @@ void nbd_client_close(BlockDriverState *bs)\n> \n> nbd_send_request(client->ioc, &request);\n> \n> + client->quit = true;\n\nPreviously, client->quit was only set when detecting a broken server,\nnow it is also set for a clean exit. Do we need to change any\ndocumentation of the field?","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=eblake@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xx18k2Mhzz9s7G\n\tfor ;\n\tTue, 19 Sep 2017 08:37:09 +1000 (AEST)","from localhost ([::1]:39185 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1du4f1-0004bI-K7\n\tfor incoming@patchwork.ozlabs.org; Mon, 18 Sep 2017 18:37:03 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36648)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1du4ee-0004WX-1F\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 18:36:41 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1du4ec-0000YR-SB\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 18:36:40 -0400","from mx1.redhat.com ([209.132.183.28]:57020)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1du4eY-0000Uc-21; Mon, 18 Sep 2017 18:36:34 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id B8DCA6A14;\n\tMon, 18 Sep 2017 22:36:32 +0000 (UTC)","from [10.10.124.97] (ovpn-124-97.rdu2.redhat.com [10.10.124.97])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 201295D980;\n\tMon, 18 Sep 2017 22:36:30 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com B8DCA6A14","To":"Vladimir Sementsov-Ogievskiy ,\n\tqemu-block@nongnu.org, qemu-devel@nongnu.org","References":"<20170918135935.255591-1-vsementsov@virtuozzo.com>\n\t<20170918135935.255591-8-vsementsov@virtuozzo.com>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"<0f9ffc3d-02e1-fde8-67ac-0f62101f1204@redhat.com>","Date":"Mon, 18 Sep 2017 17:36:30 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170918135935.255591-8-vsementsov@virtuozzo.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"Jx6KapD52Nik5VQ9veWRvHHxUvrRPPUOn\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.39]);\n\tMon, 18 Sep 2017 22:36:32 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH v2 7/7] block/nbd-client: do not yield from\n\tnbd_read_reply_entry","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, pbonzini@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1770807,"web_url":"http://patchwork.ozlabs.org/comment/1770807/","msgid":"<17fdbc1d-05db-ee91-6b13-cac9d3719c21@virtuozzo.com>","list_archive_url":null,"date":"2017-09-19T10:00:19","subject":"Re: [Qemu-devel] [PATCH v2 7/7] block/nbd-client: do not yield from\n\tnbd_read_reply_entry","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"19.09.2017 01:36, Eric Blake wrote:\n> On 09/18/2017 08:59 AM, Vladimir Sementsov-Ogievskiy wrote:\n>> Refactor nbd client to not yield from nbd_read_reply_entry. It's\n>> possible now as all reading is done in nbd_read_reply_entry and\n>> all request-related data is stored in per-request s->requests[i].\n>>\n>> We need here some additional work with s->requests[i].ret and\n>> s->quit to not fail requests on normal disconnet and to not report\n>> reading errors on normal disconnect.\n>>\n>> Signed-off-by: Vladimir Sementsov-Ogievskiy \n>> ---\n>> block/nbd-client.c | 29 ++++++++++-------------------\n>> 1 file changed, 10 insertions(+), 19 deletions(-)\n> The diffstat may have merit, but I'm wondering:\n>\n>> diff --git a/block/nbd-client.c b/block/nbd-client.c\n>> index f80a4c5564..bf20d5d5e6 100644\n>> --- a/block/nbd-client.c\n>> +++ b/block/nbd-client.c\n>> @@ -79,7 +79,11 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)\n>> while (!s->quit) {\n>> ret = nbd_receive_reply(s->ioc, &reply, &local_err);\n>> if (ret < 0) {\n>> - error_report_err(local_err);\n>> + if (s->quit) {\n>> + error_free(local_err);\n> This discards errors on all remaining coroutines after we detect an\n> early exit. Are we sure the coroutine that set s->quit will have\n> reported an appropriate error, and thus explaining why we can discard\n> all secondary errors? A comment in the code would be helpful.\n\nI'll think about it.\n\n>\n>> + } else {\n>> + error_report_err(local_err);\n>> + }\n>> }\n>> @@ -210,7 +204,7 @@ static int nbd_co_receive_reply(NBDClientSession *s, NBDRequest *request)\n>> s->requests[i].receiving = true;\n>> qemu_coroutine_yield();\n>> s->requests[i].receiving = false;\n>> - if (!s->ioc || s->quit) {\n>> + if (!s->ioc) {\n>> ret = -EIO;\n> Why don't we need to check s->quit any more? That was just added\n> earlier in the series; why the churn?\n\nit is \"to not fail requests on normal disconnet\". Because reply_entry \nmay be already finished.\nInitializing \"+    s->requests[i].ret = -EIO;\" is enough.\n\n>\n>> } else {\n>> ret = s->requests[i].ret;\n>> @@ -218,11 +212,6 @@ static int nbd_co_receive_reply(NBDClientSession *s, NBDRequest *request)\n>> \n>> s->requests[i].coroutine = NULL;\n>> \n>> - /* Kick the read_reply_co to get the next reply. */\n>> - if (s->read_reply_co) {\n>> - aio_co_wake(s->read_reply_co);\n>> - }\n>> -\n>> qemu_co_mutex_lock(&s->send_mutex);\n>> s->in_flight--;\n>> qemu_co_queue_next(&s->free_sema);\n>> @@ -364,6 +353,8 @@ void nbd_client_close(BlockDriverState *bs)\n>> \n>> nbd_send_request(client->ioc, &request);\n>> \n>> + client->quit = true;\n> Previously, client->quit was only set when detecting a broken server,\n> now it is also set for a clean exit. Do we need to change any\n> documentation of the field?\n\nIt has documentation?\n\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxJKw16SYz9sPk\n\tfor ;\n\tTue, 19 Sep 2017 20:01:06 +1000 (AEST)","from localhost ([::1]:41112 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duFKy-0004ex-Ky\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 06:01:04 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:35641)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duFKW-0004cQ-Q1\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:00:42 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duFKQ-00012q-Vq\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:00:36 -0400","from mailhub.sw.ru ([195.214.232.25]:5811 helo=relay.sw.ru)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duFKQ-0000zm-Jf\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:00:30 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v8JA0J1o013015;\n\tTue, 19 Sep 2017 13:00:19 +0300 (MSK)"],"To":"Eric Blake , qemu-block@nongnu.org,\n\tqemu-devel@nongnu.org","References":"<20170918135935.255591-1-vsementsov@virtuozzo.com>\n\t<20170918135935.255591-8-vsementsov@virtuozzo.com>\n\t<0f9ffc3d-02e1-fde8-67ac-0f62101f1204@redhat.com>","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"<17fdbc1d-05db-ee91-6b13-cac9d3719c21@virtuozzo.com>","Date":"Tue, 19 Sep 2017 13:00:19 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<0f9ffc3d-02e1-fde8-67ac-0f62101f1204@redhat.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"quoted-printable","X-MIME-Autoconverted":"from 8bit to quoted-printable by relay.sw.ru id\n\tv8JA0J1o013015","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Subject":"Re: [Qemu-devel] [PATCH v2 7/7] block/nbd-client: do not yield from\n\tnbd_read_reply_entry","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, pbonzini@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1771033,"web_url":"http://patchwork.ozlabs.org/comment/1771033/","msgid":"<5b4fc170-c315-0e86-443f-bd511a0dba84@redhat.com>","list_archive_url":null,"date":"2017-09-19T13:45:06","subject":"Re: [Qemu-devel] [PATCH v2 7/7] block/nbd-client: do not yield from\n\tnbd_read_reply_entry","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/19/2017 05:00 AM, Vladimir Sementsov-Ogievskiy wrote:\n> 19.09.2017 01:36, Eric Blake wrote:\n>> On 09/18/2017 08:59 AM, Vladimir Sementsov-Ogievskiy wrote:\n>>> Refactor nbd client to not yield from nbd_read_reply_entry. It's\n>>> possible now as all reading is done in nbd_read_reply_entry and\n>>> all request-related data is stored in per-request s->requests[i].\n>>>\n>>> We need here some additional work with s->requests[i].ret and\n>>> s->quit to not fail requests on normal disconnet and to not report\n>>> reading errors on normal disconnect.\n>>>\n\n>>> @@ -364,6 +353,8 @@ void nbd_client_close(BlockDriverState *bs)\n>>>         nbd_send_request(client->ioc, &request);\n>>>   +    client->quit = true;\n>> Previously, client->quit was only set when detecting a broken server,\n>> now it is also set for a clean exit.  Do we need to change any\n>> documentation of the field?\n> \n> It has documentation?\n\nTouche. But it probably should, especially if we are changing its\nsemantics, to make it easier to understand from looking at the struct\nwhat semantics to expect.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=eblake@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxPLB2plbz9sPm\n\tfor ;\n\tTue, 19 Sep 2017 23:46:41 +1000 (AEST)","from localhost ([::1]:43000 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duIrH-0007lI-TR\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 09:46:39 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:55736)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duIq2-0007F7-Tu\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 09:45:25 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duIpw-0003Ip-W2\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 09:45:22 -0400","from mx1.redhat.com ([209.132.183.28]:44190)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duIpp-0003Cw-Sm; Tue, 19 Sep 2017 09:45:10 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id BBAF0C04BD2C;\n\tTue, 19 Sep 2017 13:45:08 +0000 (UTC)","from [10.10.124.97] (ovpn-124-97.rdu2.redhat.com [10.10.124.97])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 1A06A600C2;\n\tTue, 19 Sep 2017 13:45:06 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com BBAF0C04BD2C","To":"Vladimir Sementsov-Ogievskiy ,\n\tqemu-block@nongnu.org, qemu-devel@nongnu.org","References":"<20170918135935.255591-1-vsementsov@virtuozzo.com>\n\t<20170918135935.255591-8-vsementsov@virtuozzo.com>\n\t<0f9ffc3d-02e1-fde8-67ac-0f62101f1204@redhat.com>\n\t<17fdbc1d-05db-ee91-6b13-cac9d3719c21@virtuozzo.com>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"<5b4fc170-c315-0e86-443f-bd511a0dba84@redhat.com>","Date":"Tue, 19 Sep 2017 08:45:06 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<17fdbc1d-05db-ee91-6b13-cac9d3719c21@virtuozzo.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"RpLm07la3r0P4GKvqCckEg6Gt04P5LOrt\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.31]);\n\tTue, 19 Sep 2017 13:45:08 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH v2 7/7] block/nbd-client: do not yield from\n\tnbd_read_reply_entry","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, pbonzini@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]