Re: [Qemu-devel] [PATCH v14 08/14] block: Support dropping active in bdrv_drop_intermediate

[Fam Zheng <famz@redhat.com>]

On Thu, 02/20 00:57, Jeff Cody wrote:
> On Thu, Feb 20, 2014 at 12:37:17PM +0800, Fam Zheng wrote:
> > On Wed, 02/19 18:24, Jeff Cody wrote:
> > > On Wed, Feb 19, 2014 at 04:22:30PM -0500, Jeff Cody wrote:
> > > > On Wed, Feb 19, 2014 at 09:42:25PM +0800, Fam Zheng wrote:
> > > > >  /*
> > > > > - * Drops images above 'base' up to and including 'top', and sets the image
> > > > > - * above 'top' to have base as its backing file.
> > > > > + * Drops images above 'base' up to and including 'top', and sets new 'base'
> > > > > + * as backing_hd of top_overlay (the image orignally has 'top' as backing
> > > > 
> > > > What is 'top_overlay'?  Do you mean "top's overlay" by this?
> > 
> > Yes, as noted in the parenthesis.
> >
> 
> I would just say "top's overlay".  What I found confusing by that, is
> when you reference something like 'top_overlay', it looks like an
> actual variable name.  So I was searching for that variable name, and
> wondered if it was just vestigial from an earlier revision.  Maybe
> that is just me, though :)
> 

I will update the wording for less confusion. Sorry about that.

> > > > And in the non-active case here, everything between top->backing_hd
> > > > and the original base is orphaned as well.  These should all be
> > > > explicitly unreferenced.
> > > 
> > > Same here, bdrv_unref() will eventually go through the chain, starting
> > > from top->backing_hd.  But this is a problem; won't we end up in a
> > > loop then?
> > 
> > Although the content is swapped, the pointer is not:
> > 
> > (I presume your "[base]" and "[top]" are denoting content, not pointer)
> >
> 
> Correct.  But part of the content that is swapped, are the backing_hd
> pointers.
> 
> > > 
> > > Take this chain:
> > > 
> > > drop_start = [A]
> > > 
> > >     |||-- ([base]) <-- [B] <--- [A] <--- ([top]) <--- [active]
> >                ^                              ^
> >                |                              |
> >               base                           top
> > > 
> > > 
> > > bdrv_swap(top, base):
> > > 
> > >     -- [B] <-- [A] <-- ([top])    |||--- ([base]) <-- [active]
> >                             ^                 ^
> >                             |                 |
> >                            base               top
> > >     |                    ^
> > >     |                    |
> > >     ---------------------
> > > 
> 
> Correct, those are the pointers.
> 
> > > Then we call bdrv_unref(drop_start (or bdrv_set_backing_hd() does),
> > > and we end up with:
> > > 
> 
> dropping an anchor here: [1]
> 
> > > bdrv_unref(A)
> > >     bdrv_unref(B)
> > >         bdrv_unref(top)
> > >             bdrv_unref(A) <--- assert
> > >                 .....
> > >             
> > > 
> > > So I think we want this line:
> > > 
> > > > > +            bdrv_set_backing_hd(base, NULL);
> > 
> > so, this breaks the chain,
> 
> Yes, you are right, we want base->backing_hd to be NULL.  But the
> chain has not been broken yet.
> 
> The loop [1] still exists, because once we enter bdrv_set_backing_hd()
> we begin to call bdrv_unref(A). And base_ptr->backing_hd still points
> to A, and B will point to base_ptr.

Yes, that need to be fixed.

> 
> Here is the first part of bdrv_set_backing_hd():
>     if (bs->backing_hd) {
>         bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
>         bdrv_unref(bs->backing_hd);
> 
> 
> > 
> > > 
> > > To be:
> > > 
> > > > > +            bdrv_set_backing_hd(top, NULL);
> > 
> > This will lose track of original base's backing_hd.
> 
> Right, we don't want that, sorry...  I shouldn't have written that, my
> brain failed me.  I mentally conflated top and [top].
> 
> > 
> > So I think we are OK here.
> >
> 
> I don't think we are, we still need to address the backing_hd loop,
> and I think it needs to be done here, where we have the information.

Again, you are right :)

Thanks,
Fam