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