Re: [Qemu-devel] [PATCH v5 25/42] mirror: Deal with filters
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
>>>
>>> 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)
_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, _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, _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,
_abort);
-bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs),
_abort);
+bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs,
_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
Re: [Qemu-devel] [PATCH v5 25/42] mirror: Deal with filters
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
>>
>> 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)
>>>_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, _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, _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,
>>>_abort);
>>> -bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs),
>>> _abort);
>>> +bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs,
>>> _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,
>>>
Re: [Qemu-devel] [PATCH v5 25/42] mirror: Deal with filters
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
>
> 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)
>> _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, _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, _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,
>> _abort);
>> -bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs),
>> _abort);
>> +bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs,
>> _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
Re: [Qemu-devel] [PATCH v5 25/42] mirror: Deal with filters
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
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)
> _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, _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, _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,
> _abort);
> -bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs),
> _abort);
> +bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs,
> _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?
Hmm, so this is the way to "skip filters reverse from the base", yes? Worth add
a comment?
> +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. */
> @@ -807,7 +813,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob
> *s)
> return 0;
> }
>
> -ret = bdrv_is_allocated_above(bs, base, offset, bytes, );
> +ret = bdrv_is_allocated_above(bs, filtered_base, offset, bytes,
> );
> if (ret < 0) {
> return ret;
> }
> @@ -903,7 +909,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
> } else {
> s->target_cluster_size = BDRV_SECTOR_SIZE;
> }
> -if (backing_filename[0] && !target_bs->backing &&
> +if (backing_filename[0] && !bdrv_backing_chain_next(target_bs) &&
> s->granularity < s->target_cluster_size) {
> s->buf_size = MAX(s->buf_size, s->target_cluster_size);
> s->cow_bitmap = bitmap_new(length);
> @@ -1083,8 +1089,9 @@ static void mirror_complete(Job *job, Error **errp)
> if (s->backing_mode == MIRROR_OPEN_BACKING_CHAIN) {
> int ret;
>
> -assert(!target->backing);
> -ret = bdrv_open_backing_file(target, NULL, "backing", errp);
> +assert(!bdrv_backing_chain_next(target));
> +ret = bdrv_open_backing_file(bdrv_skip_rw_filters(target), NULL,
> + "backing", errp);
> if (ret < 0) {
> return;
> }
> @@ -1503,8 +1510,8 @@ static void mirror_start_job(const char *job_id,
> BlockDriverState *bs,
> MirrorBlockJob *s;
> MirrorBDSOpaque *bs_opaque;
> BlockDriverState *mirror_top_bs;
> -bool target_graph_mod;
> bool target_is_backing;
> +uint64_t target_perms, target_shared_perms;
> Error *local_err = NULL;
> int ret;
>
> @@ -1523,7 +1530,7 @@ static void mirror_start_job(const char *job_id,
> BlockDriverState *bs,
> buf_size = DEFAULT_MIRROR_BUF_SIZE;
> }
>
> -if (bs == target) {
> +if (bdrv_skip_rw_filters(bs) == bdrv_skip_rw_filters(target)) {
> error_setg(errp, "Can't mirror node into itself");
> return;
> }
> @@ -1583,15 +1590,42 @@ static void mirror_start_job(const char *job_id,
>
