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 <mre...@redhat.com>
> ---
> 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 <vsement...@virtuozzo.com>
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 != 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.
> + /*
> + * 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(top, base);
> + assert(s->above_base);
> +
> + /*
> + * The topmost node with
> + * bdrv_skip_rw_filters(filtered_base) == bdrv_skip_rw_filters(base)
> + */
> + filtered_base = bdrv_filtered_cow_bs(s->above_base);
> + assert(bdrv_skip_rw_filters(filtered_base) ==
> bdrv_skip_rw_filters(base));
> +
> + /*
> + * 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.
> + */
> + iter_shared_perms = BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE;
> +
> + for (iter = top; iter != base; iter = bdrv_filtered_bs(iter)) {
> + if (iter == filtered_base) {
> + /*
> + * From here on, all nodes are filters on the base. This
> + * allows us to share BLK_PERM_CONSISTENT_READ.
> + */
> + iter_shared_perms |= BLK_PERM_CONSISTENT_READ;
> + }
> +
> ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
> - BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE,
> - errp);
> + iter_shared_perms, errp);
> if (ret < 0) {
> goto fail;
> }
> @@ -342,9 +389,7 @@ void commit_start(const char *job_id, BlockDriverState
> *bs,
> }
>
> s->base = blk_new(s->common.job.aio_context,
> - BLK_PERM_CONSISTENT_READ
> - | BLK_PERM_WRITE
> - | BLK_PERM_RESIZE,
> + perms,
> BLK_PERM_CONSISTENT_READ
> | BLK_PERM_GRAPH_MOD
> | BLK_PERM_WRITE_UNCHANGED);
> @@ -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.
> +
> + if (!backing_file_bs) {
> return -ENOTSUP;
> }
>
> if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT_SOURCE, NULL) ||
> - bdrv_op_is_blocked(bs->backing->bs, BLOCK_OP_TYPE_COMMIT_TARGET,
> NULL)) {
> + bdrv_op_is_blocked(backing_file_bs, BLOCK_OP_TYPE_COMMIT_TARGET,
> NULL))
> + {
> return -EBUSY;
> }
>
> - ro = bs->backing->bs->read_only;
> + ro = backing_file_bs->read_only;
>
> if (ro) {
> - if (bdrv_reopen_set_read_only(bs->backing->bs, false, NULL)) {
> + if (bdrv_reopen_set_read_only(backing_file_bs, false, NULL)) {
> return -EACCES;
> }
> }
> @@ -440,8 +488,6 @@ int bdrv_commit(BlockDriverState *bs)
> }
>
> /* Insert commit_top block node above backing, so we can write to it */
> - backing_file_bs = backing_bs(bs);
> -
> commit_top_bs = bdrv_new_open_driver(&bdrv_commit_top, NULL,
> BDRV_O_RDWR,
> &local_err);
> if (commit_top_bs == NULL) {
> @@ -526,15 +572,13 @@ ro_cleanup:
> qemu_vfree(buf);
>
> blk_unref(backing);
> - if (backing_file_bs) {
> - bdrv_set_backing_hd(bs, backing_file_bs, &error_abort);
> - }
> + bdrv_set_backing_hd(bs, backing_file_bs, &error_abort);
Preexisting, but we should not drop filter if we didn't added it (if we failed
above filter
insertion). You increased amount of such cases. No damage still.
> bdrv_unref(commit_top_bs);
> blk_unref(src);
>
> if (ro) {
> /* ignoring error return here */
> - bdrv_reopen_set_read_only(bs->backing->bs, true, NULL);
> + bdrv_reopen_set_read_only(backing_file_bs, true, NULL);
> }
>
> return ret;
> diff --git a/blockdev.c b/blockdev.c
> index c6f79b4e0e..7bef41c0b0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1094,7 +1094,7 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
> return;
> }
>
> - bs = blk_bs(blk);
> + bs = bdrv_skip_implicit_filters(blk_bs(blk));
> aio_context = bdrv_get_aio_context(bs);
> aio_context_acquire(aio_context);
>
> @@ -3454,7 +3454,9 @@ void qmp_block_commit(bool has_job_id, const char
> *job_id, const char *device,
>
> assert(bdrv_get_aio_context(base_bs) == aio_context);
>
> - for (iter = top_bs; iter != backing_bs(base_bs); iter =
> backing_bs(iter)) {
> + for (iter = top_bs; iter != bdrv_filtered_bs(base_bs);
> + iter = bdrv_filtered_bs(iter))
> + {
> if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_COMMIT_TARGET, errp)) {
> goto out;
> }
>
--
Best regards,
Vladimir
