qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Kevin Wolf
Subject: Re: [PATCH v2 26/36] block/backup-top: drop .active
Date: Thu, 4 Feb 2021 13:26:51 +0100

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

Kevin




reply via email to

[Prev in Thread] Current Thread [Next in Thread]