On 19.05.2026 20:03, Kevin Wolf wrote:
The whole implementation of bdrv_commit() is only correct if no new
writes come in while it's running: It has only a single loop checking
the allocation status for each block and finally calls bdrv_make_empty()
without checking if that throws away any new changes.
We already have to drain while taking the graph write lock. Just extend
the drained section to all of bdrv_commit() to make sure that we don't
get any inconsistencies.
Signed-off-by: Kevin Wolf <[email protected]>
Message-ID: <[email protected]>
Reviewed-by: Denis V. Lunev <[email protected]>
Tested-by: Denis V. Lunev <[email protected]>
Signed-off-by: Kevin Wolf <[email protected]>
---
block/commit.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
Hi!
Does this change makes sense for 10.0.x qemu stable series too?
It feels like this change should be picked up for 10.0.x (LTS)
series too.
But I need some help with that. 10.0.x has quite different code
in this area..
Can you back-port this change to 10.0.x, or should this one be dropped
from this series?
Thank you!
/mjt
diff --git a/block/commit.c b/block/commit.c
index 0d9e1a16d7a..c5e3ef03a21 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -518,6 +518,7 @@ int bdrv_commit(BlockDriverState *bs)
if (!drv)
return -ENOMEDIUM;
+ bdrv_drain_all_begin();
bdrv_graph_rdlock_main_loop();
backing_file_bs = bdrv_cow_bs(bs);
@@ -549,6 +550,10 @@ int bdrv_commit(BlockDriverState *bs)
BLK_PERM_ALL);
backing = blk_new(ctx, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
+ /* We drained all nodes, but still make requests through BlockBackends */
+ blk_set_disable_request_queuing(src, true);
+ blk_set_disable_request_queuing(backing, true);
+
ret = blk_insert_bs(src, bs, &local_err);
if (ret < 0) {
error_report_err(local_err);
@@ -565,7 +570,7 @@ int bdrv_commit(BlockDriverState *bs)
bdrv_graph_rdunlock_main_loop();
- bdrv_graph_wrlock_drained();
+ bdrv_graph_wrlock();
bdrv_set_backing_hd(commit_top_bs, backing_file_bs, &error_abort);
bdrv_set_backing_hd(bs, commit_top_bs, &error_abort);
bdrv_graph_wrunlock();
@@ -647,7 +652,7 @@ ro_cleanup:
blk_unref(backing);
bdrv_graph_rdunlock_main_loop();
- bdrv_graph_wrlock_drained();
+ bdrv_graph_wrlock();
if (bdrv_cow_bs(bs) != backing_file_bs) {
bdrv_set_backing_hd(bs, backing_file_bs, &error_abort);
}
@@ -663,6 +668,7 @@ ro_cleanup:
out:
bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_end();
return ret;
}
