Re: [PATCH v2 26/36] block/backup-top: drop .active

From: Kevin Wolf <kwolf@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: qemu-block@nongnu.org, armbru@redhat.com, qemu-devel@nongnu.org,
	mreitz@redhat.com, den@openvz.org, jsnow@redhat.com
Subject: Re: [PATCH v2 26/36] block/backup-top: drop .active
Date: Thu, 4 Feb 2021 15:31:57 +0100	[thread overview]
Message-ID: <20210204143157.GH6496@merkur.fritz.box> (raw)
In-Reply-To: <cb558baf-2ee5-aa46-ef5e-228140f3bd44@virtuozzo.com>

Am 04.02.2021 um 14:46 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 04.02.2021 16:25, Kevin Wolf wrote:
> > Am 04.02.2021 um 13:33 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 04.02.2021 15:26, Kevin Wolf wrote:
> > > > Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > > We don't need this workaround anymore: bdrv_append is already smart
> > > > > enough and we can use new bdrv_drop_filter().
> > > > > 
> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > > > ---
> > > > >    block/backup-top.c         | 38 +-------------------------------------
> > > > >    tests/qemu-iotests/283.out |  2 +-
> > > > >    2 files changed, 2 insertions(+), 38 deletions(-)
> > > > > 
> > > > > diff --git a/block/backup-top.c b/block/backup-top.c
> > > > > index 650ed6195c..84eb73aeb7 100644
> > > > > --- a/block/backup-top.c
> > > > > +++ b/block/backup-top.c
> > > > > @@ -37,7 +37,6 @@
> > > > >    typedef struct BDRVBackupTopState {
> > > > >        BlockCopyState *bcs;
> > > > >        BdrvChild *target;
> > > > > -    bool active;
> > > > >        int64_t cluster_size;
> > > > >    } BDRVBackupTopState;
> > > > > @@ -127,21 +126,6 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
> > > > >                                      uint64_t perm, uint64_t shared,
> > > > >                                      uint64_t *nperm, uint64_t *nshared)
> > > > >    {
> > > > > -    BDRVBackupTopState *s = bs->opaque;
> > > > > -
> > > > > -    if (!s->active) {
> > > > > -        /*
> > > > > -         * The filter node may be in process of bdrv_append(), which firstly do
> > > > > -         * bdrv_set_backing_hd() and then bdrv_replace_node(). This means that
> > > > > -         * we can't unshare BLK_PERM_WRITE during bdrv_append() operation. So,
> > > > > -         * let's require nothing during bdrv_append() and refresh permissions
> > > > > -         * after it (see bdrv_backup_top_append()).
> > > > > -         */
> > > > > -        *nperm = 0;
> > > > > -        *nshared = BLK_PERM_ALL;
> > > > > -        return;
> > > > > -    }
> > > > > -
> > > > >        if (!(role & BDRV_CHILD_FILTERED)) {
> > > > >            /*
> > > > >             * Target child
> > > > > @@ -229,18 +213,6 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
> > > > >        }
> > > > >        appended = true;
> > > > > -    /*
> > > > > -     * bdrv_append() finished successfully, now we can require permissions
> > > > > -     * we want.
> > > > > -     */
> > > > > -    state->active = true;
> > > > > -    bdrv_child_refresh_perms(top, top->backing, &local_err);
> > > > 
> > > > bdrv_append() uses bdrv_refresh_perms() for the whole node. Is it doing
> > > > unnecessary extra work there and should really do the same as backup-top
> > > > did here, i.e. bdrv_child_refresh_perms(bs_new->backing)?
> > > > 
> > > > (Really a comment for an earlier patch. This patch itself looks fine.)
> > > > 
> > > 
> > > You mean how backup-top code works at the point when we modified
> > > bdrv_append()? Actually all works, as we use state->active. We set it
> > > to true and should call refresh_perms. Now we drop _refresh_perms
> > > _together_ with state->active variable, so filter is always "active",
> > > but new bdrv_append can handle it now. I.e., before this patch
> > > backup-top.c code is correct but over-complicated with logic which is
> > > not necessary after bdrv_append() improvement (and of-course we need
> > > also bdrv_drop_filter() to drop the whole state->active related
> > > logic).
> > 
> > No, I just mean that bdrv_child_refresh_perms(bs, bs->backing) is enough
> > when adding a new image to the chain. A full bdrv_child_refresh_perms()
> > like we now have in bdrv_append() is doing more work than is necessary.
> > 
> > It doesn't make a difference for backup-top (because the filter has only
> > a single child), but if you append a new qcow2 snapshot, you would also
> > recalculate permissions for the bs->file subtree even though nothing has
> > changed there.
> > 
> > It's only a small detail anyway, not very important in a slow path.
> 
> Understand now. I think bdrv_append() do correct things: bs_new gets
> new parents, so we refresh the whole subtree.. So for appending qcow2
> we should refresh its file child as well. Probably new permissions of
> new bs_new parents will influence what qcow2 wants to do with it file
> node..

You mean the parents that move from bs_top to bs_new and that they could
change the permissions that bs_new needs?

Good point, yes.

Kevin