Re: [Qemu-block] [PATCH v6 28/42] stream: Deal with filters
Am 16.09.2019 um 11:52 hat Max Reitz geschrieben:
> On 13.09.19 16:16, Kevin Wolf wrote:
> > Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
> >> @@ -261,16 +272,19 @@ void stream_start(const char *job_id,
> >> BlockDriverState *bs,
> >> * disappear from the chain after this operation. The streaming job
> >> reads
> >> * every block only once, assuming that it doesn't change, so forbid
> >> writes
> >> * and resizes. Reassign the base node pointer because the backing BS
> >> of the
> >> - * bottom node might change after the call to
> >> bdrv_reopen_set_read_only()
> >> - * due to parallel block jobs running.
> >> + * above_base node might change after the call to
> >> + * bdrv_reopen_set_read_only() due to parallel block jobs running.
> >> */
> >> -base = backing_bs(bottom);
> >> -for (iter = backing_bs(bs); iter && iter != base; iter =
> >> backing_bs(iter)) {
> >> +base = bdrv_filtered_bs(above_base);
> >
> > We just calculated above_base such that it's the parent of base. Why
> > would base not already have the value we're assigning it again here?
>
> That’s no change to existing code, whose reasoning is explained in the
> comment above: bdrv_reopen_set_read_only() can yield, which might lead
> to children of the bottom node changing.
>
> If you feel like either that’s superfluous, or that if something like
> that were to happen we’d have much bigger problems, be my guest to drop
> both.
>
> But in this series I’d rather just not change it.
Ah, you mean comments are there to be read?
But actually, I think iterating down to base is too much anyway. The
reasoning in the comment for block_job_add_bdrv() is that the nodes will
be dropped at the end. But base with all of its filter will be kept
after this patch.
So I think the for loop should stop after bs->base_overlay. And then
concurrently changing links aren't even a problem any more because
that's exactly the place up to which we've frozen the chain.
Kevin
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v6 28/42] stream: Deal with filters
On 13.09.19 16:16, Kevin Wolf wrote:
> Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
>> Because of the recent changes that make the stream job independent of
>> the base node and instead track the node above it, we have to split that
>> "bottom" node into two cases: The bottom COW node, and the node directly
>> above the base node (which may be an R/W filter or the bottom COW node).
>>
>> Signed-off-by: Max Reitz
>> ---
>> qapi/block-core.json | 4
>> block/stream.c | 52
>> blockdev.c | 2 +-
>> 3 files changed, 38 insertions(+), 20 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 38c4dbd7c3..3c54717870 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2516,6 +2516,10 @@
>> # On successful completion the image file is updated to drop the backing
>> file
>> # and the BLOCK_JOB_COMPLETED event is emitted.
>> #
>> +# In case @device is a filter node, block-stream modifies the first
>> non-filter
>> +# overlay node below it to point to base's backing node (or NULL if @base
>> was
>> +# not specified) instead of modifying @device itself.
>> +#
>> # @job-id: identifier for the newly-created block job. If
>> # omitted, the device name will be used. (Since 2.7)
>> #
>> diff --git a/block/stream.c b/block/stream.c
>> index 4c8b89884a..bd4a351dae 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -31,7 +31,8 @@ enum {
>>
>> typedef struct StreamBlockJob {
>> BlockJob common;
>> -BlockDriverState *bottom;
>> +BlockDriverState *bottom_cow_node;
>> +BlockDriverState *above_base;
>
> Confusing naming, especially because in commit you used above_base for
> what is bottom_cow_node here. Vladimir already suggested using
> base_overlay consistently, so we should do this here too (for
> bottom_cow_node). above_base can keep its name because the different
> above_base in commit is going to be renamed).
Sure.
>> BlockdevOnError on_error;
>> char *backing_file_str;
>> bool bs_read_only;
>> @@ -54,7 +55,7 @@ static void stream_abort(Job *job)
>>
>> if (s->chain_frozen) {
>> BlockJob *bjob = >common;
>> -bdrv_unfreeze_chain(blk_bs(bjob->blk), s->bottom);
>> +bdrv_unfreeze_chain(blk_bs(bjob->blk), s->above_base);
>> }
>> }
>>
>> @@ -63,14 +64,15 @@ static int stream_prepare(Job *job)
>> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>> BlockJob *bjob = >common;
>> BlockDriverState *bs = blk_bs(bjob->blk);
>> -BlockDriverState *base = backing_bs(s->bottom);
>> +BlockDriverState *unfiltered_bs = bdrv_skip_rw_filters(bs);
>> +BlockDriverState *base = bdrv_filtered_bs(s->above_base);
>> Error *local_err = NULL;
>> int ret = 0;
>>
>> -bdrv_unfreeze_chain(bs, s->bottom);
>> +bdrv_unfreeze_chain(bs, s->above_base);
>> s->chain_frozen = false;
>>
>> -if (bs->backing) {
>> +if (bdrv_filtered_cow_child(unfiltered_bs)) {
>> const char *base_id = NULL, *base_fmt = NULL;
>> if (base) {
>> base_id = s->backing_file_str;
>> @@ -78,8 +80,8 @@ static int stream_prepare(Job *job)
>> base_fmt = base->drv->format_name;
>> }
>> }
>> -bdrv_set_backing_hd(bs, base, _err);
>> -ret = bdrv_change_backing_file(bs, base_id, base_fmt);
>> +bdrv_set_backing_hd(unfiltered_bs, base, _err);
>> +ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt);
>> if (local_err) {
>> error_report_err(local_err);
>> return -EPERM;
>> @@ -110,7 +112,8 @@ static int coroutine_fn stream_run(Job *job, Error
>> **errp)
>> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>> BlockBackend *blk = s->common.blk;
>> BlockDriverState *bs = blk_bs(blk);
>> -bool enable_cor = !backing_bs(s->bottom);
>> +BlockDriverState *unfiltered_bs = bdrv_skip_rw_filters(bs);
>> +bool enable_cor = !bdrv_filtered_bs(s->above_base);
>> int64_t len;
>> int64_t offset = 0;
>> uint64_t delay_ns = 0;
>> @@ -119,7 +122,7 @@ static int coroutine_fn stream_run(Job *job, Error
>> **errp)
>> int64_t n = 0; /* bytes */
>> void *buf;
>>
>> -if (bs == s->bottom) {
>> +if (unfiltered_bs == s->bottom_cow_node) {
>> /* Nothing to stream */
>> return 0;
>> }
>> @@ -154,13 +157,14 @@ static int coroutine_fn stream_run(Job *job, Error
>> **errp)
>>
>> copy = false;
>>
>> -ret = bdrv_is_allocated(bs, offset, STREAM_BUFFER_SIZE, );
>> +ret = bdrv_is_allocated(unfiltered_bs, offset, STREAM_BUFFER_SIZE,
>> );
>> if (ret == 1) {
>> /* Allocated in the top, no need to copy. */
>> } else if (ret >= 0) {
>> /* Copy if allocated in the intermediate images. Limit to the
>> *
Re: [Qemu-block] [PATCH v6 28/42] stream: Deal with filters
Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
> Because of the recent changes that make the stream job independent of
> the base node and instead track the node above it, we have to split that
> "bottom" node into two cases: The bottom COW node, and the node directly
> above the base node (which may be an R/W filter or the bottom COW node).
>
> Signed-off-by: Max Reitz
> ---
> qapi/block-core.json | 4
> block/stream.c | 52
> blockdev.c | 2 +-
> 3 files changed, 38 insertions(+), 20 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 38c4dbd7c3..3c54717870 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2516,6 +2516,10 @@
> # On successful completion the image file is updated to drop the backing file
> # and the BLOCK_JOB_COMPLETED event is emitted.
> #
> +# In case @device is a filter node, block-stream modifies the first
> non-filter
> +# overlay node below it to point to base's backing node (or NULL if @base was
> +# not specified) instead of modifying @device itself.
> +#
> # @job-id: identifier for the newly-created block job. If
> # omitted, the device name will be used. (Since 2.7)
> #
> diff --git a/block/stream.c b/block/stream.c
> index 4c8b89884a..bd4a351dae 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -31,7 +31,8 @@ enum {
>
> typedef struct StreamBlockJob {
> BlockJob common;
> -BlockDriverState *bottom;
> +BlockDriverState *bottom_cow_node;
> +BlockDriverState *above_base;
Confusing naming, especially because in commit you used above_base for
what is bottom_cow_node here. Vladimir already suggested using
base_overlay consistently, so we should do this here too (for
bottom_cow_node). above_base can keep its name because the different
above_base in commit is going to be renamed).
> BlockdevOnError on_error;
> char *backing_file_str;
> bool bs_read_only;
> @@ -54,7 +55,7 @@ static void stream_abort(Job *job)
>
> if (s->chain_frozen) {
> BlockJob *bjob = >common;
> -bdrv_unfreeze_chain(blk_bs(bjob->blk), s->bottom);
> +bdrv_unfreeze_chain(blk_bs(bjob->blk), s->above_base);
> }
> }
>
> @@ -63,14 +64,15 @@ static int stream_prepare(Job *job)
> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> BlockJob *bjob = >common;
> BlockDriverState *bs = blk_bs(bjob->blk);
> -BlockDriverState *base = backing_bs(s->bottom);
> +BlockDriverState *unfiltered_bs = bdrv_skip_rw_filters(bs);
> +BlockDriverState *base = bdrv_filtered_bs(s->above_base);
> Error *local_err = NULL;
> int ret = 0;
>
> -bdrv_unfreeze_chain(bs, s->bottom);
> +bdrv_unfreeze_chain(bs, s->above_base);
> s->chain_frozen = false;
>
> -if (bs->backing) {
> +if (bdrv_filtered_cow_child(unfiltered_bs)) {
> const char *base_id = NULL, *base_fmt = NULL;
> if (base) {
> base_id = s->backing_file_str;
> @@ -78,8 +80,8 @@ static int stream_prepare(Job *job)
> base_fmt = base->drv->format_name;
> }
> }
> -bdrv_set_backing_hd(bs, base, _err);
> -ret = bdrv_change_backing_file(bs, base_id, base_fmt);
> +bdrv_set_backing_hd(unfiltered_bs, base, _err);
> +ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt);
> if (local_err) {
> error_report_err(local_err);
> return -EPERM;
> @@ -110,7 +112,8 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> BlockBackend *blk = s->common.blk;
> BlockDriverState *bs = blk_bs(blk);
> -bool enable_cor = !backing_bs(s->bottom);
> +BlockDriverState *unfiltered_bs = bdrv_skip_rw_filters(bs);
> +bool enable_cor = !bdrv_filtered_bs(s->above_base);
> int64_t len;
> int64_t offset = 0;
> uint64_t delay_ns = 0;
> @@ -119,7 +122,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
> int64_t n = 0; /* bytes */
> void *buf;
>
> -if (bs == s->bottom) {
> +if (unfiltered_bs == s->bottom_cow_node) {
> /* Nothing to stream */
> return 0;
> }
> @@ -154,13 +157,14 @@ static int coroutine_fn stream_run(Job *job, Error
> **errp)
>
> copy = false;
>
> -ret = bdrv_is_allocated(bs, offset, STREAM_BUFFER_SIZE, );
> +ret = bdrv_is_allocated(unfiltered_bs, offset, STREAM_BUFFER_SIZE,
> );
> if (ret == 1) {
> /* Allocated in the top, no need to copy. */
> } else if (ret >= 0) {
> /* Copy if allocated in the intermediate images. Limit to the
> * known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE).
> */
> -ret = bdrv_is_allocated_above(backing_bs(bs), s->bottom, true,
> +ret =
>
Re: [Qemu-block] [PATCH v6 28/42] stream: Deal with filters
09.08.2019 19:13, Max Reitz wrote: > Because of the recent changes that make the stream job independent of > the base node and instead track the node above it, we have to split that > "bottom" node into two cases: The bottom COW node, and the node directly > above the base node (which may be an R/W filter or the bottom COW node). > > Signed-off-by: Max Reitz Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
[Qemu-block] [PATCH v6 28/42] stream: Deal with filters
Because of the recent changes that make the stream job independent of
the base node and instead track the node above it, we have to split that
"bottom" node into two cases: The bottom COW node, and the node directly
above the base node (which may be an R/W filter or the bottom COW node).
Signed-off-by: Max Reitz
---
qapi/block-core.json | 4
block/stream.c | 52
blockdev.c | 2 +-
3 files changed, 38 insertions(+), 20 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 38c4dbd7c3..3c54717870 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2516,6 +2516,10 @@
# On successful completion the image file is updated to drop the backing file
# and the BLOCK_JOB_COMPLETED event is emitted.
#
+# In case @device is a filter node, block-stream modifies the first non-filter
+# overlay node below it to point to base's backing node (or NULL if @base was
+# not specified) instead of modifying @device itself.
+#
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
diff --git a/block/stream.c b/block/stream.c
index 4c8b89884a..bd4a351dae 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,7 +31,8 @@ enum {
typedef struct StreamBlockJob {
BlockJob common;
-BlockDriverState *bottom;
+BlockDriverState *bottom_cow_node;
+BlockDriverState *above_base;
BlockdevOnError on_error;
char *backing_file_str;
bool bs_read_only;
@@ -54,7 +55,7 @@ static void stream_abort(Job *job)
if (s->chain_frozen) {
BlockJob *bjob = >common;
-bdrv_unfreeze_chain(blk_bs(bjob->blk), s->bottom);
+bdrv_unfreeze_chain(blk_bs(bjob->blk), s->above_base);
}
}
@@ -63,14 +64,15 @@ static int stream_prepare(Job *job)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockJob *bjob = >common;
BlockDriverState *bs = blk_bs(bjob->blk);
-BlockDriverState *base = backing_bs(s->bottom);
+BlockDriverState *unfiltered_bs = bdrv_skip_rw_filters(bs);
+BlockDriverState *base = bdrv_filtered_bs(s->above_base);
Error *local_err = NULL;
int ret = 0;
-bdrv_unfreeze_chain(bs, s->bottom);
+bdrv_unfreeze_chain(bs, s->above_base);
s->chain_frozen = false;
-if (bs->backing) {
+if (bdrv_filtered_cow_child(unfiltered_bs)) {
const char *base_id = NULL, *base_fmt = NULL;
if (base) {
base_id = s->backing_file_str;
@@ -78,8 +80,8 @@ static int stream_prepare(Job *job)
base_fmt = base->drv->format_name;
}
}
-bdrv_set_backing_hd(bs, base, _err);
-ret = bdrv_change_backing_file(bs, base_id, base_fmt);
+bdrv_set_backing_hd(unfiltered_bs, base, _err);
+ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt);
if (local_err) {
error_report_err(local_err);
return -EPERM;
@@ -110,7 +112,8 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockBackend *blk = s->common.blk;
BlockDriverState *bs = blk_bs(blk);
-bool enable_cor = !backing_bs(s->bottom);
+BlockDriverState *unfiltered_bs = bdrv_skip_rw_filters(bs);
+bool enable_cor = !bdrv_filtered_bs(s->above_base);
int64_t len;
int64_t offset = 0;
uint64_t delay_ns = 0;
@@ -119,7 +122,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
int64_t n = 0; /* bytes */
void *buf;
-if (bs == s->bottom) {
+if (unfiltered_bs == s->bottom_cow_node) {
/* Nothing to stream */
return 0;
}
@@ -154,13 +157,14 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
copy = false;
-ret = bdrv_is_allocated(bs, offset, STREAM_BUFFER_SIZE, );
+ret = bdrv_is_allocated(unfiltered_bs, offset, STREAM_BUFFER_SIZE, );
if (ret == 1) {
/* Allocated in the top, no need to copy. */
} else if (ret >= 0) {
/* Copy if allocated in the intermediate images. Limit to the
* known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE). */
-ret = bdrv_is_allocated_above(backing_bs(bs), s->bottom, true,
+ret = bdrv_is_allocated_above(bdrv_filtered_cow_bs(unfiltered_bs),
+ s->bottom_cow_node, true,
offset, n, );
/* Finish early if end of backing file has been reached */
if (ret == 0 && n == 0) {
@@ -231,9 +235,16 @@ void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *iter;
bool bs_read_only;
int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
-BlockDriverState *bottom = bdrv_find_overlay(bs, base);
+BlockDriverState *bottom_cow_node =
