[{"id":1770262,"web_url":"http://patchwork.ozlabs.org/comment/1770262/","msgid":"","list_archive_url":null,"date":"2017-09-18T15:43:12","subject":"Re: [Qemu-devel] [PATCH v2 3/7] block/nbd-client: refactor reading\n\treply","submitter":{"id":2701,"url":"http://patchwork.ozlabs.org/api/people/2701/","name":"Paolo Bonzini","email":"pbonzini@redhat.com"},"content":"On 18/09/2017 15:59, Vladimir Sementsov-Ogievskiy wrote:\n> Read the whole reply in one place - in nbd_read_reply_entry.\n> \n> Signed-off-by: Vladimir Sementsov-Ogievskiy \n> ---\n> block/nbd-client.h | 1 +\n> block/nbd-client.c | 42 ++++++++++++++++++++++++------------------\n> 2 files changed, 25 insertions(+), 18 deletions(-)\n> \n> diff --git a/block/nbd-client.h b/block/nbd-client.h\n> index b1900e6a6d..3f97d31013 100644\n> --- a/block/nbd-client.h\n> +++ b/block/nbd-client.h\n> @@ -21,6 +21,7 @@ typedef struct {\n> Coroutine *coroutine;\n> bool receiving; /* waiting for read_reply_co? */\n> NBDRequest *request;\n> + QEMUIOVector *qiov;\n> } NBDClientRequest;\n> \n> typedef struct NBDClientSession {\n> diff --git a/block/nbd-client.c b/block/nbd-client.c\n> index 5f241ecc22..f7b642f079 100644\n> --- a/block/nbd-client.c\n> +++ b/block/nbd-client.c\n> @@ -98,6 +98,21 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)\n> break;\n> }\n> \n> + if (s->reply.error == 0 &&\n> + s->requests[i].request->type == NBD_CMD_READ)\n> + {\n> + QEMUIOVector *qiov = s->requests[i].qiov;\n> + assert(qiov != NULL);\n> + assert(s->requests[i].request->len ==\n> + iov_size(qiov->iov, qiov->niov));\n> + if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,\n> + NULL) < 0)\n> + {\n> + s->reply.error = EIO;\n> + break;\n> + }\n> + }\n\nI am not sure this is an improvement. In principle you could have\ncommands that read replies a bit at a time without using a QEMUIOVector.\n\nPaolo\n\n> /* We're woken up again by the request itself. Note that there\n> * is no race between yielding and reentering read_reply_co. This\n> * is because:\n> @@ -118,6 +133,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)\n> s->read_reply_co = NULL;\n> }\n> \n> +/* qiov should be provided for READ and WRITE */\n> static int nbd_co_send_request(BlockDriverState *bs,\n> NBDRequest *request,\n> QEMUIOVector *qiov)\n> @@ -145,6 +161,7 @@ static int nbd_co_send_request(BlockDriverState *bs,\n> \n> request->handle = INDEX_TO_HANDLE(s, i);\n> s->requests[i].request = request;\n> + s->requests[i].qiov = qiov;\n> \n> if (s->quit) {\n> rc = -EIO;\n> @@ -155,7 +172,8 @@ static int nbd_co_send_request(BlockDriverState *bs,\n> goto err;\n> }\n> \n> - if (qiov) {\n> + if (s->requests[i].request->type == NBD_CMD_WRITE) {\n> + assert(qiov);\n> qio_channel_set_cork(s->ioc, true);\n> rc = nbd_send_request(s->ioc, request);\n> if (rc >= 0 && !s->quit) {\n> @@ -181,9 +199,7 @@ err:\n> return rc;\n> }\n> \n> -static int nbd_co_receive_reply(NBDClientSession *s,\n> - NBDRequest *request,\n> - QEMUIOVector *qiov)\n> +static int nbd_co_receive_reply(NBDClientSession *s, NBDRequest *request)\n> {\n> int ret;\n> int i = HANDLE_TO_INDEX(s, request->handle);\n> @@ -197,14 +213,6 @@ static int nbd_co_receive_reply(NBDClientSession *s,\n> } else {\n> assert(s->reply.handle == request->handle);\n> ret = -s->reply.error;\n> - if (qiov && s->reply.error == 0) {\n> - assert(request->len == iov_size(qiov->iov, qiov->niov));\n> - if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,\n> - NULL) < 0) {\n> - ret = -EIO;\n> - s->quit = true;\n> - }\n> - }\n> \n> /* Tell the read handler to read another header. */\n> s->reply.handle = 0;\n> @@ -232,16 +240,14 @@ static int nbd_co_request(BlockDriverState *bs,\n> NBDClientSession *client = nbd_get_client_session(bs);\n> int ret;\n> \n> - assert(!qiov || request->type == NBD_CMD_WRITE ||\n> - request->type == NBD_CMD_READ);\n> - ret = nbd_co_send_request(bs, request,\n> - request->type == NBD_CMD_WRITE ? qiov : NULL);\n> + assert((qiov == NULL) !=\n> + (request->type == NBD_CMD_WRITE || request->type == NBD_CMD_READ));\n> + ret = nbd_co_send_request(bs, request, qiov);\n> if (ret < 0) {\n> return ret;\n> }\n> \n> - return nbd_co_receive_reply(client, request,\n> - request->type == NBD_CMD_READ ? qiov : NULL);\n> + return nbd_co_receive_reply(client, request);\n> }\n> \n> int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,\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=)","ext-mx02.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx02.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=pbonzini@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 3xwqzy3f0qz9s06\n\tfor ;\n\tTue, 19 Sep 2017 01:43:58 +1000 (AEST)","from localhost ([::1]:37383 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dtyDE-0007ED-Fy\n\tfor incoming@patchwork.ozlabs.org; Mon, 18 Sep 2017 11:43:56 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:56557)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dtyCg-0007AA-W7\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 11:43:28 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dtyCf-0005tr-R9\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 11:43:23 -0400","from mx1.redhat.com ([209.132.183.28]:39626)\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 1dtyCa-0005qE-Qh; Mon, 18 Sep 2017 11:43:17 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\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 C5FDF883BD;\n\tMon, 18 Sep 2017 15:43:15 +0000 (UTC)","from [10.32.181.85] (unknown [10.32.181.85])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 0B9AB6060A;\n\tMon, 18 Sep 2017 15:43:13 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com C5FDF883BD","To":"Vladimir Sementsov-Ogievskiy ,\n\tqemu-block@nongnu.org, qemu-devel@nongnu.org","References":"<20170918135935.255591-1-vsementsov@virtuozzo.com>\n\t<20170918135935.255591-4-vsementsov@virtuozzo.com>","From":"Paolo Bonzini ","Message-ID":"","Date":"Mon, 18 Sep 2017 17:43:12 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170918135935.255591-4-vsementsov@virtuozzo.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.26]);\n\tMon, 18 Sep 2017 15:43:15 +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","Subject":"Re: [Qemu-devel] [PATCH v2 3/7] block/nbd-client: refactor reading\n\treply","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, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1770272,"web_url":"http://patchwork.ozlabs.org/comment/1770272/","msgid":"<7df363e9-4f05-6b90-5ee9-dd848c52857d@redhat.com>","list_archive_url":null,"date":"2017-09-18T15:54:19","subject":"Re: [Qemu-devel] [PATCH v2 3/7] block/nbd-client: refactor reading\n\treply","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/18/2017 10:43 AM, Paolo Bonzini wrote:\n> On 18/09/2017 15:59, Vladimir Sementsov-Ogievskiy wrote:\n>> Read the whole reply in one place - in nbd_read_reply_entry.\n>>\n>> Signed-off-by: Vladimir Sementsov-Ogievskiy \n>> ---\n>> block/nbd-client.h | 1 +\n>> block/nbd-client.c | 42 ++++++++++++++++++++++++------------------\n>> 2 files changed, 25 insertions(+), 18 deletions(-)\n>>\n\n> \n> I am not sure this is an improvement. In principle you could have\n> commands that read replies a bit at a time without using a QEMUIOVector.\n\nRight now we don't, but the most likely point where this would be an\nissue is the fact that we want to implement structured replies (the\nserver can send more than one response to a single request from the\nclient) in order to then implement block status queries (where the\nserver can send piecemeal information in response to a query, and the\nclient could very easily want to handle information as it comes in\nrather than waiting for the entire server response, especially if the\namount of information returned by the server is not known a priori by\nthe client, the way the length is known in advance for NBD_CMD_READ, but\ninstead learned partway through the reply). I guess the question\nbecomes a matter of whether we are over-constraining future additions by\nmaking this refactoring, or whether we can still implement block status\nqueries using a single QEMUIOVector.","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-mx03.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx03.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 3xwrDV1Zrgz9s5L\n\tfor ;\n\tTue, 19 Sep 2017 01:54:50 +1000 (AEST)","from localhost ([::1]:37435 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dtyNk-0004OG-Bn\n\tfor incoming@patchwork.ozlabs.org; Mon, 18 Sep 2017 11:54:48 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:34894)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dtyNQ-0004Mi-4e\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 11:54:29 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dtyNP-0004uv-9X\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 11:54:28 -0400","from mx1.redhat.com ([209.132.183.28]:54150)\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 1dtyNK-0004sp-Qf; Mon, 18 Sep 2017 11:54:23 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\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 BD37A7E431;\n\tMon, 18 Sep 2017 15:54:21 +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 7F3D918A5A;\n\tMon, 18 Sep 2017 15:54:20 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com BD37A7E431","To":"Paolo Bonzini ,\n\tVladimir Sementsov-Ogievskiy ,\n\tqemu-block@nongnu.org, qemu-devel@nongnu.org","References":"<20170918135935.255591-1-vsementsov@virtuozzo.com>\n\t<20170918135935.255591-4-vsementsov@virtuozzo.com>\n\t","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"<7df363e9-4f05-6b90-5ee9-dd848c52857d@redhat.com>","Date":"Mon, 18 Sep 2017 10:54:19 -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":"","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"CFCtkQFhvADkNpIhaDlUgT9sxSkdacHVv\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.27]);\n\tMon, 18 Sep 2017 15:54:21 +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 3/7] block/nbd-client: refactor reading\n\treply","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, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1770786,"web_url":"http://patchwork.ozlabs.org/comment/1770786/","msgid":"<577e29c4-5187-8c6f-aacc-c77282959fdd@virtuozzo.com>","list_archive_url":null,"date":"2017-09-19T09:25:09","subject":"Re: [Qemu-devel] [PATCH v2 3/7] block/nbd-client: refactor reading\n\treply","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"18.09.2017 18:43, Paolo Bonzini wrote:\n> On 18/09/2017 15:59, Vladimir Sementsov-Ogievskiy wrote:\n>> Read the whole reply in one place - in nbd_read_reply_entry.\n>>\n>> Signed-off-by: Vladimir Sementsov-Ogievskiy \n>> ---\n>> block/nbd-client.h | 1 +\n>> block/nbd-client.c | 42 ++++++++++++++++++++++++------------------\n>> 2 files changed, 25 insertions(+), 18 deletions(-)\n>>\n>> diff --git a/block/nbd-client.h b/block/nbd-client.h\n>> index b1900e6a6d..3f97d31013 100644\n>> --- a/block/nbd-client.h\n>> +++ b/block/nbd-client.h\n>> @@ -21,6 +21,7 @@ typedef struct {\n>> Coroutine *coroutine;\n>> bool receiving; /* waiting for read_reply_co? */\n>> NBDRequest *request;\n>> + QEMUIOVector *qiov;\n>> } NBDClientRequest;\n>> \n>> typedef struct NBDClientSession {\n>> diff --git a/block/nbd-client.c b/block/nbd-client.c\n>> index 5f241ecc22..f7b642f079 100644\n>> --- a/block/nbd-client.c\n>> +++ b/block/nbd-client.c\n>> @@ -98,6 +98,21 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)\n>> break;\n>> }\n>> \n>> + if (s->reply.error == 0 &&\n>> + s->requests[i].request->type == NBD_CMD_READ)\n>> + {\n>> + QEMUIOVector *qiov = s->requests[i].qiov;\n>> + assert(qiov != NULL);\n>> + assert(s->requests[i].request->len ==\n>> + iov_size(qiov->iov, qiov->niov));\n>> + if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,\n>> + NULL) < 0)\n>> + {\n>> + s->reply.error = EIO;\n>> + break;\n>> + }\n>> + }\n> I am not sure this is an improvement. In principle you could have\n> commands that read replies a bit at a time without using a QEMUIOVector.\n>\n> Paolo\n\nHm, what do you mean? In this patch I don't change \"how do we read it\", \nI only move the reading code to one coroutine, to make it simple to \nextend it in future (block status, etc). nbd_read_reply_enty has all \ninformation it need (s->requests[i].request) to handle the whole reply.. \nIt's an improvement, because it leads to separating reply_entry \ncoroutine - it don't need any synchronization (yield/wake) more with \nrequest coroutines.\n\n>\n>> /* We're woken up again by the request itself. Note that there\n>> * is no race between yielding and reentering read_reply_co. This\n>> * is because:\n>> @@ -118,6 +133,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)\n>> s->read_reply_co = NULL;\n>> }\n>> \n>> +/* qiov should be provided for READ and WRITE */\n>> static int nbd_co_send_request(BlockDriverState *bs,\n>> NBDRequest *request,\n>> QEMUIOVector *qiov)\n>> @@ -145,6 +161,7 @@ static int nbd_co_send_request(BlockDriverState *bs,\n>> \n>> request->handle = INDEX_TO_HANDLE(s, i);\n>> s->requests[i].request = request;\n>> + s->requests[i].qiov = qiov;\n>> \n>> if (s->quit) {\n>> rc = -EIO;\n>> @@ -155,7 +172,8 @@ static int nbd_co_send_request(BlockDriverState *bs,\n>> goto err;\n>> }\n>> \n>> - if (qiov) {\n>> + if (s->requests[i].request->type == NBD_CMD_WRITE) {\n>> + assert(qiov);\n>> qio_channel_set_cork(s->ioc, true);\n>> rc = nbd_send_request(s->ioc, request);\n>> if (rc >= 0 && !s->quit) {\n>> @@ -181,9 +199,7 @@ err:\n>> return rc;\n>> }\n>> \n>> -static int nbd_co_receive_reply(NBDClientSession *s,\n>> - NBDRequest *request,\n>> - QEMUIOVector *qiov)\n>> +static int nbd_co_receive_reply(NBDClientSession *s, NBDRequest *request)\n>> {\n>> int ret;\n>> int i = HANDLE_TO_INDEX(s, request->handle);\n>> @@ -197,14 +213,6 @@ static int nbd_co_receive_reply(NBDClientSession *s,\n>> } else {\n>> assert(s->reply.handle == request->handle);\n>> ret = -s->reply.error;\n>> - if (qiov && s->reply.error == 0) {\n>> - assert(request->len == iov_size(qiov->iov, qiov->niov));\n>> - if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,\n>> - NULL) < 0) {\n>> - ret = -EIO;\n>> - s->quit = true;\n>> - }\n>> - }\n>> \n>> /* Tell the read handler to read another header. */\n>> s->reply.handle = 0;\n>> @@ -232,16 +240,14 @@ static int nbd_co_request(BlockDriverState *bs,\n>> NBDClientSession *client = nbd_get_client_session(bs);\n>> int ret;\n>> \n>> - assert(!qiov || request->type == NBD_CMD_WRITE ||\n>> - request->type == NBD_CMD_READ);\n>> - ret = nbd_co_send_request(bs, request,\n>> - request->type == NBD_CMD_WRITE ? qiov : NULL);\n>> + assert((qiov == NULL) !=\n>> + (request->type == NBD_CMD_WRITE || request->type == NBD_CMD_READ));\n>> + ret = nbd_co_send_request(bs, request, qiov);\n>> if (ret < 0) {\n>> return ret;\n>> }\n>> \n>> - return nbd_co_receive_reply(client, request,\n>> - request->type == NBD_CMD_READ ? qiov : NULL);\n>> + return nbd_co_receive_reply(client, request);\n>> }\n>> \n>> int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,\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 3xxHYJ67fXz9ryr\n\tfor ;\n\tTue, 19 Sep 2017 19:25:56 +1000 (AEST)","from localhost ([::1]:40975 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duEmw-0007t0-Vy\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 05:25:55 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:51894)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duEmX-0007qV-OP\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 05:25:35 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duEmT-0004ty-1o\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 05:25:29 -0400","from mailhub.sw.ru ([195.214.232.25]:23699 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 1duEmS-0004p6-L3\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 05:25:24 -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 v8J9P9db030112;\n\tTue, 19 Sep 2017 12:25:09 +0300 (MSK)"],"To":"Paolo Bonzini , qemu-block@nongnu.org,\n\tqemu-devel@nongnu.org","References":"<20170918135935.255591-1-vsementsov@virtuozzo.com>\n\t<20170918135935.255591-4-vsementsov@virtuozzo.com>\n\t","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"<577e29c4-5187-8c6f-aacc-c77282959fdd@virtuozzo.com>","Date":"Tue, 19 Sep 2017 12:25:09 +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":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","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 3/7] block/nbd-client: refactor reading\n\treply","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, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1770810,"web_url":"http://patchwork.ozlabs.org/comment/1770810/","msgid":"<26582eb0-1db3-3aac-3652-44e3fbb1e49c@redhat.com>","list_archive_url":null,"date":"2017-09-19T10:01:55","subject":"Re: [Qemu-devel] [PATCH v2 3/7] block/nbd-client: refactor reading\n\treply","submitter":{"id":2701,"url":"http://patchwork.ozlabs.org/api/people/2701/","name":"Paolo Bonzini","email":"pbonzini@redhat.com"},"content":"On 19/09/2017 11:25, Vladimir Sementsov-Ogievskiy wrote:\n> 18.09.2017 18:43, Paolo Bonzini wrote:\n>> On 18/09/2017 15:59, Vladimir Sementsov-Ogievskiy wrote:\n>>> Read the whole reply in one place - in nbd_read_reply_entry.\n>>>\n>>> Signed-off-by: Vladimir Sementsov-Ogievskiy \n>>> ---\n>>> block/nbd-client.h | 1 +\n>>> block/nbd-client.c | 42 ++++++++++++++++++++++++------------------\n>>> 2 files changed, 25 insertions(+), 18 deletions(-)\n>>>\n>>> diff --git a/block/nbd-client.h b/block/nbd-client.h\n>>> index b1900e6a6d..3f97d31013 100644\n>>> --- a/block/nbd-client.h\n>>> +++ b/block/nbd-client.h\n>>> @@ -21,6 +21,7 @@ typedef struct {\n>>> Coroutine *coroutine;\n>>> bool receiving; /* waiting for read_reply_co? */\n>>> NBDRequest *request;\n>>> + QEMUIOVector *qiov;\n>>> } NBDClientRequest;\n>>> typedef struct NBDClientSession {\n>>> diff --git a/block/nbd-client.c b/block/nbd-client.c\n>>> index 5f241ecc22..f7b642f079 100644\n>>> --- a/block/nbd-client.c\n>>> +++ b/block/nbd-client.c\n>>> @@ -98,6 +98,21 @@ static coroutine_fn void nbd_read_reply_entry(void\n>>> *opaque)\n>>> break;\n>>> }\n>>> + if (s->reply.error == 0 &&\n>>> + s->requests[i].request->type == NBD_CMD_READ)\n>>> + {\n>>> + QEMUIOVector *qiov = s->requests[i].qiov;\n>>> + assert(qiov != NULL);\n>>> + assert(s->requests[i].request->len ==\n>>> + iov_size(qiov->iov, qiov->niov));\n>>> + if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,\n>>> + NULL) < 0)\n>>> + {\n>>> + s->reply.error = EIO;\n>>> + break;\n>>> + }\n>>> + }\n>> I am not sure this is an improvement. In principle you could have\n>> commands that read replies a bit at a time without using a QEMUIOVector.\n>>\n>> Paolo\n> \n> Hm, what do you mean? In this patch I don't change \"how do we read it\",\n> I only move the reading code to one coroutine, to make it simple to\n> extend it in future (block status, etc).\n\nI disagree that it is easier to extend it in the future. If some\ncommands in the future need a different \"how do we read it\" (e.g. for\nstructured reply), nbd_read_reply_entry may not have all the information\nit needs anymore.\n\nIn fact, you're pushing knowledge of the commands from nbd_co_request to\nnbd_read_reply_entry:\n\n+ if (s->reply.error == 0 &&\n+ s->requests[i].request->type == NBD_CMD_READ)\n\nand knowledge of NBD_CMD_READ simply doesn't belong there. So there is\nan example of that right now, it is not some arbitrary bad thing that\ncould happen in the future.\n\nPaolo\n\n\n> nbd_read_reply_enty has all\n> information it need (s->requests[i].request) to handle the whole reply..\n> It's an improvement, because it leads to separating reply_entry\n> coroutine - it don't need any synchronization (yield/wake) more with\n> request coroutines.\n> \n>>\n>>> /* We're woken up again by the request itself. Note that\n>>> there\n>>> * is no race between yielding and reentering\n>>> read_reply_co. This\n>>> * is because:\n>>> @@ -118,6 +133,7 @@ static coroutine_fn void\n>>> nbd_read_reply_entry(void *opaque)\n>>> s->read_reply_co = NULL;\n>>> }\n>>> +/* qiov should be provided for READ and WRITE */\n>>> static int nbd_co_send_request(BlockDriverState *bs,\n>>> NBDRequest *request,\n>>> QEMUIOVector *qiov)\n>>> @@ -145,6 +161,7 @@ static int nbd_co_send_request(BlockDriverState *bs,\n>>> request->handle = INDEX_TO_HANDLE(s, i);\n>>> s->requests[i].request = request;\n>>> + s->requests[i].qiov = qiov;\n>>> if (s->quit) {\n>>> rc = -EIO;\n>>> @@ -155,7 +172,8 @@ static int nbd_co_send_request(BlockDriverState *bs,\n>>> goto err;\n>>> }\n>>> - if (qiov) {\n>>> + if (s->requests[i].request->type == NBD_CMD_WRITE) {\n>>> + assert(qiov);\n>>> qio_channel_set_cork(s->ioc, true);\n>>> rc = nbd_send_request(s->ioc, request);\n>>> if (rc >= 0 && !s->quit) {\n>>> @@ -181,9 +199,7 @@ err:\n>>> return rc;\n>>> }\n>>> -static int nbd_co_receive_reply(NBDClientSession *s,\n>>> - NBDRequest *request,\n>>> - QEMUIOVector *qiov)\n>>> +static int nbd_co_receive_reply(NBDClientSession *s, NBDRequest\n>>> *request)\n>>> {\n>>> int ret;\n>>> int i = HANDLE_TO_INDEX(s, request->handle);\n>>> @@ -197,14 +213,6 @@ static int nbd_co_receive_reply(NBDClientSession\n>>> *s,\n>>> } else {\n>>> assert(s->reply.handle == request->handle);\n>>> ret = -s->reply.error;\n>>> - if (qiov && s->reply.error == 0) {\n>>> - assert(request->len == iov_size(qiov->iov, qiov->niov));\n>>> - if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,\n>>> - NULL) < 0) {\n>>> - ret = -EIO;\n>>> - s->quit = true;\n>>> - }\n>>> - }\n>>> /* Tell the read handler to read another header. */\n>>> s->reply.handle = 0;\n>>> @@ -232,16 +240,14 @@ static int nbd_co_request(BlockDriverState *bs,\n>>> NBDClientSession *client = nbd_get_client_session(bs);\n>>> int ret;\n>>> - assert(!qiov || request->type == NBD_CMD_WRITE ||\n>>> - request->type == NBD_CMD_READ);\n>>> - ret = nbd_co_send_request(bs, request,\n>>> - request->type == NBD_CMD_WRITE ? qiov\n>>> : NULL);\n>>> + assert((qiov == NULL) !=\n>>> + (request->type == NBD_CMD_WRITE || request->type ==\n>>> NBD_CMD_READ));\n>>> + ret = nbd_co_send_request(bs, request, qiov);\n>>> if (ret < 0) {\n>>> return ret;\n>>> }\n>>> - return nbd_co_receive_reply(client, request,\n>>> - request->type == NBD_CMD_READ ? qiov\n>>> : NULL);\n>>> + return nbd_co_receive_reply(client, request);\n>>> }\n>>> int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,\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=)","ext-mx01.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx01.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=pbonzini@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 3xxJMp1cXgz9s7F\n\tfor ;\n\tTue, 19 Sep 2017 20:02:46 +1000 (AEST)","from localhost ([::1]:41150 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duFMa-0005bO-D6\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 06:02:44 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36743)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duFMA-0005ac-RS\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:02:23 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duFM4-0001vs-TF\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:02:18 -0400","from mx1.redhat.com ([209.132.183.28]:40044)\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 1duFLt-0001ou-P0; Tue, 19 Sep 2017 06:02:01 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\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 C034781E1D;\n\tTue, 19 Sep 2017 10:02:00 +0000 (UTC)","from [10.36.117.61] (ovpn-117-61.ams2.redhat.com [10.36.117.61])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 7B2EB60618;\n\tTue, 19 Sep 2017 10:01:58 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com C034781E1D","To":"Vladimir Sementsov-Ogievskiy ,\n\tqemu-block@nongnu.org, qemu-devel@nongnu.org","References":"<20170918135935.255591-1-vsementsov@virtuozzo.com>\n\t<20170918135935.255591-4-vsementsov@virtuozzo.com>\n\t\n\t<577e29c4-5187-8c6f-aacc-c77282959fdd@virtuozzo.com>","From":"Paolo Bonzini ","Message-ID":"<26582eb0-1db3-3aac-3652-44e3fbb1e49c@redhat.com>","Date":"Tue, 19 Sep 2017 12:01:55 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<577e29c4-5187-8c6f-aacc-c77282959fdd@virtuozzo.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.25]);\n\tTue, 19 Sep 2017 10:02:00 +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","Subject":"Re: [Qemu-devel] [PATCH v2 3/7] block/nbd-client: refactor reading\n\treply","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, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1770872,"web_url":"http://patchwork.ozlabs.org/comment/1770872/","msgid":"<5fff43cf-717d-2d64-f51d-57f116888921@virtuozzo.com>","list_archive_url":null,"date":"2017-09-19T11:03:47","subject":"Re: [Qemu-devel] [PATCH v2 3/7] block/nbd-client: refactor reading\n\treply","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"19.09.2017 13:01, Paolo Bonzini wrote:\n> On 19/09/2017 11:25, Vladimir Sementsov-Ogievskiy wrote:\n>> 18.09.2017 18:43, Paolo Bonzini wrote:\n>>> On 18/09/2017 15:59, Vladimir Sementsov-Ogievskiy wrote:\n>>>> Read the whole reply in one place - in nbd_read_reply_entry.\n>>>>\n>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy \n>>>> ---\n>>>> block/nbd-client.h | 1 +\n>>>> block/nbd-client.c | 42 ++++++++++++++++++++++++------------------\n>>>> 2 files changed, 25 insertions(+), 18 deletions(-)\n>>>>\n>>>> diff --git a/block/nbd-client.h b/block/nbd-client.h\n>>>> index b1900e6a6d..3f97d31013 100644\n>>>> --- a/block/nbd-client.h\n>>>> +++ b/block/nbd-client.h\n>>>> @@ -21,6 +21,7 @@ typedef struct {\n>>>> Coroutine *coroutine;\n>>>> bool receiving; /* waiting for read_reply_co? */\n>>>> NBDRequest *request;\n>>>> + QEMUIOVector *qiov;\n>>>> } NBDClientRequest;\n>>>> typedef struct NBDClientSession {\n>>>> diff --git a/block/nbd-client.c b/block/nbd-client.c\n>>>> index 5f241ecc22..f7b642f079 100644\n>>>> --- a/block/nbd-client.c\n>>>> +++ b/block/nbd-client.c\n>>>> @@ -98,6 +98,21 @@ static coroutine_fn void nbd_read_reply_entry(void\n>>>> *opaque)\n>>>> break;\n>>>> }\n>>>> + if (s->reply.error == 0 &&\n>>>> + s->requests[i].request->type == NBD_CMD_READ)\n>>>> + {\n>>>> + QEMUIOVector *qiov = s->requests[i].qiov;\n>>>> + assert(qiov != NULL);\n>>>> + assert(s->requests[i].request->len ==\n>>>> + iov_size(qiov->iov, qiov->niov));\n>>>> + if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,\n>>>> + NULL) < 0)\n>>>> + {\n>>>> + s->reply.error = EIO;\n>>>> + break;\n>>>> + }\n>>>> + }\n>>> I am not sure this is an improvement. In principle you could have\n>>> commands that read replies a bit at a time without using a QEMUIOVector.\n>>>\n>>> Paolo\n>> Hm, what do you mean? In this patch I don't change \"how do we read it\",\n>> I only move the reading code to one coroutine, to make it simple to\n>> extend it in future (block status, etc).\n> I disagree that it is easier to extend it in the future. If some\n> commands in the future need a different \"how do we read it\" (e.g. for\n> structured reply), nbd_read_reply_entry may not have all the information\n> it needs anymore.\n\nhow is it possible? all information is in requests[i].\n\n>\n> In fact, you're pushing knowledge of the commands from nbd_co_request to\n> nbd_read_reply_entry:\n>\n> + if (s->reply.error == 0 &&\n> + s->requests[i].request->type == NBD_CMD_READ)\n>\n> and knowledge of NBD_CMD_READ simply doesn't belong there. So there is\n> an example of that right now, it is not some arbitrary bad thing that\n> could happen in the future.\n>\n> Paolo\n\nI can't agree that my point of view is wrong, it's just different.\nYou want commands, reading from socket, each knows what it should read.\nI want the receiver - one sequential reader, that can read all kinds of \nreplies and just\nwake requesting coroutines when all reading is done. For me it looks simpler\nthan switching.\n\nWe can add reading callbacks for commands which have some payload, like\ns->requests[i].read_payload(ioc, request) or may be \ns->requests[i].request->read_payload(...),\nand call them from reply_entry instead of if (s->... == NBD_CMD_READ).\n\nHow do you see the refactoring for introducing structured reply?\n\n\n>\n>\n>> nbd_read_reply_enty has all\n>> information it need (s->requests[i].request) to handle the whole reply..\n>> It's an improvement, because it leads to separating reply_entry\n>> coroutine - it don't need any synchronization (yield/wake) more with\n>> request coroutines.\n>>\n>>>> /* We're woken up again by the request itself. Note that\n>>>> there\n>>>> * is no race between yielding and reentering\n>>>> read_reply_co. This\n>>>> * is because:\n>>>> @@ -118,6 +133,7 @@ static coroutine_fn void\n>>>> nbd_read_reply_entry(void *opaque)\n>>>> s->read_reply_co = NULL;\n>>>> }\n>>>> +/* qiov should be provided for READ and WRITE */\n>>>> static int nbd_co_send_request(BlockDriverState *bs,\n>>>> NBDRequest *request,\n>>>> QEMUIOVector *qiov)\n>>>> @@ -145,6 +161,7 @@ static int nbd_co_send_request(BlockDriverState *bs,\n>>>> request->handle = INDEX_TO_HANDLE(s, i);\n>>>> s->requests[i].request = request;\n>>>> + s->requests[i].qiov = qiov;\n>>>> if (s->quit) {\n>>>> rc = -EIO;\n>>>> @@ -155,7 +172,8 @@ static int nbd_co_send_request(BlockDriverState *bs,\n>>>> goto err;\n>>>> }\n>>>> - if (qiov) {\n>>>> + if (s->requests[i].request->type == NBD_CMD_WRITE) {\n>>>> + assert(qiov);\n>>>> qio_channel_set_cork(s->ioc, true);\n>>>> rc = nbd_send_request(s->ioc, request);\n>>>> if (rc >= 0 && !s->quit) {\n>>>> @@ -181,9 +199,7 @@ err:\n>>>> return rc;\n>>>> }\n>>>> -static int nbd_co_receive_reply(NBDClientSession *s,\n>>>> - NBDRequest *request,\n>>>> - QEMUIOVector *qiov)\n>>>> +static int nbd_co_receive_reply(NBDClientSession *s, NBDRequest\n>>>> *request)\n>>>> {\n>>>> int ret;\n>>>> int i = HANDLE_TO_INDEX(s, request->handle);\n>>>> @@ -197,14 +213,6 @@ static int nbd_co_receive_reply(NBDClientSession\n>>>> *s,\n>>>> } else {\n>>>> assert(s->reply.handle == request->handle);\n>>>> ret = -s->reply.error;\n>>>> - if (qiov && s->reply.error == 0) {\n>>>> - assert(request->len == iov_size(qiov->iov, qiov->niov));\n>>>> - if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,\n>>>> - NULL) < 0) {\n>>>> - ret = -EIO;\n>>>> - s->quit = true;\n>>>> - }\n>>>> - }\n>>>> /* Tell the read handler to read another header. */\n>>>> s->reply.handle = 0;\n>>>> @@ -232,16 +240,14 @@ static int nbd_co_request(BlockDriverState *bs,\n>>>> NBDClientSession *client = nbd_get_client_session(bs);\n>>>> int ret;\n>>>> - assert(!qiov || request->type == NBD_CMD_WRITE ||\n>>>> - request->type == NBD_CMD_READ);\n>>>> - ret = nbd_co_send_request(bs, request,\n>>>> - request->type == NBD_CMD_WRITE ? qiov\n>>>> : NULL);\n>>>> + assert((qiov == NULL) !=\n>>>> + (request->type == NBD_CMD_WRITE || request->type ==\n>>>> NBD_CMD_READ));\n>>>> + ret = nbd_co_send_request(bs, request, qiov);\n>>>> if (ret < 0) {\n>>>> return ret;\n>>>> }\n>>>> - return nbd_co_receive_reply(client, request,\n>>>> - request->type == NBD_CMD_READ ? qiov\n>>>> : NULL);\n>>>> + return nbd_co_receive_reply(client, request);\n>>>> }\n>>>> int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,\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 3xxKlN4WZxz9rxj\n\tfor ;\n\tTue, 19 Sep 2017 21:04:48 +1000 (AEST)","from localhost ([::1]:41528 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duGKc-0007Fg-Ms\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 07:04:46 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:37284)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duGJz-0007Dx-B5\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 07:04:08 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duGJx-00017x-Gq\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 07:04:07 -0400","from mailhub.sw.ru ([195.214.232.25]:48498 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 1duGJx-00015c-13\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 07:04:05 -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 v8JB3lvg022710;\n\tTue, 19 Sep 2017 14:03:47 +0300 (MSK)"],"To":"Paolo Bonzini , qemu-block@nongnu.org,\n\tqemu-devel@nongnu.org","References":"<20170918135935.255591-1-vsementsov@virtuozzo.com>\n\t<20170918135935.255591-4-vsementsov@virtuozzo.com>\n\t\n\t<577e29c4-5187-8c6f-aacc-c77282959fdd@virtuozzo.com>\n\t<26582eb0-1db3-3aac-3652-44e3fbb1e49c@redhat.com>","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"<5fff43cf-717d-2d64-f51d-57f116888921@virtuozzo.com>","Date":"Tue, 19 Sep 2017 14:03:47 +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":"<26582eb0-1db3-3aac-3652-44e3fbb1e49c@redhat.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","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 3/7] block/nbd-client: refactor reading\n\treply","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, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1771008,"web_url":"http://patchwork.ozlabs.org/comment/1771008/","msgid":"<11ccb9c4-aecb-e82f-a520-50710dae8ea1@redhat.com>","list_archive_url":null,"date":"2017-09-19T12:50:30","subject":"Re: [Qemu-devel] [PATCH v2 3/7] block/nbd-client: refactor reading\n\treply","submitter":{"id":2701,"url":"http://patchwork.ozlabs.org/api/people/2701/","name":"Paolo Bonzini","email":"pbonzini@redhat.com"},"content":"On 19/09/2017 13:03, Vladimir Sementsov-Ogievskiy wrote:\n>>>\n>> I disagree that it is easier to extend it in the future. If some\n>> commands in the future need a different \"how do we read it\" (e.g. for\n>> structured reply), nbd_read_reply_entry may not have all the information\n>> it needs anymore.\n> \n> how is it possible? all information is in requests[i].\n\nIf you are okay with violating information hiding, then it is.\n\n>>\n>> In fact, you're pushing knowledge of the commands from nbd_co_request to\n>> nbd_read_reply_entry:\n>>\n>> + if (s->reply.error == 0 &&\n>> + s->requests[i].request->type == NBD_CMD_READ)\n>>\n>> and knowledge of NBD_CMD_READ simply doesn't belong there. So there is\n>> an example of that right now, it is not some arbitrary bad thing that\n>> could happen in the future.\n> \n> I can't agree that my point of view is wrong, it's just different.\n> You want commands, reading from socket, each knows what it should read.\n> I want the receiver - one sequential reader, that can read all kinds of\n> replies and just wake requesting coroutines when all reading is done.\n> For me it looks simpler than switching.\n\nIt may look simpler, but I think it goes against the principle of\ncoroutines which is to have related code in the same function. Here you\nhave the command function that takes care of sending the request payload\nbut not receiving the reply payload (except that for commands that\naren't as simple as read or write, it will have to _process_ the reply\npayload). What to do with the reply payload is in another function that\nhas to peek at the command in order to find out what to do. It doesn't\nseem like a better design, honestly.\n\nPaolo","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-mx06.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=pbonzini@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 3xxNzq1hVFz9sMN\n\tfor ;\n\tTue, 19 Sep 2017 23:30:47 +1000 (AEST)","from localhost ([::1]:42891 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duIbt-00048T-BO\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 09:30:45 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:45890)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duHzB-0004a1-0i\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 08:50:45 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duHz9-0003RR-Oc\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 08:50:44 -0400","from mx1.redhat.com ([209.132.183.28]:41022)\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 1duHz2-0003Lk-6f; Tue, 19 Sep 2017 08:50:36 -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 07ADB1F561;\n\tTue, 19 Sep 2017 12:50:35 +0000 (UTC)","from [10.36.117.61] (ovpn-117-61.ams2.redhat.com [10.36.117.61])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 0925419EF1;\n\tTue, 19 Sep 2017 12:50:31 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 07ADB1F561","To":"Vladimir Sementsov-Ogievskiy ,\n\tqemu-block@nongnu.org, qemu-devel@nongnu.org","References":"<20170918135935.255591-1-vsementsov@virtuozzo.com>\n\t<20170918135935.255591-4-vsementsov@virtuozzo.com>\n\t\n\t<577e29c4-5187-8c6f-aacc-c77282959fdd@virtuozzo.com>\n\t<26582eb0-1db3-3aac-3652-44e3fbb1e49c@redhat.com>\n\t<5fff43cf-717d-2d64-f51d-57f116888921@virtuozzo.com>","From":"Paolo Bonzini ","Message-ID":"<11ccb9c4-aecb-e82f-a520-50710dae8ea1@redhat.com>","Date":"Tue, 19 Sep 2017 14:50:30 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<5fff43cf-717d-2d64-f51d-57f116888921@virtuozzo.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","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.30]);\n\tTue, 19 Sep 2017 12:50:35 +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","Subject":"Re: [Qemu-devel] [PATCH v2 3/7] block/nbd-client: refactor reading\n\treply","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, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]