| Message ID | 20201127144522.29991-27-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: update graph permissions update | expand |
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
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).
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. Kevin
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..
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
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); - if (local_err) { - error_prepend(&local_err, - "Cannot set permissions for backup-top filter: "); - goto fail; - } - state->cluster_size = cluster_size; state->bcs = block_copy_state_new(top->backing, state->target, cluster_size, write_flags, &local_err); @@ -256,7 +228,6 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, fail: if (appended) { - state->active = false; bdrv_backup_top_drop(top); } else { bdrv_unref(top); @@ -272,16 +243,9 @@ void bdrv_backup_top_drop(BlockDriverState *bs) { BDRVBackupTopState *s = bs->opaque; - bdrv_drained_begin(bs); + bdrv_drop_filter(bs, &error_abort); block_copy_state_free(s->bcs); - s->active = false; - bdrv_child_refresh_perms(bs, bs->backing, &error_abort); - bdrv_replace_node(bs, bs->backing->bs, &error_abort); - bdrv_set_backing_hd(bs, NULL, &error_abort); - - bdrv_drained_end(bs); - bdrv_unref(bs); } diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out index fbb7d0f619..a34e4e3f92 100644 --- a/tests/qemu-iotests/283.out +++ b/tests/qemu-iotests/283.out @@ -5,4 +5,4 @@ {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}} {"return": {}} {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}} -{"error": {"class": "GenericError", "desc": "Cannot set permissions for backup-top filter: Conflicts with use by source as 'image', which does not allow 'write' on base"}} +{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Conflicts with use by source as 'image', which does not allow 'write' on base"}}
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(-)