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 <[email protected]>
> > @@ -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
pgpcf7yk85BwX.pgp
Description: PGP signature
