[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 5/5] replication: Remove workaround
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [PATCH v5 5/5] replication: Remove workaround |
Date: |
Thu, 15 Jul 2021 16:48:06 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 |
12.07.2021 14:54, Lukas Straub wrote:
Remove the workaround introduced in commit
6ecbc6c52672db5c13805735ca02784879ce8285
"replication: Avoid blk_make_empty() on read-only child".
It is not needed anymore since s->hidden_disk is guaranteed to be
writable when secondary_do_checkpoint() runs. Because replication_start(),
_do_checkpoint() and _stop() are only called by COLO migration code
and COLO-migration activates all disks via bdrv_invalidate_cache_all()
before it calls these functions.
In replication_child_perm()
we have
if ((bs->open_flags & (BDRV_O_INACTIVE | BDRV_O_RDWR)) == BDRV_O_RDWR) {
*nperm |= BLK_PERM_WRITE;
}
That's probably possible
1. configure a block graph like described in replicatio doc, but all disks
opened read-only. so we don't have WRITE permission
2. start replication
3. crash on trying to make disk empty in do_checkpoint with no WRITE permission
Still, I think if it possible, we'll crash on first bdrv_make_empty of active
disk, and that's preexisting.
So:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
block/replication.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/block/replication.c b/block/replication.c
index 772bb63374..1e9dc4d309 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -356,17 +356,7 @@ static void secondary_do_checkpoint(BlockDriverState *bs,
Error **errp)
return;
}
- BlockBackend *blk = blk_new(qemu_get_current_aio_context(),
- BLK_PERM_WRITE, BLK_PERM_ALL);
- blk_insert_bs(blk, s->hidden_disk->bs, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- blk_unref(blk);
- return;
- }
-
- ret = blk_make_empty(blk, errp);
- blk_unref(blk);
+ ret = bdrv_make_empty(s->hidden_disk, errp);
if (ret < 0) {
return;
}
--
2.20.1
--
Best regards,
Vladimir
- [PATCH v5 0/5] replication: Bugfix and properly attach children, Lukas Straub, 2021/07/12
- [PATCH v5 2/5] replication: Reduce usage of s->hidden_disk and s->secondary_disk, Lukas Straub, 2021/07/12
- [PATCH v5 1/5] replication: Remove s->active_disk, Lukas Straub, 2021/07/12
- [PATCH v5 3/5] replication: Properly attach children, Lukas Straub, 2021/07/12
- [PATCH v5 4/5] replication: Assert that children are writable, Lukas Straub, 2021/07/12
- [PATCH v5 5/5] replication: Remove workaround, Lukas Straub, 2021/07/12
- Re: [PATCH v5 5/5] replication: Remove workaround,
Vladimir Sementsov-Ogievskiy <=