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