From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33646) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WnWJQ-00082n-Ae for qemu-devel@nongnu.org; Thu, 22 May 2014 12:57:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WnWJK-00081M-6B for qemu-devel@nongnu.org; Thu, 22 May 2014 12:57:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58130) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WnWJJ-00080s-Tz for qemu-devel@nongnu.org; Thu, 22 May 2014 12:57:42 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s4MGvdsH014329 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 22 May 2014 12:57:40 -0400 Date: Thu, 22 May 2014 12:57:36 -0400 From: Jeff Cody Message-ID: <20140522165736.GC6042@localhost.localdomain> References: <1400565880-13409-1-git-send-email-famz@redhat.com> <1400565880-13409-7-git-send-email-famz@redhat.com> <20140521140303.GG6619@stefanha-thinkpad.redhat.com> <20140521142448.GC13700@localhost.localdomain> <20140521143750.GB1891@T430.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140521143750.GB1891@T430.nay.redhat.com> Subject: Re: [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, armbru@redhat.com, hbrock@redhat.com, qemu-devel@nongnu.org, rjones@redhat.com, imain@redhat.com, Stefan Hajnoczi , pbonzini@redhat.com On Wed, May 21, 2014 at 10:37:50PM +0800, Fam Zheng wrote: > On Wed, 05/21 10:24, Jeff Cody wrote: > > On Wed, May 21, 2014 at 04:03:03PM +0200, Stefan Hajnoczi wrote: > > > On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote: > > > > diff --git a/block/mirror.c b/block/mirror.c > > > > index 1c38aa8..6a53d79 100644 > > > > --- a/block/mirror.c > > > > +++ b/block/mirror.c > > > > @@ -499,6 +499,7 @@ immediate_exit: > > > > * trigger the unref from the top one */ > > > > BlockDriverState *p = s->base->backing_hd; > > > > s->base->backing_hd = NULL; > > > > + bdrv_op_unblock_all(p, s->base->backing_blocker); > > > > bdrv_unref(p); > > > > } > > > > } > > > > > > Would be cleaner to call bdrv_set_backing_hd(s->base, NULL) here instead > > > of open coding it. > > > > > > > Patch 10 gets rid of essentially this whole chunk of code, and > > replaces it with bdrv_drop_intermediate(). So it does get cleaned up, > > just later in the series. > > Thanks for pointing out, Jeff. > > Stefan, if there are other reasons to respin, I'll take your suggestion and > update this, and then split the series, so you can merge 1-6. > Unfortunately, it will need a respin. I've been doing some more testing, and with just patches 1-6, block-commit asserts. The culprits are the open coded backing_hd assignments, like above, in bdrv_drop_intermediate() (there are two, and either will cause an assert): ret = bdrv_change_backing_file(new_top_bs, backing_file_str, base_bs->drv ? base_bs->drv->format_name : ""); if (ret) { goto exit; } >> new_top_bs->backing_hd = base_bs; And: QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { /* so that bdrv_close() does not recursively close the chain */ >> intermediate_state->bs->backing_hd = NULL; bdrv_unref(intermediate_state->bs); ^^^^^^^^^^^ This will assert Without the call to bdrv_set_backing_hd(), the backing_blocker does not get cleared, and this asserts in bdrv_delete(): assert(bdrv_op_blocker_is_empty(bs)); So all instances of open coded assignment of backing_hd should be replaced, either in this patch, or (preferably) in a new patch after patch 5.