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 <vsement...@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. Kevin