On 18.06.19 16:55, Vladimir Sementsov-Ogievskiy wrote: > 18.06.2019 17:47, Max Reitz wrote: >> On 18.06.19 15:12, Vladimir Sementsov-Ogievskiy wrote: >>> 13.06.2019 1:09, Max Reitz wrote: >>>> This includes some permission limiting (for example, we only need to >>>> take the RESIZE permission for active commits where the base is smaller >>>> than the top). >>>> >>>> Signed-off-by: Max Reitz >>> >>> ohm, unfortunately I'm far from knowing block/mirror mechanics and interfaces :(. >>> >>> still some comments below. >>> >>>> --- >>>> block/mirror.c | 110 +++++++++++++++++++++++++++++++++++++------------ >>>> blockdev.c | 47 +++++++++++++++++---- >>>> 2 files changed, 124 insertions(+), 33 deletions(-) >>>> >>>> diff --git a/block/mirror.c b/block/mirror.c >>>> index 4fa8f57c80..3d767e3030 100644 >>>> --- a/block/mirror.c >>>> +++ b/block/mirror.c >>>> @@ -660,8 +660,10 @@ static int mirror_exit_common(Job *job) >>>> &error_abort); >>>> if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) { >>>> BlockDriverState *backing = s->is_none_mode ? src : s->base; >>>> - if (backing_bs(target_bs) != backing) { >>>> - bdrv_set_backing_hd(target_bs, backing, &local_err); >>>> + BlockDriverState *unfiltered_target = bdrv_skip_rw_filters(target_bs); >>>> + >>>> + if (bdrv_filtered_cow_bs(unfiltered_target) != backing) { >>>> + bdrv_set_backing_hd(unfiltered_target, backing, &local_err); >>>> if (local_err) { >>>> error_report_err(local_err); >>>> ret = -EPERM; >>>> @@ -711,7 +713,7 @@ static int mirror_exit_common(Job *job) >>>> block_job_remove_all_bdrv(bjob); >>>> bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL, >>>> &error_abort); >>>> - bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort); >>>> + bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs, &error_abort); >>>> >>>> /* We just changed the BDS the job BB refers to (with either or both of the >>>> * bdrv_replace_node() calls), so switch the BB back so the cleanup does >>>> @@ -757,6 +759,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) >>>> { >>>> int64_t offset; >>>> BlockDriverState *base = s->base; >>>> + BlockDriverState *filtered_base; >>>> BlockDriverState *bs = s->mirror_top_bs->backing->bs; >>>> BlockDriverState *target_bs = blk_bs(s->target); >>>> int ret; >>>> @@ -795,6 +798,9 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) >>>> s->initial_zeroing_ongoing = false; >>>> } >>>> >>>> + /* Will be NULL if @base is not in @bs's chain */ >>> >>> Should we assert that not NULL? >> >> Well, but it can be NULL. It is only non-NULL for active commit. >> >>> Hmm, so this is the way to "skip filters reverse from the base", yes? Worth add a comment? >> >> We need this because bdrv_is_allocated() will report everything as >> allocated in a filter if it is allocated in its filtered child. So if >> we use @base in bdrv_is_allocated_above() and there is a filter on top >> of it, bdrv_is_allocated_above() will report everything as allocated >> that is allocated in @base (which we do not want). >> >> Therefor, we need to go to the topmost R/W filter on top of @base, so >> that bdrv_is_allocated_above() actually starts at the first COW chain >> element above @base. >> >> As for the comment, I thought the name “filtered base” would suffice, >> but sure. >> >> (“@filtered_base is the topmost node in the @bs-@base chain that is >> connected to @base only through filters” or something; plus the >> explanation why we need it.) >> >>>> + filtered_base = bdrv_filtered_cow_bs(bdrv_find_overlay(bs, base)); >>>> + >>>> /* First part, loop on the sectors and initialize the dirty bitmap. */ >>>> for (offset = 0; offset < s->bdev_length; ) { >>>> /* Just to make sure we are not exceeding int limit. */ >> >> [...] >> >>>> @@ -1583,15 +1590,42 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs, >>>> * In the case of active commit, things look a bit different, though, >>>> * because the target is an already populated backing file in active use. >>>> * We can allow anything except resize there.*/ >>>> + >>>> + target_perms = BLK_PERM_WRITE; >>>> + target_shared_perms = BLK_PERM_WRITE_UNCHANGED; >>>> + >>>> target_is_backing = bdrv_chain_contains(bs, target); >>> >>> don't you want skip filters here? actual target node may be in backing chain, but has separate >>> filters above it >> >> I don’t quite understand. bdrv_chain_contains() iterates over the >> filter chain, so it shouldn’t matter whether there are filters above >> target or not. >> >> [...] > > > I just imagine something like this: > > bs > | > ... target node (it's filter) > | / > v v > base (unfiltered target) Well, that’s just broken. Good point. Hm. Can this be actually made to work? The filter_target search could be amended (by looking for the overlay of bdrv_skip_rw_filters(target)). The loop to block intermediate nodes, though... Would require some more modifications. We’d probably also need two loops, one from bs to bdrv_skip_rw_filters(target), and one from the target to bdrv_skip_rw_filters(target). All in all I think it’s best to just forbid this case for now. (We can try something like that again for blockdev-copy in the future(TM).) So I’ll just check whether bdrv_skip_rw_filters(target) is in the chain, and if so (but target_is_backing is false), return an error. Max >>>> @@ -1641,15 +1675,39 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs, >>>> /* In commit_active_start() all intermediate nodes disappear, so >>>> * any jobs in them must be blocked */ >>> >>> hmm, preexisting, it s/jobs/nodes/ >> >> I think the idea was that no other jobs may be run on intermediate >> nodes. (But by now it’s no longer just about jobs, so yes, should be >> s/jobs/nodes/. I don’t know whether I should squeeze that in here, though.) >> >> Max >> > >