Re: [PATCH 10/21] block/copy-before-write: relax permission requirements when no parents

[Vladimir Sementsov-Ogievskiy]

18.05.2021 14:10, Max Reitz wrote:
> On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
> > We are going to publish copy-before-write filter. So, user should be
> > able to create it with blockdev-add first, specifying both filtered and
> > target children. And then do blockdev-reopen, to actually insert the
> > filter where needed.
> >
> > Currently, filter unshares write permission unconditionally on source
> > node. It's good, but it will not allow to do blockdev-add. So, let's
> > relax restrictions when filter doesn't have any parent.
> >
> > Test output is modified, as now permission conflict happens only when
> > job creates a blk parent for filter node.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   block/copy-before-write.c  | 8 +++++---
> >   tests/qemu-iotests/283.out | 2 +-
> >   2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> > index 4776172f77..af2bb97a30 100644
> > --- a/block/copy-before-write.c
> > +++ b/block/copy-before-write.c
> > @@ -142,10 +142,12 @@ static void cbw_child_perm(BlockDriverState *bs, 
> > BdrvChild *c,
> >           bdrv_default_perms(bs, c, role, reopen_queue,
> >                              perm, shared, nperm, nshared);
> > -        if (perm & BLK_PERM_WRITE) {
> > -            *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
> > +        if (!QLIST_EMPTY(&bs->parents)) {
>
> I understand this works because with the transactional system, at the time the 
> permissions are checked, the graph has already been changed, yes?

Right

> I was wondering still whether there was any way to express this through 
> permissions alone.  I guess we could check
> “perm & BLK_PERM_CONSISTENT_READ”, but that would actually be just a crutch to 
> see whether there are any parents.  I suppose this really is about whether there 
> are parents or not.  So:

It's a workaround for blockdev-add to work. It's a workaround, because it would 
be better for the filter to never share WRITE. But this way we'll have to make 
an interface to transactionally create and insert filter. Probably we will at 
some point, than this workaround could be dropped. So, if we have to 
special-case a situation when there no parents, let's check exactly this, to be 
more strict.

> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> > +            if (perm & BLK_PERM_WRITE) {
> > +                *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
> > +            }
> > +            *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> >           }
> > -        *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> >       }
> >   }
> > diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
> > index e08f807dab..d5350ce7a7 100644
> > --- a/tests/qemu-iotests/283.out
> > +++ b/tests/qemu-iotests/283.out
> > @@ -5,7 +5,7 @@
> >   {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": 
> > "other", "take-child-perms": ["write"]}}
> >   {"return": {}}
> >   {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", 
> > "target": "target"}}
> > -{"error": {"class": "GenericError", "desc": "Cannot append copy-before-write 
> > filter: Conflicts with use by source as 'image', which does not allow 'write' on base"}}
> > +{"error": {"class": "GenericError", "desc": "Conflicts with use by source as 
> > 'image', which does not allow 'write' on base"}}
> >   === copy-before-write filter should be gone after job-finalize ===


--
Best regards,
Vladimir