[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v14 08/14] block: Support dropping active in bdr
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH v14 08/14] block: Support dropping active in bdrv_drop_intermediate |
Date: |
Thu, 20 Feb 2014 16:34:15 +0800 |
User-agent: |
Mutt/1.5.22 (2013-10-16) |
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
- Re: [Qemu-devel] [PATCH v14 06/14] block: Add backing_blocker in BlockDriverState, (continued)
[Qemu-devel] [PATCH v14 07/14] block: Parse "backing" option to reference existing BDS, Fam Zheng, 2014/02/19
[Qemu-devel] [PATCH v14 08/14] block: Support dropping active in bdrv_drop_intermediate, Fam Zheng, 2014/02/19
[Qemu-devel] [PATCH v14 09/14] stream: Use bdrv_drop_intermediate and drop close_unused_images, Fam Zheng, 2014/02/19
[Qemu-devel] [PATCH v14 10/14] qmp: Add command 'blockdev-backup', Fam Zheng, 2014/02/19
[Qemu-devel] [PATCH v14 12/14] block: Add blockdev-backup to transaction, Fam Zheng, 2014/02/19
[Qemu-devel] [PATCH v14 11/14] block: Allow backup on referenced named BlockDriverState, Fam Zheng, 2014/02/19
[Qemu-devel] [PATCH v14 13/14] qemu-iotests: Test blockdev-backup in 055, Fam Zheng, 2014/02/19
[Qemu-devel] [PATCH v14 14/14] qemu-iotests: Image fleecing test case 081, Fam Zheng, 2014/02/19