[{"id":1772798,"web_url":"http://patchwork.ozlabs.org/comment/1772798/","msgid":"<20170921132943.GE13703@lemon.lan>","list_archive_url":null,"date":"2017-09-21T13:29:43","subject":"Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end\n\tcallback","submitter":{"id":24872,"url":"http://patchwork.ozlabs.org/api/people/24872/","name":"Fam Zheng","email":"famz@redhat.com"},"content":"On Thu, 09/21 16:17, Manos Pitsidianakis wrote:\n> BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end\n\ns/bdrv_do_drain/bdrv_co_drain/\n\n> of the drain. The throttle driver (block/throttle.c) needs a way to mark the\n> end of the drain in order to toggle io_limits_disabled correctly, thus\n> bdrv_co_drain_end is needed.\n> \n> Signed-off-by: Manos Pitsidianakis \n> ---\n> include/block/block_int.h | 6 ++++++\n> block/io.c | 48 +++++++++++++++++++++++++++++++++--------------\n> 2 files changed, 40 insertions(+), 14 deletions(-)\n> \n> diff --git a/include/block/block_int.h b/include/block/block_int.h\n> index ba4c383393..21950cfda3 100644\n> --- a/include/block/block_int.h\n> +++ b/include/block/block_int.h\n> @@ -356,8 +356,14 @@ struct BlockDriver {\n> /**\n> * Drain and stop any internal sources of requests in the driver, and\n> * remain so until next I/O callback (e.g. bdrv_co_writev) is called.\n\nThis line needs update too, maybe:\n\n /**\n * bdrv_co_drain drains and stops any ... and remain so until\n * bdrv_co_drain_end is called.\n\n> + *\n> + * The callbacks are called at the beginning and ending of the drain\n> + * respectively. They should be implemented if the driver needs to e.g.\n\nAs implied by above change, should we explicitly require \"should both be\nimplemented\"? It may not be technically required, but I think is cleaner and\neasier to reason about.\n\n> + * manage scheduled I/O requests, or toggle an internal state. After an\n\ns/After an/After a/\n\n> + * bdrv_co_drain_end() invocation new requests will continue normally.\n> */\n> void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);\n> + void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);\n> \n> void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,\n> Error **errp);\n> diff --git a/block/io.c b/block/io.c\n> index 4378ae4c7d..b0a10ad3ef 100644\n> --- a/block/io.c\n> +++ b/block/io.c\n> @@ -153,6 +153,7 @@ typedef struct {\n> Coroutine *co;\n> BlockDriverState *bs;\n> bool done;\n> + bool begin;\n> } BdrvCoDrainData;\n> \n> static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)\n> @@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)\n> BdrvCoDrainData *data = opaque;\n> BlockDriverState *bs = data->bs;\n> \n> - bs->drv->bdrv_co_drain(bs);\n> + if (data->begin) {\n> + bs->drv->bdrv_co_drain(bs);\n> + } else {\n> + bs->drv->bdrv_co_drain_end(bs);\n> + }\n> \n> /* Set data->done before reading bs->wakeup. */\n> atomic_mb_set(&data->done, true);\n> bdrv_wakeup(bs);\n> }\n> \n> -static void bdrv_drain_invoke(BlockDriverState *bs)\n> +static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)\n> {\n> - BdrvCoDrainData data = { .bs = bs, .done = false };\n> + BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};\n> \n> - if (!bs->drv || !bs->drv->bdrv_co_drain) {\n> + if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||\n> + (!begin && !bs->drv->bdrv_co_drain_end)) {\n> return;\n> }\n> \n> @@ -180,15 +186,16 @@ static void bdrv_drain_invoke(BlockDriverState *bs)\n> BDRV_POLL_WHILE(bs, !data.done);\n> }\n> \n> -static bool bdrv_drain_recurse(BlockDriverState *bs)\n> +static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)\n> {\n> BdrvChild *child, *tmp;\n> bool waited;\n> \n> - waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);\n> -\n> /* Ensure any pending metadata writes are submitted to bs->file. */\n> - bdrv_drain_invoke(bs);\n> + bdrv_drain_invoke(bs, begin);\n> +\n> + /* Wait for drained requests to finish */\n> + waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);\n> \n> QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {\n> BlockDriverState *bs = child->bs;\n> @@ -205,7 +212,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)\n> */\n> bdrv_ref(bs);\n> }\n> - waited |= bdrv_drain_recurse(bs);\n> + waited |= bdrv_drain_recurse(bs, begin);\n> if (in_main_loop) {\n> bdrv_unref(bs);\n> }\n> @@ -221,12 +228,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)\n> BlockDriverState *bs = data->bs;\n> \n> bdrv_dec_in_flight(bs);\n> - bdrv_drained_begin(bs);\n> + if (data->begin) {\n> + bdrv_drained_begin(bs);\n> + } else {\n> + bdrv_drained_end(bs);\n> + }\n> +\n> data->done = true;\n> aio_co_wake(co);\n> }\n> \n> -static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)\n> +static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,\n> + bool begin)\n> {\n> BdrvCoDrainData data;\n> \n> @@ -239,6 +252,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)\n> .co = qemu_coroutine_self(),\n> .bs = bs,\n> .done = false,\n> + .begin = begin,\n> };\n> bdrv_inc_in_flight(bs);\n> aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),\n> @@ -253,7 +267,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)\n> void bdrv_drained_begin(BlockDriverState *bs)\n> {\n> if (qemu_in_coroutine()) {\n> - bdrv_co_yield_to_drain(bs);\n> + bdrv_co_yield_to_drain(bs, true);\n> return;\n> }\n> \n> @@ -262,17 +276,22 @@ void bdrv_drained_begin(BlockDriverState *bs)\n> bdrv_parent_drained_begin(bs);\n> }\n> \n> - bdrv_drain_recurse(bs);\n> + bdrv_drain_recurse(bs, true);\n> }\n> \n> void bdrv_drained_end(BlockDriverState *bs)\n> {\n> + if (qemu_in_coroutine()) {\n> + bdrv_co_yield_to_drain(bs, false);\n> + return;\n> + }\n> assert(bs->quiesce_counter > 0);\n> if (atomic_fetch_dec(&bs->quiesce_counter) > 1) {\n> return;\n> }\n> \n> bdrv_parent_drained_end(bs);\n> + bdrv_drain_recurse(bs, false);\n> aio_enable_external(bdrv_get_aio_context(bs));\n> }\n> \n> @@ -350,7 +369,7 @@ void bdrv_drain_all_begin(void)\n> aio_context_acquire(aio_context);\n> for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {\n> if (aio_context == bdrv_get_aio_context(bs)) {\n> - waited |= bdrv_drain_recurse(bs);\n> + waited |= bdrv_drain_recurse(bs, true);\n> }\n> }\n> aio_context_release(aio_context);\n> @@ -371,6 +390,7 @@ void bdrv_drain_all_end(void)\n> aio_context_acquire(aio_context);\n> aio_enable_external(aio_context);\n> bdrv_parent_drained_end(bs);\n> + bdrv_drain_recurse(bs, false);\n> aio_context_release(aio_context);\n> }\n> \n> -- \n> 2.11.0\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-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=famz@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 3xyctk6by5z9t43\n\tfor ;\n\tThu, 21 Sep 2017 23:30:38 +1000 (AEST)","from localhost ([::1]:53714 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dv1Yr-0002A8-3k\n\tfor incoming@patchwork.ozlabs.org; Thu, 21 Sep 2017 09:30:37 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:59356)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dv1YJ-00022l-4Z\n\tfor qemu-devel@nongnu.org; Thu, 21 Sep 2017 09:30:09 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dv1YE-0001Pz-Pw\n\tfor qemu-devel@nongnu.org; Thu, 21 Sep 2017 09:30:03 -0400","from mx1.redhat.com ([209.132.183.28]:54660)\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 1dv1Y6-0001HR-9T; Thu, 21 Sep 2017 09:29:50 -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 A5950C074F1E;\n\tThu, 21 Sep 2017 13:29:48 +0000 (UTC)","from localhost (ovpn-12-27.pek2.redhat.com [10.72.12.27])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 2102D6017B;\n\tThu, 21 Sep 2017 13:29:44 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com A5950C074F1E","Date":"Thu, 21 Sep 2017 21:29:43 +0800","From":"Fam Zheng ","To":"Manos Pitsidianakis ","Message-ID":"<20170921132943.GE13703@lemon.lan>","References":"<20170921131707.17364-1-el13635@mail.ntua.gr>\n\t<20170921131707.17364-2-el13635@mail.ntua.gr>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170921131707.17364-2-el13635@mail.ntua.gr>","User-Agent":"Mutt/1.8.3 (2017-05-23)","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\tThu, 21 Sep 2017 13:29:48 +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 1/3] block: add bdrv_co_drain_end\n\tcallback","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":"Kevin Wolf , Stefan Hajnoczi ,\n\tqemu-devel , qemu-block ,\n\tMax Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1772800,"web_url":"http://patchwork.ozlabs.org/comment/1772800/","msgid":"<20170921133037.GF13703@lemon.lan>","list_archive_url":null,"date":"2017-09-21T13:30:37","subject":"Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end\n\tcallback","submitter":{"id":24872,"url":"http://patchwork.ozlabs.org/api/people/24872/","name":"Fam Zheng","email":"famz@redhat.com"},"content":"On Thu, 09/21 16:17, Manos Pitsidianakis wrote:\n> BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end\n> of the drain. The throttle driver (block/throttle.c) needs a way to mark the\n> end of the drain in order to toggle io_limits_disabled correctly, thus\n> bdrv_co_drain_end is needed.\n> \n> Signed-off-by: Manos Pitsidianakis \n\nReviewed-by: Fam Zheng ","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=famz@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 3xycvl0vz4z9t43\n\tfor ;\n\tThu, 21 Sep 2017 23:31:31 +1000 (AEST)","from localhost ([::1]:53723 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dv1Zh-0002eW-8r\n\tfor incoming@patchwork.ozlabs.org; Thu, 21 Sep 2017 09:31:29 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:59509)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dv1Z7-0002ag-NG\n\tfor qemu-devel@nongnu.org; Thu, 21 Sep 2017 09:30:56 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dv1Z6-0001tk-Ud\n\tfor qemu-devel@nongnu.org; Thu, 21 Sep 2017 09:30:53 -0400","from mx1.redhat.com ([209.132.183.28]:34584)\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 1dv1Yx-0001qi-KP; Thu, 21 Sep 2017 09:30:43 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\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 9DC49CCB7A;\n\tThu, 21 Sep 2017 13:30:42 +0000 (UTC)","from localhost (ovpn-12-27.pek2.redhat.com [10.72.12.27])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 82B236B54E;\n\tThu, 21 Sep 2017 13:30:38 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 9DC49CCB7A","Date":"Thu, 21 Sep 2017 21:30:37 +0800","From":"Fam Zheng ","To":"Manos Pitsidianakis ","Message-ID":"<20170921133037.GF13703@lemon.lan>","References":"<20170921131707.17364-1-el13635@mail.ntua.gr>\n\t<20170921131707.17364-2-el13635@mail.ntua.gr>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170921131707.17364-2-el13635@mail.ntua.gr>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.39]);\n\tThu, 21 Sep 2017 13:30:42 +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 1/3] block: add bdrv_co_drain_end\n\tcallback","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":"Kevin Wolf , Stefan Hajnoczi ,\n\tqemu-devel , qemu-block ,\n\tMax Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1772943,"web_url":"http://patchwork.ozlabs.org/comment/1772943/","msgid":"<20170921153935.ffyvqej37qhfkrm5@postretch>","list_archive_url":null,"date":"2017-09-21T15:39:35","subject":"Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end\n\tcallback","submitter":{"id":71571,"url":"http://patchwork.ozlabs.org/api/people/71571/","name":"Manos Pitsidianakis","email":"el13635@mail.ntua.gr"},"content":"On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:\n>On Thu, 09/21 16:17, Manos Pitsidianakis wrote:\n>> BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end\n>\n>s/bdrv_do_drain/bdrv_co_drain/\n>\n>> of the drain. The throttle driver (block/throttle.c) needs a way to mark the\n>> end of the drain in order to toggle io_limits_disabled correctly, thus\n>> bdrv_co_drain_end is needed.\n>>\n>> Signed-off-by: Manos Pitsidianakis \n>> ---\n>> include/block/block_int.h | 6 ++++++\n>> block/io.c | 48 +++++++++++++++++++++++++++++++++--------------\n>> 2 files changed, 40 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/include/block/block_int.h b/include/block/block_int.h\n>> index ba4c383393..21950cfda3 100644\n>> --- a/include/block/block_int.h\n>> +++ b/include/block/block_int.h\n>> @@ -356,8 +356,14 @@ struct BlockDriver {\n>> /**\n>> * Drain and stop any internal sources of requests in the driver, and\n>> * remain so until next I/O callback (e.g. bdrv_co_writev) is called.\n>\n>This line needs update too, maybe:\n>\n> /**\n> * bdrv_co_drain drains and stops any ... and remain so until\n> * bdrv_co_drain_end is called.\n>\n>> + *\n>> + * The callbacks are called at the beginning and ending of the drain\n>> + * respectively. They should be implemented if the driver needs to e.g.\n>\n>As implied by above change, should we explicitly require \"should both be\n>implemented\"? It may not be technically required, but I think is cleaner and\n>easier to reason about.\n>\n\nIt might imply to someone that there's an \nassert(drv->bdrv_co_drain_begin && drv->bdrv_co_drain_end) somewhere \nunless you state they don't have to be implemented at the same time. How \nabout we be completely explicit:\n\n bdrv_co_drain_begin is called if implemented in the beggining of a\n drain operation to drain and stop any internal sources of requests in\n the driver.\n bdrv_co_drain_end is called if implemented at the end of the drain.\n\n They should be used by the driver to e.g. manage scheduled I/O\n requests, or toggle an internal state. After the end of the drain new\n requests will continue normally.\n\nI hope this is easier for a reader to understand!\n\n>> + * manage scheduled I/O requests, or toggle an internal state. \n>> After an\n>\n>s/After an/After a/\n>\n>> + * bdrv_co_drain_end() invocation new requests will continue normally.\n>> */\n>> void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);\n>> + void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);\n>>\n>> void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,\n>> Error **errp);\n>> diff --git a/block/io.c b/block/io.c\n>> index 4378ae4c7d..b0a10ad3ef 100644\n>> --- a/block/io.c\n>> +++ b/block/io.c\n>> @@ -153,6 +153,7 @@ typedef struct {\n>> Coroutine *co;\n>> BlockDriverState *bs;\n>> bool done;\n>> + bool begin;\n>> } BdrvCoDrainData;\n>>\n>> static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)\n>> @@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)\n>> BdrvCoDrainData *data = opaque;\n>> BlockDriverState *bs = data->bs;\n>>\n>> - bs->drv->bdrv_co_drain(bs);\n>> + if (data->begin) {\n>> + bs->drv->bdrv_co_drain(bs);\n>> + } else {\n>> + bs->drv->bdrv_co_drain_end(bs);\n>> + }\n>>\n>> /* Set data->done before reading bs->wakeup. */\n>> atomic_mb_set(&data->done, true);\n>> bdrv_wakeup(bs);\n>> }\n>>\n>> -static void bdrv_drain_invoke(BlockDriverState *bs)\n>> +static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)\n>> {\n>> - BdrvCoDrainData data = { .bs = bs, .done = false };\n>> + BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};\n>>\n>> - if (!bs->drv || !bs->drv->bdrv_co_drain) {\n>> + if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||\n>> + (!begin && !bs->drv->bdrv_co_drain_end)) {\n>> return;\n>> }\n>>\n>> @@ -180,15 +186,16 @@ static void bdrv_drain_invoke(BlockDriverState *bs)\n>> BDRV_POLL_WHILE(bs, !data.done);\n>> }\n>>\n>> -static bool bdrv_drain_recurse(BlockDriverState *bs)\n>> +static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)\n>> {\n>> BdrvChild *child, *tmp;\n>> bool waited;\n>>\n>> - waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);\n>> -\n>> /* Ensure any pending metadata writes are submitted to bs->file. */\n>> - bdrv_drain_invoke(bs);\n>> + bdrv_drain_invoke(bs, begin);\n>> +\n>> + /* Wait for drained requests to finish */\n>> + waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);\n>>\n>> QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {\n>> BlockDriverState *bs = child->bs;\n>> @@ -205,7 +212,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)\n>> */\n>> bdrv_ref(bs);\n>> }\n>> - waited |= bdrv_drain_recurse(bs);\n>> + waited |= bdrv_drain_recurse(bs, begin);\n>> if (in_main_loop) {\n>> bdrv_unref(bs);\n>> }\n>> @@ -221,12 +228,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)\n>> BlockDriverState *bs = data->bs;\n>>\n>> bdrv_dec_in_flight(bs);\n>> - bdrv_drained_begin(bs);\n>> + if (data->begin) {\n>> + bdrv_drained_begin(bs);\n>> + } else {\n>> + bdrv_drained_end(bs);\n>> + }\n>> +\n>> data->done = true;\n>> aio_co_wake(co);\n>> }\n>>\n>> -static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)\n>> +static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,\n>> + bool begin)\n>> {\n>> BdrvCoDrainData data;\n>>\n>> @@ -239,6 +252,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)\n>> .co = qemu_coroutine_self(),\n>> .bs = bs,\n>> .done = false,\n>> + .begin = begin,\n>> };\n>> bdrv_inc_in_flight(bs);\n>> aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),\n>> @@ -253,7 +267,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)\n>> void bdrv_drained_begin(BlockDriverState *bs)\n>> {\n>> if (qemu_in_coroutine()) {\n>> - bdrv_co_yield_to_drain(bs);\n>> + bdrv_co_yield_to_drain(bs, true);\n>> return;\n>> }\n>>\n>> @@ -262,17 +276,22 @@ void bdrv_drained_begin(BlockDriverState *bs)\n>> bdrv_parent_drained_begin(bs);\n>> }\n>>\n>> - bdrv_drain_recurse(bs);\n>> + bdrv_drain_recurse(bs, true);\n>> }\n>>\n>> void bdrv_drained_end(BlockDriverState *bs)\n>> {\n>> + if (qemu_in_coroutine()) {\n>> + bdrv_co_yield_to_drain(bs, false);\n>> + return;\n>> + }\n>> assert(bs->quiesce_counter > 0);\n>> if (atomic_fetch_dec(&bs->quiesce_counter) > 1) {\n>> return;\n>> }\n>>\n>> bdrv_parent_drained_end(bs);\n>> + bdrv_drain_recurse(bs, false);\n>> aio_enable_external(bdrv_get_aio_context(bs));\n>> }\n>>\n>> @@ -350,7 +369,7 @@ void bdrv_drain_all_begin(void)\n>> aio_context_acquire(aio_context);\n>> for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {\n>> if (aio_context == bdrv_get_aio_context(bs)) {\n>> - waited |= bdrv_drain_recurse(bs);\n>> + waited |= bdrv_drain_recurse(bs, true);\n>> }\n>> }\n>> aio_context_release(aio_context);\n>> @@ -371,6 +390,7 @@ void bdrv_drain_all_end(void)\n>> aio_context_acquire(aio_context);\n>> aio_enable_external(aio_context);\n>> bdrv_parent_drained_end(bs);\n>> + bdrv_drain_recurse(bs, false);\n>> aio_context_release(aio_context);\n>> }\n>>\n>> --\n>> 2.11.0\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 3xygmM5XQfz9t4B\n\tfor ;\n\tFri, 22 Sep 2017 01:40:18 +1000 (AEST)","from localhost ([::1]:54258 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dv3aJ-0001ZQ-Cc\n\tfor incoming@patchwork.ozlabs.org; Thu, 21 Sep 2017 11:40:15 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:43002)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dv3Zt-0001Xv-SA\n\tfor qemu-devel@nongnu.org; Thu, 21 Sep 2017 11:39:51 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dv3Zs-00009w-Ho\n\tfor qemu-devel@nongnu.org; Thu, 21 Sep 2017 11:39:49 -0400","from smtp1.ntua.gr ([2001:648:2000:de::183]:46732)\n\tby eggs.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from )\n\tid 1dv3Zm-0008Qb-41; Thu, 21 Sep 2017 11:39:42 -0400","from mail.ntua.gr (carp0.noc.ntua.gr [147.102.222.60])\n\t(authenticated bits=0)\n\tby smtp1.ntua.gr (8.15.2/8.15.2) with ESMTPSA id v8LFdZv2073459\n\t(version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256\n\tverify=NOT); Thu, 21 Sep 2017 18:39:36 +0300 (EEST)\n\t(envelope-from el13635@mail.ntua.gr)"],"X-Authentication-Warning":"smtp1.ntua.gr: Host carp0.noc.ntua.gr\n\t[147.102.222.60] claimed to be mail.ntua.gr","Date":"Thu, 21 Sep 2017 18:39:35 +0300","From":"Manos Pitsidianakis ","To":"Fam Zheng ","Message-ID":"<20170921153935.ffyvqej37qhfkrm5@postretch>","Mail-Followup-To":"Manos Pitsidianakis ,\n\tFam Zheng , qemu-devel ,\n\tqemu-block , Kevin Wolf ,\n\tStefan Hajnoczi ,\n\tMax Reitz ","References":"<20170921131707.17364-1-el13635@mail.ntua.gr>\n\t<20170921131707.17364-2-el13635@mail.ntua.gr>\n\t<20170921132943.GE13703@lemon.lan>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha512;\n\tprotocol=\"application/pgp-signature\"; boundary=\"t4375hxdetoo2llw\"","Content-Disposition":"inline","In-Reply-To":"<20170921132943.GE13703@lemon.lan>","User-Agent":"NeoMutt/20170609-57-1e93be (1.8.3)","X-detected-operating-system":"by eggs.gnu.org: Genre and OS details not\n\trecognized.","X-Received-From":"2001:648:2000:de::183","Subject":"Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end\n\tcallback","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":"Kevin Wolf , Stefan Hajnoczi ,\n\tqemu-devel , qemu-block ,\n\tMax Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1773226,"web_url":"http://patchwork.ozlabs.org/comment/1773226/","msgid":"<20170922023053.GB1397@lemon.lan>","list_archive_url":null,"date":"2017-09-22T02:30:53","subject":"Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end\n\tcallback","submitter":{"id":24872,"url":"http://patchwork.ozlabs.org/api/people/24872/","name":"Fam Zheng","email":"famz@redhat.com"},"content":"On Thu, 09/21 18:39, Manos Pitsidianakis wrote:\n> On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:\n> > On Thu, 09/21 16:17, Manos Pitsidianakis wrote:\n> > > BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end\n> > \n> > s/bdrv_do_drain/bdrv_co_drain/\n> > \n> > > of the drain. The throttle driver (block/throttle.c) needs a way to mark the\n> > > end of the drain in order to toggle io_limits_disabled correctly, thus\n> > > bdrv_co_drain_end is needed.\n> > > \n> > > Signed-off-by: Manos Pitsidianakis \n> > > ---\n> > > include/block/block_int.h | 6 ++++++\n> > > block/io.c | 48 +++++++++++++++++++++++++++++++++--------------\n> > > 2 files changed, 40 insertions(+), 14 deletions(-)\n> > > \n> > > diff --git a/include/block/block_int.h b/include/block/block_int.h\n> > > index ba4c383393..21950cfda3 100644\n> > > --- a/include/block/block_int.h\n> > > +++ b/include/block/block_int.h\n> > > @@ -356,8 +356,14 @@ struct BlockDriver {\n> > > /**\n> > > * Drain and stop any internal sources of requests in the driver, and\n> > > * remain so until next I/O callback (e.g. bdrv_co_writev) is called.\n> > \n> > This line needs update too, maybe:\n> > \n> > /**\n> > * bdrv_co_drain drains and stops any ... and remain so until\n> > * bdrv_co_drain_end is called.\n> > \n> > > + *\n> > > + * The callbacks are called at the beginning and ending of the drain\n> > > + * respectively. They should be implemented if the driver needs to e.g.\n> > \n> > As implied by above change, should we explicitly require \"should both be\n> > implemented\"? It may not be technically required, but I think is cleaner and\n> > easier to reason about.\n> > \n> \n> It might imply to someone that there's an assert(drv->bdrv_co_drain_begin &&\n> drv->bdrv_co_drain_end) somewhere unless you state they don't have to be\n> implemented at the same time. How about we be completely explicit:\n> \n> bdrv_co_drain_begin is called if implemented in the beggining of a\n> drain operation to drain and stop any internal sources of requests in\n> the driver.\n> bdrv_co_drain_end is called if implemented at the end of the drain.\n> \n> They should be used by the driver to e.g. manage scheduled I/O\n> requests, or toggle an internal state. After the end of the drain new\n> requests will continue normally.\n> \n> I hope this is easier for a reader to understand!\n\nI don't like the inconsistent semantics of when the drained section ends, if we\nallow drivers to implement bdrv_co_drain_begin but omit bdrv_co_drained_end.\nCurrently the point where the section ends is, as said in the comment, when next\nI/O callback is invoked. Now we are adding the explicit \".bdrv_co_drain_end\"\ninto the fomular, if we still keep the previous convention, the interface\ncontract is just mixed of two things for no good reason. I don't think it's\ntechnically necessary.\n\nLet's just add the assert:\n\n assert(!!drv->bdrv_co_drain_begin == !!bdrv_co_drain_end);\n\nin bdrv_drain_invoke, add a comment here, then add an empty .bdrv_co_drain_end\nin qed.\n\nFam","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=famz@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 3xyyDB3bgZz9sPk\n\tfor ;\n\tFri, 22 Sep 2017 12:31:51 +1000 (AEST)","from localhost ([::1]:56397 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dvDkq-00073z-Ub\n\tfor incoming@patchwork.ozlabs.org; Thu, 21 Sep 2017 22:31:48 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:47550)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dvDk9-00072L-K3\n\tfor qemu-devel@nongnu.org; Thu, 21 Sep 2017 22:31:06 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dvDk7-0002dJ-VK\n\tfor qemu-devel@nongnu.org; Thu, 21 Sep 2017 22:31:05 -0400","from mx1.redhat.com ([209.132.183.28]:58667)\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 1dvDk2-0002I3-KZ; Thu, 21 Sep 2017 22:30:58 -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 6821E7C839;\n\tFri, 22 Sep 2017 02:30:57 +0000 (UTC)","from localhost (ovpn-12-27.pek2.redhat.com [10.72.12.27])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id B18F617AA3;\n\tFri, 22 Sep 2017 02:30:54 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 6821E7C839","Date":"Fri, 22 Sep 2017 10:30:53 +0800","From":"Fam Zheng ","To":"Manos Pitsidianakis ,\n\tqemu-devel ,\n\tqemu-block , Kevin Wolf ,\n\tStefan Hajnoczi , Max Reitz ","Message-ID":"<20170922023053.GB1397@lemon.lan>","References":"<20170921131707.17364-1-el13635@mail.ntua.gr>\n\t<20170921131707.17364-2-el13635@mail.ntua.gr>\n\t<20170921132943.GE13703@lemon.lan>\n\t<20170921153935.ffyvqej37qhfkrm5@postretch>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170921153935.ffyvqej37qhfkrm5@postretch>","User-Agent":"Mutt/1.8.3 (2017-05-23)","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.27]);\n\tFri, 22 Sep 2017 02:30:57 +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 1/3] block: add bdrv_co_drain_end\n\tcallback","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","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1773474,"web_url":"http://patchwork.ozlabs.org/comment/1773474/","msgid":"<20170922110320.GD12295@localhost.localdomain>","list_archive_url":null,"date":"2017-09-22T11:03:20","subject":"Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end\n\tcallback","submitter":{"id":2714,"url":"http://patchwork.ozlabs.org/api/people/2714/","name":"Kevin Wolf","email":"kwolf@redhat.com"},"content":"Am 22.09.2017 um 04:30 hat Fam Zheng geschrieben:\n> On Thu, 09/21 18:39, Manos Pitsidianakis wrote:\n> > On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:\n> > > On Thu, 09/21 16:17, Manos Pitsidianakis wrote:\n> > It might imply to someone that there's an assert(drv->bdrv_co_drain_begin &&\n> > drv->bdrv_co_drain_end) somewhere unless you state they don't have to be\n> > implemented at the same time. How about we be completely explicit:\n> > \n> > bdrv_co_drain_begin is called if implemented in the beggining of a\n> > drain operation to drain and stop any internal sources of requests in\n> > the driver.\n> > bdrv_co_drain_end is called if implemented at the end of the drain.\n> > \n> > They should be used by the driver to e.g. manage scheduled I/O\n> > requests, or toggle an internal state. After the end of the drain new\n> > requests will continue normally.\n> > \n> > I hope this is easier for a reader to understand!\n> \n> I don't like the inconsistent semantics of when the drained section\n> ends, if we allow drivers to implement bdrv_co_drain_begin but omit\n> bdrv_co_drained_end. Currently the point where the section ends is,\n> as said in the comment, when next I/O callback is invoked. Now we are\n> adding the explicit \".bdrv_co_drain_end\" into the fomular, if we still\n> keep the previous convention, the interface contract is just mixed of\n> two things for no good reason. I don't think it's technically\n> necessary.\n\nWe don't keep the convention with the next I/O callback. We just allow\ndrivers to omit an empty implementation of either callback, which seems\nto be a very sensible default to me.\n\n> Let's just add the assert:\n> \n> assert(!!drv->bdrv_co_drain_begin == !!bdrv_co_drain_end);\n> \n> in bdrv_drain_invoke, add a comment here\n\nI'm not in favour of this, but if we do want to have it, wouldn't\nbdrv_register() be a much better place for the assertion?\n\n> then add an empty .bdrv_co_drain_end in qed.\n\nIf you need empty functions anywhere, it's a sign that we have a bad\ndefault behaviour.\n\nKevin","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=kwolf@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 3xz9bD4WWvz9ryr\n\tfor ;\n\tFri, 22 Sep 2017 21:04:07 +1000 (AEST)","from localhost ([::1]:57976 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dvLkZ-0006rb-9r\n\tfor incoming@patchwork.ozlabs.org; Fri, 22 Sep 2017 07:04:03 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:34664)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dvLk8-0006pM-9O\n\tfor qemu-devel@nongnu.org; Fri, 22 Sep 2017 07:03:40 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dvLk7-0006mw-7m\n\tfor qemu-devel@nongnu.org; Fri, 22 Sep 2017 07:03:36 -0400","from mx1.redhat.com ([209.132.183.28]:50290)\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 1dvLjz-0006iU-SP; Fri, 22 Sep 2017 07:03:28 -0400","from smtp.corp.redhat.com\n\t(int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16])\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 AA082806B2;\n\tFri, 22 Sep 2017 11:03:26 +0000 (UTC)","from localhost.localdomain (ovpn-116-100.ams2.redhat.com\n\t[10.36.116.100])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 49D8E5C269;\n\tFri, 22 Sep 2017 11:03:21 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com AA082806B2","Date":"Fri, 22 Sep 2017 13:03:20 +0200","From":"Kevin Wolf ","To":"Fam Zheng ","Message-ID":"<20170922110320.GD12295@localhost.localdomain>","References":"<20170921131707.17364-1-el13635@mail.ntua.gr>\n\t<20170921131707.17364-2-el13635@mail.ntua.gr>\n\t<20170921132943.GE13703@lemon.lan>\n\t<20170921153935.ffyvqej37qhfkrm5@postretch>\n\t<20170922023053.GB1397@lemon.lan>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170922023053.GB1397@lemon.lan>","User-Agent":"Mutt/1.9.0 (2017-09-02)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.16","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.26]);\n\tFri, 22 Sep 2017 11:03:26 +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 1/3] block: add bdrv_co_drain_end\n\tcallback","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":"Max Reitz , Stefan Hajnoczi ,\n\tqemu-devel , qemu-block ,\n\tManos Pitsidianakis ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1773553,"web_url":"http://patchwork.ozlabs.org/comment/1773553/","msgid":"<20170922123400.GA32000@lemon>","list_archive_url":null,"date":"2017-09-22T12:34:43","subject":"Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end\n\tcallback","submitter":{"id":24872,"url":"http://patchwork.ozlabs.org/api/people/24872/","name":"Fam Zheng","email":"famz@redhat.com"},"content":"On Fri, 09/22 13:03, Kevin Wolf wrote:\n> Am 22.09.2017 um 04:30 hat Fam Zheng geschrieben:\n> > On Thu, 09/21 18:39, Manos Pitsidianakis wrote:\n> > > On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:\n> > > > On Thu, 09/21 16:17, Manos Pitsidianakis wrote:\n> > > It might imply to someone that there's an assert(drv->bdrv_co_drain_begin &&\n> > > drv->bdrv_co_drain_end) somewhere unless you state they don't have to be\n> > > implemented at the same time. How about we be completely explicit:\n> > > \n> > > bdrv_co_drain_begin is called if implemented in the beggining of a\n> > > drain operation to drain and stop any internal sources of requests in\n> > > the driver.\n> > > bdrv_co_drain_end is called if implemented at the end of the drain.\n> > > \n> > > They should be used by the driver to e.g. manage scheduled I/O\n> > > requests, or toggle an internal state. After the end of the drain new\n> > > requests will continue normally.\n> > > \n> > > I hope this is easier for a reader to understand!\n> > \n> > I don't like the inconsistent semantics of when the drained section\n> > ends, if we allow drivers to implement bdrv_co_drain_begin but omit\n> > bdrv_co_drained_end. Currently the point where the section ends is,\n> > as said in the comment, when next I/O callback is invoked. Now we are\n> > adding the explicit \".bdrv_co_drain_end\" into the fomular, if we still\n> > keep the previous convention, the interface contract is just mixed of\n> > two things for no good reason. I don't think it's technically\n> > necessary.\n> \n> We don't keep the convention with the next I/O callback. We just allow\n> drivers to omit an empty implementation of either callback, which seems\n> to be a very sensible default to me.\n> \n\nOK, I'm fine with this.\n\nFam","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-mx05.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx05.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=famz@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 3xzClg4RJhz9sPk\n\tfor ;\n\tFri, 22 Sep 2017 22:41:35 +1000 (AEST)","from localhost ([::1]:58667 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dvNGv-0007aF-Lb\n\tfor incoming@patchwork.ozlabs.org; Fri, 22 Sep 2017 08:41:33 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:57510)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dvNAc-0002VK-Dg\n\tfor qemu-devel@nongnu.org; Fri, 22 Sep 2017 08:35:08 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dvNAb-00078J-Fq\n\tfor qemu-devel@nongnu.org; Fri, 22 Sep 2017 08:35:02 -0400","from mx1.redhat.com ([209.132.183.28]:44314)\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 1dvNAP-0006zk-0I; Fri, 22 Sep 2017 08:34:49 -0400","from smtp.corp.redhat.com\n\t(int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16])\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 E30AF13AA4;\n\tFri, 22 Sep 2017 12:34:47 +0000 (UTC)","from localhost (ovpn-12-35.pek2.redhat.com [10.72.12.35])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 6D1785C88A;\n\tFri, 22 Sep 2017 12:34:45 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com E30AF13AA4","Date":"Fri, 22 Sep 2017 20:34:43 +0800","From":"Fam Zheng ","To":"Kevin Wolf ","Message-ID":"<20170922123400.GA32000@lemon>","References":"<20170921131707.17364-1-el13635@mail.ntua.gr>\n\t<20170921131707.17364-2-el13635@mail.ntua.gr>\n\t<20170921132943.GE13703@lemon.lan>\n\t<20170921153935.ffyvqej37qhfkrm5@postretch>\n\t<20170922023053.GB1397@lemon.lan>\n\t<20170922110320.GD12295@localhost.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170922110320.GD12295@localhost.localdomain>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.16","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.29]);\n\tFri, 22 Sep 2017 12:34:48 +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 1/3] block: add bdrv_co_drain_end\n\tcallback","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":"Manos Pitsidianakis ,\n\tqemu-block , qemu-devel ,\n\tStefan Hajnoczi , Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]