[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 25/42] mirror: Deal with filters
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v5 25/42] mirror: Deal with filters |
Date: |
Tue, 18 Jun 2019 16:47:30 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
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.
[...]
>> @@ -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
[Qemu-devel] [PATCH v5 24/42] block: Use child access functions for QAPI queries, Max Reitz, 2019/06/12
[Qemu-devel] [PATCH v5 26/42] backup: Deal with filters, Max Reitz, 2019/06/12
[Qemu-devel] [PATCH v5 27/42] commit: Deal with filters, Max Reitz, 2019/06/12
[Qemu-devel] [PATCH v5 31/42] block: Drop backing_bs(), Max Reitz, 2019/06/12