[{"id":1769845,"web_url":"http://patchwork.ozlabs.org/comment/1769845/","msgid":"<20170918034431.GA15551@lemon.lan>","list_archive_url":null,"date":"2017-09-18T03:44:31","subject":"Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during\n\tbdrv_drain_recurse","submitter":{"id":24872,"url":"http://patchwork.ozlabs.org/api/people/24872/","name":"Fam Zheng","email":"famz@redhat.com"},"content":"On Wed, 09/13 20:18, Max Reitz wrote:\n> Drainined a BDS child may lead to both the original BDS and/or its other\n> children being deleted (e.g. if the original BDS represents a block\n> job). We should prepare for this in both bdrv_drain_recurse() and\n> bdrv_drained_begin() by monitoring whether the BDS we are about to drain\n> still exists at all.\n\nCan the deletion happen when IOThread calls\nbdrv_drain_recurse/bdrv_drained_begin? If not, is it enough to do\n\n ...\n if (in_main_loop) {\n bdrv_ref(bs);\n }\n ...\n if (in_main_loop) {\n bdrv_unref(bs);\n }\n\nto protect the main loop case? So the BdrvDeletedStatus state is not needed.\n\nFam\n\n> \n> Signed-off-by: Max Reitz \n> ---\n> block/io.c | 72 +++++++++++++++++++++++++++++++++++++++++++++-----------------\n> 1 file changed, 52 insertions(+), 20 deletions(-)\n> \n> diff --git a/block/io.c b/block/io.c\n> index 4378ae4c7d..8ec1a564ad 100644\n> --- a/block/io.c\n> +++ b/block/io.c\n> @@ -182,33 +182,57 @@ static void bdrv_drain_invoke(BlockDriverState *bs)\n> \n> static bool bdrv_drain_recurse(BlockDriverState *bs)\n> {\n> - BdrvChild *child, *tmp;\n> + BdrvChild *child;\n> bool waited;\n> + struct BDSToDrain {\n> + BlockDriverState *bs;\n> + BdrvDeletedStatus del_stat;\n> + QLIST_ENTRY(BDSToDrain) next;\n> + };\n> + QLIST_HEAD(, BDSToDrain) bs_list = QLIST_HEAD_INITIALIZER(bs_list);\n> + bool in_main_loop =\n> + qemu_get_current_aio_context() == qemu_get_aio_context();\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> \n> - QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {\n> - BlockDriverState *bs = child->bs;\n> - bool in_main_loop =\n> - qemu_get_current_aio_context() == qemu_get_aio_context();\n> - assert(bs->refcnt > 0);\n> - if (in_main_loop) {\n> - /* In case the recursive bdrv_drain_recurse processes a\n> - * block_job_defer_to_main_loop BH and modifies the graph,\n> - * let's hold a reference to bs until we are done.\n> - *\n> - * IOThread doesn't have such a BH, and it is not safe to call\n> - * bdrv_unref without BQL, so skip doing it there.\n> - */\n> - bdrv_ref(bs);\n> - }\n> - waited |= bdrv_drain_recurse(bs);\n> - if (in_main_loop) {\n> - bdrv_unref(bs);\n> + /* Draining children may result in other children being removed and maybe\n> + * even deleted, so copy the children list first */\n> + QLIST_FOREACH(child, &bs->children, next) {\n> + struct BDSToDrain *bs2d = g_new0(struct BDSToDrain, 1);\n> +\n> + bs2d->bs = child->bs;\n> + QLIST_INSERT_HEAD(&bs->deleted_status, &bs2d->del_stat, next);\n> +\n> + QLIST_INSERT_HEAD(&bs_list, bs2d, next);\n> + }\n> +\n> + while (!QLIST_EMPTY(&bs_list)) {\n> + struct BDSToDrain *bs2d = QLIST_FIRST(&bs_list);\n> + QLIST_REMOVE(bs2d, next);\n> +\n> + if (!bs2d->del_stat.deleted) {\n> + QLIST_REMOVE(&bs2d->del_stat, next);\n> +\n> + if (in_main_loop) {\n> + /* In case the recursive bdrv_drain_recurse processes a\n> + * block_job_defer_to_main_loop BH and modifies the graph,\n> + * let's hold a reference to the BDS until we are done.\n> + *\n> + * IOThread doesn't have such a BH, and it is not safe to call\n> + * bdrv_unref without BQL, so skip doing it there.\n> + */\n> + bdrv_ref(bs2d->bs);\n> + }\n> + waited |= bdrv_drain_recurse(bs2d->bs);\n> + if (in_main_loop) {\n> + bdrv_unref(bs2d->bs);\n> + }\n> }\n> +\n> + g_free(bs2d);\n> }\n> \n> return waited;\n> @@ -252,17 +276,25 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)\n> \n> void bdrv_drained_begin(BlockDriverState *bs)\n> {\n> + BdrvDeletedStatus del_stat = { .deleted = false };\n> +\n> if (qemu_in_coroutine()) {\n> bdrv_co_yield_to_drain(bs);\n> return;\n> }\n> \n> + QLIST_INSERT_HEAD(&bs->deleted_status, &del_stat, next);\n> +\n> if (atomic_fetch_inc(&bs->quiesce_counter) == 0) {\n> aio_disable_external(bdrv_get_aio_context(bs));\n> bdrv_parent_drained_begin(bs);\n> }\n> \n> - bdrv_drain_recurse(bs);\n> + if (!del_stat.deleted) {\n> + QLIST_REMOVE(&del_stat, next);\n> +\n> + bdrv_drain_recurse(bs);\n> + }\n> }\n> \n> void bdrv_drained_end(BlockDriverState *bs)\n> -- \n> 2.13.5\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-mx04.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx04.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 3xwX2Y4h94z9s3T\n\tfor ;\n\tMon, 18 Sep 2017 13:45:09 +1000 (AEST)","from localhost ([::1]:34507 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dtmzb-0000E6-Pv\n\tfor incoming@patchwork.ozlabs.org; Sun, 17 Sep 2017 23:45:07 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:49475)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dtmzF-0000Bi-MA\n\tfor qemu-devel@nongnu.org; Sun, 17 Sep 2017 23:44:47 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dtmzD-00026B-Uh\n\tfor qemu-devel@nongnu.org; Sun, 17 Sep 2017 23:44:45 -0400","from mx1.redhat.com ([209.132.183.28]:32888)\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 1dtmz9-00022x-4N; Sun, 17 Sep 2017 23:44:39 -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 F33E480099;\n\tMon, 18 Sep 2017 03:44:37 +0000 (UTC)","from localhost (ovpn-12-141.pek2.redhat.com [10.72.12.141])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 34C1560468;\n\tMon, 18 Sep 2017 03:44:32 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com F33E480099","Date":"Mon, 18 Sep 2017 11:44:31 +0800","From":"Fam Zheng ","To":"Max Reitz ","Message-ID":"<20170918034431.GA15551@lemon.lan>","References":"<20170913181910.29688-1-mreitz@redhat.com>\n\t<20170913181910.29688-3-mreitz@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170913181910.29688-3-mreitz@redhat.com>","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.28]);\n\tMon, 18 Sep 2017 03:44:38 +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 02/18] block: BDS deletion during\n\tbdrv_drain_recurse","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 , John Snow ,\n\tStefan Hajnoczi , qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1770301,"web_url":"http://patchwork.ozlabs.org/comment/1770301/","msgid":"<85fd9e96-4135-a62d-76ff-631104cd02f1@redhat.com>","list_archive_url":null,"date":"2017-09-18T16:13:03","subject":"Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during\n\tbdrv_drain_recurse","submitter":{"id":36836,"url":"http://patchwork.ozlabs.org/api/people/36836/","name":"Max Reitz","email":"mreitz@redhat.com"},"content":"On 2017-09-18 05:44, Fam Zheng wrote:\n> On Wed, 09/13 20:18, Max Reitz wrote:\n>> Drainined a BDS child may lead to both the original BDS and/or its other\n>> children being deleted (e.g. if the original BDS represents a block\n>> job). We should prepare for this in both bdrv_drain_recurse() and\n>> bdrv_drained_begin() by monitoring whether the BDS we are about to drain\n>> still exists at all.\n> \n> Can the deletion happen when IOThread calls\n> bdrv_drain_recurse/bdrv_drained_begin?\n\nI don't think so, because (1) my issue was draining a block job and that\ncan only be completed in the main loop, and (2) I would like to think\nit's always impossible, considering that bdrv_unref() may only be called\nwith the BQL.\n\n> If not, is it enough to do\n> \n> ...\n> if (in_main_loop) {\n> bdrv_ref(bs);\n> }\n> ...\n> if (in_main_loop) {\n> bdrv_unref(bs);\n> }\n> \n> to protect the main loop case? So the BdrvDeletedStatus state is not needed.\n\nWe already have that in bdrv_drained_recurse(), don't we?\n\nThe issue here is, though, that QLIST_FOREACH_SAFE() stores the next\nchild pointer to @tmp. However, once the current child @child is\ndrained, @tmp may no longer be valid -- it may have been detached from\n@bs, and it may even have been deleted.\n\nWe could work around the latter by increasing the next child's reference\nsomehow (but BdrvChild doesn't really have a refcount, and in order to\ndo so, we would probably have to emulate being a parent or\nsomething...), but then you'd still have the issue of @tmp being\ndetached from the children list we're trying to iterate over. So\ntmp->next is no longer valid.\n\nAnyway, so the latter is the reason why I decided to introduce the bs_list.\n\nBut maybe that actually saves us from having to fiddle with BdrvChild...\n Since it's just a list of BDSs now, it may be enough to simply\nbdrv_ref() all of the BDSs in that list before draining any of them. So\n we'd keep creating the bs_list and then we'd move the existing\nbdrv_ref() from the drain loop into the loop filling bs_list.\n\nAnd adding a bdrv_ref()/bdrv_unref() pair to bdrv_drained_begin() should\nhopefully work there, too.\n\nMax","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=mreitz@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 3xws754cFFz9s7F\n\tfor ;\n\tTue, 19 Sep 2017 02:35:13 +1000 (AEST)","from localhost ([::1]:37766 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dtz0p-0002a9-DN\n\tfor incoming@patchwork.ozlabs.org; Mon, 18 Sep 2017 12:35:11 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:42465)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dtyfq-0000z1-Lk\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 12:13:31 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dtyfp-0006LW-Cj\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 12:13:30 -0400","from mx1.redhat.com ([209.132.183.28]:36512)\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 1dtyfk-0006Gj-Fy; Mon, 18 Sep 2017 12:13:24 -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 6E08DC08EC14;\n\tMon, 18 Sep 2017 16:13:23 +0000 (UTC)","from dresden.str.redhat.com (unknown [10.40.205.73])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 7EA1F5D984;\n\tMon, 18 Sep 2017 16:13:05 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 6E08DC08EC14","To":"Fam Zheng ","References":"<20170913181910.29688-1-mreitz@redhat.com>\n\t<20170913181910.29688-3-mreitz@redhat.com>\n\t<20170918034431.GA15551@lemon.lan>","From":"Max Reitz ","Message-ID":"<85fd9e96-4135-a62d-76ff-631104cd02f1@redhat.com>","Date":"Mon, 18 Sep 2017 18:13:03 +0200","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":"<20170918034431.GA15551@lemon.lan>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"k1CgRqXMoES3PRuC9eWiJKxjrIKJMxOko\"","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.31]);\n\tMon, 18 Sep 2017 16:13:23 +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 02/18] block: BDS deletion during\n\tbdrv_drain_recurse","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 , John Snow ,\n\tStefan Hajnoczi , qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1783137,"web_url":"http://patchwork.ozlabs.org/comment/1783137/","msgid":"","list_archive_url":null,"date":"2017-10-09T18:30:13","subject":"Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during\n\tbdrv_drain_recurse","submitter":{"id":36836,"url":"http://patchwork.ozlabs.org/api/people/36836/","name":"Max Reitz","email":"mreitz@redhat.com"},"content":"On 2017-09-18 18:13, Max Reitz wrote:\n> On 2017-09-18 05:44, Fam Zheng wrote:\n>> On Wed, 09/13 20:18, Max Reitz wrote:\n>>> Drainined a BDS child may lead to both the original BDS and/or its other\n>>> children being deleted (e.g. if the original BDS represents a block\n>>> job). We should prepare for this in both bdrv_drain_recurse() and\n>>> bdrv_drained_begin() by monitoring whether the BDS we are about to drain\n>>> still exists at all.\n>>\n>> Can the deletion happen when IOThread calls\n>> bdrv_drain_recurse/bdrv_drained_begin?\n> \n> I don't think so, because (1) my issue was draining a block job and that\n> can only be completed in the main loop, and (2) I would like to think\n> it's always impossible, considering that bdrv_unref() may only be called\n> with the BQL.\n> \n>> If not, is it enough to do\n>>\n>> ...\n>> if (in_main_loop) {\n>> bdrv_ref(bs);\n>> }\n>> ...\n>> if (in_main_loop) {\n>> bdrv_unref(bs);\n>> }\n>>\n>> to protect the main loop case? So the BdrvDeletedStatus state is not needed.\n> \n> We already have that in bdrv_drained_recurse(), don't we?\n> \n> The issue here is, though, that QLIST_FOREACH_SAFE() stores the next\n> child pointer to @tmp. However, once the current child @child is\n> drained, @tmp may no longer be valid -- it may have been detached from\n> @bs, and it may even have been deleted.\n> \n> We could work around the latter by increasing the next child's reference\n> somehow (but BdrvChild doesn't really have a refcount, and in order to\n> do so, we would probably have to emulate being a parent or\n> something...), but then you'd still have the issue of @tmp being\n> detached from the children list we're trying to iterate over. So\n> tmp->next is no longer valid.\n> \n> Anyway, so the latter is the reason why I decided to introduce the bs_list.\n> \n> But maybe that actually saves us from having to fiddle with BdrvChild...\n> Since it's just a list of BDSs now, it may be enough to simply\n> bdrv_ref() all of the BDSs in that list before draining any of them. So\n> we'd keep creating the bs_list and then we'd move the existing\n> bdrv_ref() from the drain loop into the loop filling bs_list.\n> \n> And adding a bdrv_ref()/bdrv_unref() pair to bdrv_drained_begin() should\n> hopefully work there, too.\n\nIt turns out it isn't so simple after all... because bdrv_close()\ninvokes bdrv_drained_begin(). So we may end up with an endless recursion\nhere.\n\nOne way to fix this would be to skip the bdrv_drained_begin() in\nbdrv_close() if this would result in such a recursion... But any\nsolution that comes quickly to my mind would require another BDS field,\ntoo -- just checking the quiesce_counter is probably not enough because\nthis might just indicate concurrent drainage that stops before\nbdrv_close() wants it to stop.\n\nSo maybe BdrvDeletedStatus is the simplest solution after all...?\n\nMax","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=mreitz@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 3y9pj43Dkkz9sRq\n\tfor ;\n\tTue, 10 Oct 2017 05:31:04 +1100 (AEDT)","from localhost ([::1]:59256 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e1cpS-0002JA-HB\n\tfor incoming@patchwork.ozlabs.org; Mon, 09 Oct 2017 14:31:02 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:40066)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e1cp3-0002Gn-KL\n\tfor qemu-devel@nongnu.org; Mon, 09 Oct 2017 14:30:38 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e1cp2-00044k-IG\n\tfor qemu-devel@nongnu.org; Mon, 09 Oct 2017 14:30:37 -0400","from mx1.redhat.com ([209.132.183.28]:51924)\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 1e1cox-00042C-CT; Mon, 09 Oct 2017 14:30:31 -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 4AA7181E17;\n\tMon, 9 Oct 2017 18:30:30 +0000 (UTC)","from dresden.str.redhat.com (ovpn-204-245.brq.redhat.com\n\t[10.40.204.245])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 6162918946;\n\tMon, 9 Oct 2017 18:30:14 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 4AA7181E17","From":"Max Reitz ","To":"Fam Zheng ","References":"<20170913181910.29688-1-mreitz@redhat.com>\n\t<20170913181910.29688-3-mreitz@redhat.com>\n\t<20170918034431.GA15551@lemon.lan>\n\t<85fd9e96-4135-a62d-76ff-631104cd02f1@redhat.com>","Message-ID":"","Date":"Mon, 9 Oct 2017 20:30:13 +0200","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":"<85fd9e96-4135-a62d-76ff-631104cd02f1@redhat.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"m0Q4rX1kppHQgMjPOhj4awGdSR7spaqpW\"","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.25]);\n\tMon, 09 Oct 2017 18:30:30 +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 02/18] block: BDS deletion during\n\tbdrv_drain_recurse","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 , John Snow ,\n\tStefan Hajnoczi , qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1783513,"web_url":"http://patchwork.ozlabs.org/comment/1783513/","msgid":"<20171010083651.GC4177@dhcp-200-186.str.redhat.com>","list_archive_url":null,"date":"2017-10-10T08:36:51","subject":"Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during\n\tbdrv_drain_recurse","submitter":{"id":2714,"url":"http://patchwork.ozlabs.org/api/people/2714/","name":"Kevin Wolf","email":"kwolf@redhat.com"},"content":"Am 13.09.2017 um 20:18 hat Max Reitz geschrieben:\n> Drainined a BDS child may lead to both the original BDS and/or its other\n> children being deleted (e.g. if the original BDS represents a block\n> job). We should prepare for this in both bdrv_drain_recurse() and\n> bdrv_drained_begin() by monitoring whether the BDS we are about to drain\n> still exists at all.\n> \n> Signed-off-by: Max Reitz \n\nHow hard would it be to write a test case for this? qemu-iotests\nprobably isn't the right tool, but I feel a C unit test would be\npossible.\n\n> - QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {\n> - BlockDriverState *bs = child->bs;\n> - bool in_main_loop =\n> - qemu_get_current_aio_context() == qemu_get_aio_context();\n> - assert(bs->refcnt > 0);\n\nWould it make sense to keep this assertion for the !deleted case?\n\n> - if (in_main_loop) {\n> - /* In case the recursive bdrv_drain_recurse processes a\n> - * block_job_defer_to_main_loop BH and modifies the graph,\n> - * let's hold a reference to bs until we are done.\n> - *\n> - * IOThread doesn't have such a BH, and it is not safe to call\n> - * bdrv_unref without BQL, so skip doing it there.\n> - */\n> - bdrv_ref(bs);\n> - }\n> - waited |= bdrv_drain_recurse(bs);\n> - if (in_main_loop) {\n> - bdrv_unref(bs);\n> + /* Draining children may result in other children being removed and maybe\n> + * even deleted, so copy the children list first */\n\nMaybe it's just me, but I failed to understand this correctly at first.\nHow about \"being removed from their parent\" to clarify that it's not the\nBDS that is removed, but just the reference?\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-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=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 3yB9Tt0Qssz9tY0\n\tfor ;\n\tTue, 10 Oct 2017 19:37:36 +1100 (AEDT)","from localhost ([::1]:33517 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e1q2g-0003X7-9B\n\tfor incoming@patchwork.ozlabs.org; Tue, 10 Oct 2017 04:37:34 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:55386)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e1q2I-0003Uw-0j\n\tfor qemu-devel@nongnu.org; Tue, 10 Oct 2017 04:37:11 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e1q2H-0007Hd-6g\n\tfor qemu-devel@nongnu.org; Tue, 10 Oct 2017 04:37:10 -0400","from mx1.redhat.com ([209.132.183.28]:47902)\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 1e1q2C-0007E9-QT; Tue, 10 Oct 2017 04:37:04 -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 1BE3725761;\n\tTue, 10 Oct 2017 08:37:02 +0000 (UTC)","from dhcp-200-186.str.redhat.com (dhcp-200-186.str.redhat.com\n\t[10.33.200.186])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 229965C892;\n\tTue, 10 Oct 2017 08:36:52 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 1BE3725761","Date":"Tue, 10 Oct 2017 10:36:51 +0200","From":"Kevin Wolf ","To":"Max Reitz ","Message-ID":"<20171010083651.GC4177@dhcp-200-186.str.redhat.com>","References":"<20170913181910.29688-1-mreitz@redhat.com>\n\t<20170913181910.29688-3-mreitz@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170913181910.29688-3-mreitz@redhat.com>","User-Agent":"Mutt/1.9.1 (2017-09-22)","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.39]);\n\tTue, 10 Oct 2017 08:37:02 +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 02/18] block: BDS deletion during\n\tbdrv_drain_recurse","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":"Stefan Hajnoczi , John Snow ,\n\tFam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1784540,"web_url":"http://patchwork.ozlabs.org/comment/1784540/","msgid":"","list_archive_url":null,"date":"2017-10-11T11:41:39","subject":"Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during\n\tbdrv_drain_recurse","submitter":{"id":36836,"url":"http://patchwork.ozlabs.org/api/people/36836/","name":"Max Reitz","email":"mreitz@redhat.com"},"content":"On 2017-10-10 10:36, Kevin Wolf wrote:\n> Am 13.09.2017 um 20:18 hat Max Reitz geschrieben:\n>> Drainined a BDS child may lead to both the original BDS and/or its other\n>> children being deleted (e.g. if the original BDS represents a block\n>> job). We should prepare for this in both bdrv_drain_recurse() and\n>> bdrv_drained_begin() by monitoring whether the BDS we are about to drain\n>> still exists at all.\n>>\n>> Signed-off-by: Max Reitz \n> \n> How hard would it be to write a test case for this? qemu-iotests\n> probably isn't the right tool, but I feel a C unit test would be\n> possible.\n\nI can look into it, but I can't promise anything.\n\n>> - QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {\n>> - BlockDriverState *bs = child->bs;\n>> - bool in_main_loop =\n>> - qemu_get_current_aio_context() == qemu_get_aio_context();\n>> - assert(bs->refcnt > 0);\n> \n> Would it make sense to keep this assertion for the !deleted case?\n\nSure, why not.\n\n>> - if (in_main_loop) {\n>> - /* In case the recursive bdrv_drain_recurse processes a\n>> - * block_job_defer_to_main_loop BH and modifies the graph,\n>> - * let's hold a reference to bs until we are done.\n>> - *\n>> - * IOThread doesn't have such a BH, and it is not safe to call\n>> - * bdrv_unref without BQL, so skip doing it there.\n>> - */\n>> - bdrv_ref(bs);\n>> - }\n>> - waited |= bdrv_drain_recurse(bs);\n>> - if (in_main_loop) {\n>> - bdrv_unref(bs);\n>> + /* Draining children may result in other children being removed and maybe\n>> + * even deleted, so copy the children list first */\n> \n> Maybe it's just me, but I failed to understand this correctly at first.\n> How about \"being removed from their parent\" to clarify that it's not the\n> BDS that is removed, but just the reference?\n\nWell, it's the BdrvChild that's removed, that's what I meant by\n\"children\". But then the comment speaks of \"children list\" and means\ncreation of a list of BDSs, sooo... Yes, some change necessary.\n\nMax","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=mreitz@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 3yBsXc6Z3Yz9sNr\n\tfor ;\n\tWed, 11 Oct 2017 22:42:23 +1100 (AEDT)","from localhost ([::1]:40383 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e2FP3-0001mX-1Z\n\tfor incoming@patchwork.ozlabs.org; Wed, 11 Oct 2017 07:42:21 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:55782)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e2FOg-0001lY-3G\n\tfor qemu-devel@nongnu.org; Wed, 11 Oct 2017 07:41:59 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e2FOe-0004T2-V4\n\tfor qemu-devel@nongnu.org; Wed, 11 Oct 2017 07:41:58 -0400","from mx1.redhat.com ([209.132.183.28]:42686)\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 1e2FOa-0004Qk-Gs; Wed, 11 Oct 2017 07:41:52 -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 E537B820ED;\n\tWed, 11 Oct 2017 11:41:50 +0000 (UTC)","from dresden.str.redhat.com (unknown [10.40.205.86])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 839CB6C1FD;\n\tWed, 11 Oct 2017 11:41:41 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com E537B820ED","To":"Kevin Wolf ","References":"<20170913181910.29688-1-mreitz@redhat.com>\n\t<20170913181910.29688-3-mreitz@redhat.com>\n\t<20171010083651.GC4177@dhcp-200-186.str.redhat.com>","From":"Max Reitz ","Message-ID":"","Date":"Wed, 11 Oct 2017 13:41:39 +0200","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":"<20171010083651.GC4177@dhcp-200-186.str.redhat.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"VG7onOP7wIG1s3kIAInFME6fORSr2P3Ui\"","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.26]);\n\tWed, 11 Oct 2017 11:41:51 +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 02/18] block: BDS deletion during\n\tbdrv_drain_recurse","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":"Stefan Hajnoczi , John Snow ,\n\tFam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]