Re: [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState

[Jeff Cody]

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.