From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38374) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQdhx-0000tx-Dt for qemu-devel@nongnu.org; Wed, 25 Feb 2015 10:17:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YQdhs-0006U3-1D for qemu-devel@nongnu.org; Wed, 25 Feb 2015 10:17:05 -0500 Message-ID: <54EDD743.30403@redhat.com> Date: Wed, 25 Feb 2015 09:08:03 -0500 From: Max Reitz MIME-Version: 1.0 References: <1424792164-2130-1-git-send-email-mreitz@redhat.com> <1424792164-2130-5-git-send-email-mreitz@redhat.com> <20150225071200.GG5293@ad.nay.redhat.com> In-Reply-To: <20150225071200.GG5293@ad.nay.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 04/11] quorum: Fix close path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: Kevin Wolf , qemu-block@nongnu.org, qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi , Paolo Bonzini On 2015-02-25 at 02:12, Fam Zheng wrote: > On Tue, 02/24 10:35, Max Reitz wrote: >> bdrv_unref() can lead to bdrv_close(), which in turn will result in >> bdrv_drain_all(). This function will later be called blk_drain_all() and >> iterate only over the BlockBackends for which blk_is_inserted() holds >> true; therefore, bdrv_is_inserted() and thus quorum_is_inserted() will >> probably be called. >> >> This patch makes quorum_is_inserted() aware of the fact that some >> children may have been closed already. >> >> Signed-off-by: Max Reitz >> Reviewed-by: Eric Blake >> --- >> block/quorum.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/block/quorum.c b/block/quorum.c >> index 7a75cea..5ae2398 100644 >> --- a/block/quorum.c >> +++ b/block/quorum.c >> @@ -1005,6 +1005,7 @@ static void quorum_close(BlockDriverState *bs) >> >> for (i = 0; i < s->num_children; i++) { >> bdrv_unref(s->bs[i]); >> + s->bs[i] = NULL; > Is quorum_is_inserted called by bdrv_close in this bdrv_unref? Maybe. In any case, the BDS will still be valid at that point (it's called through bdrv_drain_all() which is called from bdrv_close() before it did any modifications to the BDS; also note that this bdrv_drain_all() is actually supposed to affect the BDS being closed since bdrv_make_anon() is called only afterwards). In effect it probably doesn't really matter whether to do the bdrv_unref() before or after s->bs[i] has been reset. If it is done before, quorum_is_inserted() will return true for the first BDS being closed (and false for all the others) which most probably won't do anything; if it is done after, quorum_is_inserted() will return false every time. Max > If yes, I think > unsetting s->bs[i] should happen before bdrv_unref(), so quorum_is_inserted > won't count this one again. > > Fam > >> } >> >> g_free(s->bs); >> @@ -1070,7 +1071,7 @@ static bool quorum_is_inserted(BlockDriverState *bs) >> int i; >> >> for (i = 0; i < s->num_children; i++) { >> - if (!bdrv_is_inserted(s->bs[i])) { >> + if (s->bs[i] && !bdrv_is_inserted(s->bs[i])) { >> return false; >> } >> } >> -- >> 2.1.0 >> >>