Re: [Qemu-block] [PATCH v6 27/42] commit: Deal with filters
On 31.08.19 12:44, Vladimir Sementsov-Ogievskiy wrote:
> 09.08.2019 19:13, Max Reitz wrote:
>> This includes some permission limiting (for example, we only need to
>> take the RESIZE permission if the base is smaller than the top).
>>
>> Signed-off-by: Max Reitz
>> ---
>> block/block-backend.c | 16 +---
>> block/commit.c| 96 +++
>> blockdev.c| 6 ++-
>> 3 files changed, 85 insertions(+), 33 deletions(-)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index c13c5c83b0..0bc592d023 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -2180,11 +2180,17 @@ int blk_commit_all(void)
>> AioContext *aio_context = blk_get_aio_context(blk);
>>
>> aio_context_acquire(aio_context);
>> -if (blk_is_inserted(blk) && blk->root->bs->backing) {
>> -int ret = bdrv_commit(blk->root->bs);
>> -if (ret < 0) {
>> -aio_context_release(aio_context);
>> -return ret;
>> +if (blk_is_inserted(blk)) {
>> +BlockDriverState *non_filter;
>> +
>> +/* Legacy function, so skip implicit filters */
>> +non_filter = bdrv_skip_implicit_filters(blk->root->bs);
>> +if (bdrv_filtered_cow_child(non_filter)) {
>> +int ret = bdrv_commit(non_filter);
>> +if (ret < 0) {
>> +aio_context_release(aio_context);
>> +return ret;
>> +}
>> }
>
> and if non_filter is explicit filter we just skip it. I think we'd better
> return
> error in this case. For example, just drop if (bdrv_filtered_cow_child) and
> get
> ENOTSUP from bdrv_commit in this case.
Sounds good, yes.
> And with at least this fixed I'm OK with this patch:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy
>
> However some comments below:
>
>> }
>> aio_context_release(aio_context);
>> diff --git a/block/commit.c b/block/commit.c
>> index 5a7672c7c7..40d1c8eeac 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -37,6 +37,7 @@ typedef struct CommitBlockJob {
>> BlockBackend *top;
>> BlockBackend *base;
>> BlockDriverState *base_bs;
>> +BlockDriverState *above_base;
>
> you called it base_overlay in mirror, seems better to keep same naming
Indeed.
[...]
>> @@ -315,17 +336,43 @@ void commit_start(const char *job_id, BlockDriverState
>> *bs,
>>
>> s->commit_top_bs = commit_top_bs;
>>
>> -/* Block all nodes between top and base, because they will
>> - * disappear from the chain after this operation. */
>> -assert(bdrv_chain_contains(top, base));
>> -for (iter = top; iter != base; iter = backing_bs(iter)) {
>> -/* XXX BLK_PERM_WRITE needs to be allowed so we don't block
>> ourselves
>> - * at s->base (if writes are blocked for a node, they are also
>> blocked
>> - * for its backing file). The other options would be a second filter
>> - * driver above s->base. */
>
> This code part is absolutely equal to corresponding in block/mirror.c.. It
> would be great
> to put it into a function and reuse. However its not about these series.
It would probably be great to just drop block/commit.c altogether and
fully merge it into block/mirror.c at some point.
(I suppose we’d just have to check whether there’s any parent who’s
taken the WRITE permission on the top node, and if so, emit READY (and
if not, skip to COMPLETED).)
[...]
>> @@ -412,19 +457,22 @@ int bdrv_commit(BlockDriverState *bs)
>> if (!drv)
>> return -ENOMEDIUM;
>>
>> -if (!bs->backing) {
>> +backing_file_bs = bdrv_filtered_cow_bs(bs);
>
> Hmm just note: if in future we'll have cow child which is not bs->backing, a
> lot of code will
> fail, as we always assume that cow child is bs->backing. May be, this should
> be commented in
> bdrv_filtered_cow_child implementation.
I couldn’t see why we’d ever do this. I hope we never do.
(Aside from just removing bs->file and bs->backing altogether.)
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v6 27/42] commit: Deal with filters
09.08.2019 19:13, Max Reitz wrote:
> This includes some permission limiting (for example, we only need to
> take the RESIZE permission if the base is smaller than the top).
>
> Signed-off-by: Max Reitz
> ---
> block/block-backend.c | 16 +---
> block/commit.c| 96 +++
> blockdev.c| 6 ++-
> 3 files changed, 85 insertions(+), 33 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index c13c5c83b0..0bc592d023 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2180,11 +2180,17 @@ int blk_commit_all(void)
> AioContext *aio_context = blk_get_aio_context(blk);
>
> aio_context_acquire(aio_context);
> -if (blk_is_inserted(blk) && blk->root->bs->backing) {
> -int ret = bdrv_commit(blk->root->bs);
> -if (ret < 0) {
> -aio_context_release(aio_context);
> -return ret;
> +if (blk_is_inserted(blk)) {
> +BlockDriverState *non_filter;
> +
> +/* Legacy function, so skip implicit filters */
> +non_filter = bdrv_skip_implicit_filters(blk->root->bs);
> +if (bdrv_filtered_cow_child(non_filter)) {
> +int ret = bdrv_commit(non_filter);
> +if (ret < 0) {
> +aio_context_release(aio_context);
> +return ret;
> +}
> }
and if non_filter is explicit filter we just skip it. I think we'd better return
error in this case. For example, just drop if (bdrv_filtered_cow_child) and get
ENOTSUP from bdrv_commit in this case.
And with at least this fixed I'm OK with this patch:
Reviewed-by: Vladimir Sementsov-Ogievskiy
However some comments below:
> }
> aio_context_release(aio_context);
> diff --git a/block/commit.c b/block/commit.c
> index 5a7672c7c7..40d1c8eeac 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -37,6 +37,7 @@ typedef struct CommitBlockJob {
> BlockBackend *top;
> BlockBackend *base;
> BlockDriverState *base_bs;
> +BlockDriverState *above_base;
you called it base_overlay in mirror, seems better to keep same naming
> BlockdevOnError on_error;
> bool base_read_only;
> bool chain_frozen;
> @@ -110,7 +111,7 @@ static void commit_abort(Job *job)
>* XXX Can (or should) we somehow keep 'consistent read' blocked even
>* after the failed/cancelled commit job is gone? If we already wrote
>* something to base, the intermediate images aren't valid any more. */
> -bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
> +bdrv_replace_node(s->commit_top_bs, s->commit_top_bs->backing->bs,
> &error_abort);
>
> bdrv_unref(s->commit_top_bs);
> @@ -174,7 +175,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
> break;
> }
> /* Copy if allocated above the base */
> -ret = bdrv_is_allocated_above(blk_bs(s->top), blk_bs(s->base), false,
> +ret = bdrv_is_allocated_above(blk_bs(s->top), s->above_base, true,
> offset, COMMIT_BUFFER_SIZE, &n);
> copy = (ret == 1);
> trace_commit_one_iteration(s, offset, n, ret);
> @@ -267,15 +268,35 @@ void commit_start(const char *job_id, BlockDriverState
> *bs,
> CommitBlockJob *s;
> BlockDriverState *iter;
> BlockDriverState *commit_top_bs = NULL;
> +BlockDriverState *filtered_base;
> Error *local_err = NULL;
> +int64_t base_size, top_size;
> +uint64_t perms, iter_shared_perms;
> int ret;
>
> assert(top != bs);
> -if (top == base) {
> +if (bdrv_skip_rw_filters(top) == bdrv_skip_rw_filters(base)) {
> error_setg(errp, "Invalid files for merge: top and base are the
> same");
> return;
> }
>
> +base_size = bdrv_getlength(base);
> +if (base_size < 0) {
> +error_setg_errno(errp, -base_size, "Could not inquire base image
> size");
> +return;
> +}
> +
> +top_size = bdrv_getlength(top);
> +if (top_size < 0) {
> +error_setg_errno(errp, -top_size, "Could not inquire top image
> size");
> +return;
> +}
> +
> +perms = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;
> +if (base_size < top_size) {
> +perms |= BLK_PERM_RESIZE;
> +}
> +
> s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0,
> BLK_PERM_ALL,
>speed, creation_flags, NULL, NULL, errp);
> if (!s) {
> @@ -315,17 +336,43 @@ void commit_start(const char *job_id, BlockDriverState
> *bs,
>
> s->commit_top_bs = commit_top_bs;
>
> -/* Block all nodes between top and base, because they will
> - * disappear from the chain after this operation. */
> -assert(bdrv_chain_contains(top, base));
> -for (iter = top; iter !=
[Qemu-block] [PATCH v6 27/42] commit: Deal with filters
This includes some permission limiting (for example, we only need to
take the RESIZE permission if the base is smaller than the top).
Signed-off-by: Max Reitz
---
block/block-backend.c | 16 +---
block/commit.c| 96 +++
blockdev.c| 6 ++-
3 files changed, 85 insertions(+), 33 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index c13c5c83b0..0bc592d023 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2180,11 +2180,17 @@ int blk_commit_all(void)
AioContext *aio_context = blk_get_aio_context(blk);
aio_context_acquire(aio_context);
-if (blk_is_inserted(blk) && blk->root->bs->backing) {
-int ret = bdrv_commit(blk->root->bs);
-if (ret < 0) {
-aio_context_release(aio_context);
-return ret;
+if (blk_is_inserted(blk)) {
+BlockDriverState *non_filter;
+
+/* Legacy function, so skip implicit filters */
+non_filter = bdrv_skip_implicit_filters(blk->root->bs);
+if (bdrv_filtered_cow_child(non_filter)) {
+int ret = bdrv_commit(non_filter);
+if (ret < 0) {
+aio_context_release(aio_context);
+return ret;
+}
}
}
aio_context_release(aio_context);
diff --git a/block/commit.c b/block/commit.c
index 5a7672c7c7..40d1c8eeac 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -37,6 +37,7 @@ typedef struct CommitBlockJob {
BlockBackend *top;
BlockBackend *base;
BlockDriverState *base_bs;
+BlockDriverState *above_base;
BlockdevOnError on_error;
bool base_read_only;
bool chain_frozen;
@@ -110,7 +111,7 @@ static void commit_abort(Job *job)
* XXX Can (or should) we somehow keep 'consistent read' blocked even
* after the failed/cancelled commit job is gone? If we already wrote
* something to base, the intermediate images aren't valid any more. */
-bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
+bdrv_replace_node(s->commit_top_bs, s->commit_top_bs->backing->bs,
&error_abort);
bdrv_unref(s->commit_top_bs);
@@ -174,7 +175,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
break;
}
/* Copy if allocated above the base */
-ret = bdrv_is_allocated_above(blk_bs(s->top), blk_bs(s->base), false,
+ret = bdrv_is_allocated_above(blk_bs(s->top), s->above_base, true,
offset, COMMIT_BUFFER_SIZE, &n);
copy = (ret == 1);
trace_commit_one_iteration(s, offset, n, ret);
@@ -267,15 +268,35 @@ void commit_start(const char *job_id, BlockDriverState
*bs,
CommitBlockJob *s;
BlockDriverState *iter;
BlockDriverState *commit_top_bs = NULL;
+BlockDriverState *filtered_base;
Error *local_err = NULL;
+int64_t base_size, top_size;
+uint64_t perms, iter_shared_perms;
int ret;
assert(top != bs);
-if (top == base) {
+if (bdrv_skip_rw_filters(top) == bdrv_skip_rw_filters(base)) {
error_setg(errp, "Invalid files for merge: top and base are the same");
return;
}
+base_size = bdrv_getlength(base);
+if (base_size < 0) {
+error_setg_errno(errp, -base_size, "Could not inquire base image
size");
+return;
+}
+
+top_size = bdrv_getlength(top);
+if (top_size < 0) {
+error_setg_errno(errp, -top_size, "Could not inquire top image size");
+return;
+}
+
+perms = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;
+if (base_size < top_size) {
+perms |= BLK_PERM_RESIZE;
+}
+
s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, BLK_PERM_ALL,
speed, creation_flags, NULL, NULL, errp);
if (!s) {
@@ -315,17 +336,43 @@ void commit_start(const char *job_id, BlockDriverState
*bs,
s->commit_top_bs = commit_top_bs;
-/* Block all nodes between top and base, because they will
- * disappear from the chain after this operation. */
-assert(bdrv_chain_contains(top, base));
-for (iter = top; iter != base; iter = backing_bs(iter)) {
-/* XXX BLK_PERM_WRITE needs to be allowed so we don't block ourselves
- * at s->base (if writes are blocked for a node, they are also blocked
- * for its backing file). The other options would be a second filter
- * driver above s->base. */
+/*
+ * Block all nodes between top and base, because they will
+ * disappear from the chain after this operation.
+ * Note that this assumes that the user is fine with removing all
+ * nodes (including R/W filters) between top and base. Assuring
+ * this is the responsibility of the interface (i.e. whoever calls
+ * commit_start()).
+ */
+s->above_base = bdrv_find_overlay(t
