| Message ID | 20190809161407.11920-28-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Deal with filters | expand |
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 <mreitz@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 <vsementsov@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; > } >
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 <mreitz@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. Sounds good, yes. > And with at least this fixed I'm OK with this patch: > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@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 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
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(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); + + 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); 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; }
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 <mreitz@redhat.com> --- block/block-backend.c | 16 +++++--- block/commit.c | 96 +++++++++++++++++++++++++++++++------------ blockdev.c | 6 ++- 3 files changed, 85 insertions(+), 33 deletions(-)