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

[Vladimir Sementsov-Ogievskiy]

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).


--
Best regards,
Vladimir