Am 27.04.26 um 7:43 PM schrieb Kevin Wolf:
> 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]>
> ---
> block/commit.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> 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();
I suppose we could instead drain only the affected BDSs? Blocking all
for the whole duration of the blk_pread+blk_pwrite loop seems a bit much.
Independently of that, I wonder if blk_commit_all() should drain all?
I'm not familiar with it, but it seems that the intent is to have a
point-in-time state which is consistent between different BDSs? That
intent could be made explicit by draining all.
> 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;
> }
