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

[Vladimir Sementsov-Ogievskiy]

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