Re: [PATCH v7 35/47] commit: Deal with filters

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 12640 bytes --]

Am 20.08.2020 um 13:27 hat Max Reitz geschrieben:
> On 19.08.20 19:58, Kevin Wolf wrote:
> > Am 25.06.2020 um 17:22 hat Max Reitz geschrieben:
> >> 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          |  9 +++-
> >>  block/commit.c                 | 96 +++++++++++++++++++++++++---------
> >>  block/monitor/block-hmp-cmds.c |  2 +-
> >>  blockdev.c                     |  4 +-
> >>  4 files changed, 81 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/block/block-backend.c b/block/block-backend.c
> >> index 6936b25c83..7f2c7dbccc 100644
> >> --- a/block/block-backend.c
> >> +++ b/block/block-backend.c
> >> @@ -2271,8 +2271,13 @@ 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);
> > 
> > The old code didn't try to commit nodes that don't have a backing file.
> > 
> >> +        if (blk_is_inserted(blk)) {
> >> +            BlockDriverState *non_filter;
> >> +            int ret;
> >> +
> >> +            /* Legacy function, so skip implicit filters */
> >> +            non_filter = bdrv_skip_implicit_filters(blk->root->bs);
> >> +            ret = bdrv_commit(non_filter);
> > 
> > The new one tries unconditionally. For nodes without a backing file,
> > bdrv_commit() will return -ENOTSUP, so the whole function fails.
> 
> :(
> 
> Hm.  Should I fix it by checking for
> bdrv_cow_bs(bdrv_skip_implicit_filters())?  Or bdrv_backing_chain_next()
> and change the bdrv_skip_implicit_filters() to a bdrv_skip_filters()?  I
> feel like that would make even more sense.

I agree that bdrv_skip_filters() makes more sense. If I have a qcow2
image and an explicit throttle filter on top, there is no reason to skip
this image.

bdrv_backing_chain_next() or bdrv_cow_bs() should be the same in a
boolean context, so I'd vote for bdrv_cow_bs() because it has less work
to do to get the same result.

> > (First real bug at patch 35. I almost thought I wouldn't find any!)
> 
> :)
> 
> >>              if (ret < 0) {
> >>                  aio_context_release(aio_context);
> >>                  return ret;
> >> diff --git a/block/commit.c b/block/commit.c
> >> index 7732d02dfe..4122b6736d 100644
> >> --- a/block/commit.c
> >> +++ b/block/commit.c
> >> @@ -37,6 +37,7 @@ typedef struct CommitBlockJob {
> >>      BlockBackend *top;
> >>      BlockBackend *base;
> >>      BlockDriverState *base_bs;
> >> +    BlockDriverState *base_overlay;
> >>      BlockdevOnError on_error;
> >>      bool base_read_only;
> >>      bool chain_frozen;
> > 
> > Hm, again this mysterious base_overlay. I know that stream introduced it
> > to avoid freezing the link to base, but commit doesn't seem to do that.
> > 
> > Is it to avoid using the block status of filter drivers between
> > base_overlay and base?
> 
> Yes.
> 
> > If so, I guess that goes back to the question I
> > raised earlier in this series: What is the block status supposed to tell
> > for filter nodes?
> 
> Honestly, I would really like to get away without having to answer that
> question in this series.  Intuitively, I feel like falling through to
> the next data-bearing layer is not something most callers want.  But I’d
> rather investigate that question separately from this series (even
> though that likely means we’ll never do it), and just treat it as it is
> in this series.

Well, I'm asking the question because not having the answer makes us
jump through hoops in this series to accomodate a behaviour it probably
shouldn't even have. (Because I agree that filters should probably keep
DATA clear, i.e. they are never the layer that defines the content.)

Additional node references (i.e. references that are not edges in the
graph) always make the design more complicated and require us to
consider more things like what happens on graph changes. So it's a
question of maintainability.

> > But anyway, in contrast to mirror, commit actually freezes the chain
> > between commit_top_bs and base, so it should be safe at least.
> > 
> >> @@ -89,7 +90,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);
> >> @@ -153,7 +154,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->base_overlay, true,
> >>                                        offset, COMMIT_BUFFER_SIZE, &n);
> >>          copy = (ret == 1);
> >>          trace_commit_one_iteration(s, offset, n, ret);
> >> @@ -253,15 +254,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_filters(top) == bdrv_skip_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;
> >> +    }
> > 
> > base_perms would indicate which permissions these are (particularly
> > because it's not the next thing that requires permissions, but only used
> > further down the function).
> 
> %s/\<perms\>/base_perms/?  Sure.

Sorry, I admit this wasn't phrased very clearly. But yes, renaming the
variable this way is what I meant.

> >>      s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, BLK_PERM_ALL,
> >>                           speed, creation_flags, NULL, NULL, errp);
> >>      if (!s) {
> >> @@ -301,17 +322,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->base_overlay = bdrv_find_overlay(top, base);
> >> +    assert(s->base_overlay);
> >> +
> >> +    /*
> >> +     * The topmost node with
> >> +     * bdrv_skip_filters(filtered_base) == bdrv_skip_filters(base)
> >> +     */
> >> +    filtered_base = bdrv_cow_bs(s->base_overlay);
> >> +    assert(bdrv_skip_filters(filtered_base) == bdrv_skip_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_filter_or_cow_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;
> >>          }
> >> @@ -328,9 +375,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);
> >> @@ -398,19 +443,22 @@ int bdrv_commit(BlockDriverState *bs)
> >>      if (!drv)
> >>          return -ENOMEDIUM;
> >>  
> >> -    if (!bs->backing) {
> >> +    backing_file_bs = bdrv_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;
> >>          }
> >>      }
> >> @@ -428,8 +476,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) {
> >> @@ -515,15 +561,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);
> > 
> > This means that bdrv_set_backing_hd() is now called to undo a change
> > that hasn't even been made yet. This fails (with &error_abort) if the
> > backing chain is frozen.
> > 
> > On the other hand, the other bdrv_set_backing_hd() calls in the same
> > function would fail the same way.
> 
> True. :)
> 
> Still, maybe there’s an op blocker from a concurrent job, so we go to
> the failure path and then we’d abort here.  So better to guard it by
> checking whether bdrv_cow_bs(bs) != backing_file_bs.

Certainly can't hurt.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]