Re: [Qemu-devel] [PATCH v6 14/42] block: Use CAFs when working with backing chains

[Kevin Wolf]

Am 09.09.2019 um 10:25 hat Max Reitz geschrieben:
> On 05.09.19 16:05, Kevin Wolf wrote:
> > Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
> >> Use child access functions when iterating through backing chains so
> >> filters do not break the chain.
> >>
> >> Signed-off-by: Max Reitz <address@hidden>
> >> ---
> >>  block.c | 40 ++++++++++++++++++++++++++++------------
> >>  1 file changed, 28 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index 86b84bea21..42abbaf0ba 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -4376,7 +4376,8 @@ int bdrv_change_backing_file(BlockDriverState *bs,
> >>  }
> >>  
> >>  /*
> >> - * Finds the image layer in the chain that has 'bs' as its backing file.
> >> + * Finds the image layer in the chain that has 'bs' (or a filter on
> >> + * top of it) as its backing file.
> >>   *
> >>   * active is the current topmost image.
> >>   *
> >> @@ -4388,11 +4389,18 @@ int bdrv_change_backing_file(BlockDriverState *bs,
> >>  BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
> >>                                      BlockDriverState *bs)
> >>  {
> >> -    while (active && bs != backing_bs(active)) {
> >> -        active = backing_bs(active);
> >> +    bs = bdrv_skip_rw_filters(bs);
> >> +    active = bdrv_skip_rw_filters(active);
> > 
> > This does more than the commit message says. In addition to iterating
> > through filters instead of stopping, it also changes the semantics of
> > the function to return the next non-filter on top of bs instead of the
> > next node.
> 
> Which is to say the overlay.
> 
> (I think we only ever use “overlay” in the COW sense.)

I think we do, but so far also only ever for immediate COW childs, not
for skipping through intermediate node.

> > The block jobs seem to use it only for bdrv_is_allocated_above(), which
> > should return the same thing in both cases, so the behaviour stays the
> > same. qmp_block_commit() will check op blockers on a different node now,
> > which could be a fix or a bug, I can't tell offhand. Probably the
> > blocking doesn't really work anyway.
> 
> You mean that the op blocker could have been on a block job filter node
> before?  I think that‘s pretty much the point of this fix; that that
> doesn’t make sense.  (We didn’t have mirror_top_bs and the like at
> 058223a6e3b.)

On the off chance that the op blocker actually works, it can't be a job
filter. I was thinking more of throttling, blkdebug etc.

> > All of this should be mentioned in the commit message at least. Maybe
> > it's also worth splitting in two patches.
> 
> I don’t know.  The function was written when there were no filters.

I doubt it. blkdebug is a really old filter.

> This change would have been a no-op then.  The fact that it isn’t to me
> just means that introducing filters broke it.
> 
> So I don’t know what I would write.  Maybe “bdrv_find_overlay() now
> actually finds the overlay, that is, it will not return a filter node.
> This is the behavior that all callers expect (because they work on COW
> backing chains).”

Maybe just something like "In addition, filter nodes are not returned as
overlays any more. Instead, the first non-filter node on top of bs is
considered the overlay now."?

Kevin

signature.asc
Description: PGP signature