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.
