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 <mre...@redhat.com> >> >> 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) > >>> @@ -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 > -- Best regards, Vladimir