Re: [Qemu-devel] [PATCH 1/2] iotests: 030 TestParallelOps non-shared base node

[Kevin Wolf]

Am 21.03.2019 um 18:05 hat Andrey Shinkevich geschrieben:
> 
> 
> On 21/03/2019 13:53, Kevin Wolf wrote:
> > Am 20.03.2019 um 18:02 hat Alberto Garcia geschrieben:
> >> On Wed 20 Mar 2019 10:16:10 AM CET, Kevin Wolf wrote:
> >>>> Oh, I see. Let's use a shorter chain for simplicity:
> >>>>
> >>>>     A <- B <- C <- D <- E
> >>>
> >>> Written from right to left, i.e. E being the base and A the top layer?
> >>> We usually write things the other write round, I hope this doesn't get
> >>> too confusing later.
> >>
> >> Oh my... yes, of course you're right, I should have written it the other
> >> way around:
> >>
> >>     E <- D <- C <- B <- A
> >>
> >>>> 1) If we stream first from E to C we add a filter F:
> >>>>
> >>>>     A <- B <- F <- C <- D <- E
> >>
> >> ( which should have been written   E <- D <- C <- F <- B <- A )
> >>
> >>>>     Now we can't stream from C to A because F is on the way, and the F-C
> >>>>     link is frozen.
> >>>
> >>> Why is a frozen link a problem? The streaming operation isn't going to
> >>> change this link, it just copies data from the subchain (including F
> >>> and C) to A. This is not something that a frozen link should prevent.
> >>
> >> Not the operation itself, but the first thing that block-stream does is
> >> freeze the chain from top (A) to base (C), so this would fail if there's
> >> already a frozen link on the way (C <- F on this case?).
> > 
> > Oh, I see. I think this is why I suggested a counter originally instead
> > of a bool.
> > 
> >>> So it seems frozen links allow the wrong case, but block the correct
> >>> one? :-(
> >>
> >> Yes, we probably need to rethink this scenario a bit.
> > 
> > But yes, even with a counter, the other problem would still remain (that
> > the first job can't remove the filter on completion because the second
> > one has frozen its link to the filter).
> 
> With this example E <- D <- C <- F <- B <- A,
> 
> In the current implementation of the copy-on-read filter,
> its bs->backing is not initialized (while it is not true for the filter
> in block-commit job). So, bdrv_freeze_backing_chain() doesn't go beyond 
> the cor-filter node. With the two parallel block-stream jobs, we get the
> following sub-chains frozen:
> F <- B <- A
> E <- D <- C
> as C <- F backing BdrvChild link doesn't exist.

Hm, yes, but that's more by accident than a proper design. Of course,
freezing the chain shouldn't be stopped by a filter.

Currently, bdrv_freeze_backing_chain() simply stops if we reach a point
where the backing chain ends, even if we haven't reached the base. I
think this should be an assertion failure because the function should
never be called with a base that isn't even in the backing chain.

> If the cor-filter is inserted with the bdrv_append(), we get two
> BdrvChild links (file and backing) pointed to the same BlockDriverState
> 'C' and additionally some child-permissions issues that I have not 
> resolved yet...
> Due to the fact mentioned above, freezing the backing chain works with
> the filter inserted. But, with the one BdrvChild *file link only in the
> BlockDriverState of the cor-filter, we encounter a broken chain each
> time we iterate through it with the backing_bs(F) (=NULL) in many other
> pieces of the code. In turn, it breaks the existing model.
> That's the point! (((
> What can we do with that?

I think Max's series "Deal with filters" might be very helpful for this
kind of thing. With it, bdrv_freeze_backing_chain() can easily be made
work even across the cor filter.

Basically, the solution in this series is that it replaces backing_bs()
with different other functions depending on what is really wanted.
Amonst others, it introduces a function to get to the next non-filter in
the backing file chain, no matter whether it has to go through bs->file
or bs->backing, which is probably the right replacement in your case.

Kevin