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..
--
Best regards,
Vladimir
