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. 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? In my patch Stream-block-job-involves-copy-on-read-filter.patch : static BlockDriverState *child_file_bs(BlockDriverState *bs) { return bs->file ? bs->file->bs : NULL; } static BlockDriverState *skip_filter(BlockDriverState *bs) { BlockDriverState *ret_bs = bs; while (ret_bs && ret_bs->drv && ret_bs->drv->is_filter) { ret_bs = child_file_bs(ret_bs); } return ret_bs; } But the solution above looks clumsy to me. I would appreciate to hear any other ideas from you. Andrey > > I don't think that's a case we want to just forbid because nobody needs > this anyway. It's a sign of a more fundamental problem in our design, > and I'm sure it will bite us in other places, too. > > Kevin >