Re: [Qemu-devel] [PATCH 36/54] commit: Use real permissions in commit block job

[Kevin Wolf <kwolf@redhat.com>]

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

Am 24.02.2017 um 18:29 hat Max Reitz geschrieben:
> On 21.02.2017 15:58, Kevin Wolf wrote:
> > This is probably one of the most interesting conversions to the new
> > op blocker system because a commit block job intentionally leaves some
> > intermediate block nodes in the backing chain that aren't valid on their
> > own any more; only the whole chain together results in a valid view.
> > 
> > In order to provide the 'consistent read' permission to the parents of
> > the 'top' node of the commit job, a new filter block driver is inserted
> > above 'top' which doesn't require 'consistent read' on its backing
> > chain. Subsequently, the commit job can block 'consistent read' on all
> > intermediate nodes without causing a conflict.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>

> > @@ -262,34 +305,62 @@ void commit_start(const char *job_id, BlockDriverState *bs,
> >          }
> >      }
> >  
> > +    /* Insert commit_top block node above top, so we can block consistent read
> > +     * on the backing chain below it */
> > +    commit_top_bs = bdrv_new_open_driver(&bdrv_commit_top, NULL, BDRV_O_RDWR,
> 
> Why RDWR when the driver only allows reads anyway?

Good question. I'll try to change it, maybe it doesn't break everything.

> > +                                         errp);
> > +    if (commit_top_bs == NULL) {
> > +        goto fail;
> > +    }
> > +
> > +    bdrv_set_backing_hd(commit_top_bs, top);
> > +    bdrv_set_backing_hd(overlay_bs, commit_top_bs);
> > +
> > +    s->commit_top_bs = commit_top_bs;
> > +    bdrv_unref(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 != backing_bs(base); iter = backing_bs(iter)) {
> > -        /* FIXME Use real permissions */
> > -        block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
> > -                           BLK_PERM_ALL, &error_abort);
> > +    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.
> 
> As far as I can see, the loop doesn't even touch base, though...?

If bs isn't writable, bs->backing generally isn't writable either, and
we are touching a parent of base.

> >                         The other options would be a second filter driver above
> > +         * s->base. */
> > +        ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
> 
> Don't we need CONSISTENT_READ at least for top?

top can't provide CONSISTENT_READ because its backing files can't
provide it. It's the job (one of the jobs) of commit_top_bs to shield
the parents of top from the loss of CONSISTENT_READ.

> > +                                 BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE,
> > +                                 errp);
> > +        if (ret < 0) {
> > +            goto fail;
> > +        }
> >      }
> > +
> > +    ret = block_job_add_bdrv(&s->common, "base", base, 0, BLK_PERM_ALL, errp);
> > +    if (ret < 0) {
> > +        goto fail;
> > +    }
> > +
> >      /* overlay_bs must be blocked because it needs to be modified to
> > -     * update the backing image string, but if it's the root node then
> > -     * don't block it again */
> > -    if (bs != overlay_bs) {
> > -        /* FIXME Use real permissions */
> > -        block_job_add_bdrv(&s->common, "overlay of top", overlay_bs, 0,
> > -                           BLK_PERM_ALL, &error_abort);
> > +     * update the backing image string. */
> > +    ret = block_job_add_bdrv(&s->common, "overlay of top", overlay_bs,
> > +                             BLK_PERM_GRAPH_MOD, BLK_PERM_ALL, errp);
> > +    if (ret < 0) {
> > +        goto fail;
> >      }
> >  
> > -    /* FIXME Use real permissions */
> > -    s->base = blk_new(0, BLK_PERM_ALL);
> > +    s->base = blk_new(BLK_PERM_CONSISTENT_READ
> 
> Do we actually need CONSISTENT_READ for the base?

If base doesn't provide CONSISTENT_READ, commit_top_bs wouldn't be able
to provide it either.

If we ever find a case where this is too restrictive because the parents
of commit_top_bs don't need CONSISTENT_READ, we can probably be less
strict in this case, but just getting commit to work is already tricky
enough that I wouldn't like to do it in this patch (or even series).

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]