Re: [Qemu-devel] [PATCH v5 25/42] mirror: Deal with filters

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5209 bytes --]

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 <mreitz@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.

[...]

>> @@ -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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]