|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [PATCH v2 26/36] block/backup-top: drop .active |
Date: | Thu, 4 Feb 2021 16:46:43 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 |
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.. -- Best regards, Vladimir
[Prev in Thread] | Current Thread | [Next in Thread] |