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

[Max Reitz]

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

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

signature.asc
Description: OpenPGP digital signature