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