Re: [PATCH v4 3/5] block: let bdrv_reopen_commit_post() report a failure

[Kevin Wolf <kwolf@redhat.com>]

Am 24.08.2026 um 15:37 hat Denis V. Lunev geschrieben:
> From: Denis V. Lunev <den@openvz.org>
> 
> The callback runs after bdrv_reopen_multiple() has committed the
> transaction, so it cannot reject the reopen. It can still find that
> the node it has just made writable is unusable, and has no way to say
> so: bdrv_reopen() returns success and the caller carries on.
> 
> Give it a return value and an Error argument. The reopen stays
> committed, the error only reports that the node is gone. Every queued
> node still gets its callback, the first error is the one reported. A
> callback may leave its node without a driver, and so may the I/O of a
> later bdrv_reopen_prepare(), so do not assume that the nodes still
> ahead of it in the queue have one. qcow2 is the only implementation and
> does not fail yet.
> 
> An error therefore means one of two things now, either that the reopen
> was denied and nothing changed, or that it went through and left a tree
> which cannot be used. Nothing is undone in the second case: the node is
> beyond repair by another reopen, and a caller which reacts to the error
> by reopening anything is making it worse. bdrv_reopen_multiple() says
> so, nothing else changes.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> Cc: qemu-stable@nongnu.org

What you write both in the commit messages of this and the following
patches and in this thread makes me think that you understand well that
this approach is fundamentally wrong if we're taking the transaction
model seriously, so I don't have to explain that.

If we were discussing which transaction phase should contain image
repairing completely out of the context of QEMU code, I think we would
be very quick to agree that it's the prepare phase.

The problem is just that it shows once again the old problem that the
reopen design is flawed and doesn't allow writing to the image in the
prepare phase for a ro -> rw transition. Even the already existing
qcow2_reopen_commit_post() is just a hack; in theory, the bitmaps should
be made writable in prepare and then commit should just be a switchover.
Reverting to read-only in abort is an operation that can tolerate
failure, it just gives up access.

I've been thinking for a long time that during reopen, parent nodes
should have access to both the old state and the new state of their
children to allow all necessary modifications. This feels like a very
big change, though.

I'm wondering if for ro/rw specifically (which is actually the one
setting that is always involved in these nasty cases), it might be
enough if we define the expected state after prepare to be that the
image is writable if either the old or the new state is writable. Then
commit and abort always just either leave it writable or drop write
permissions.

Do you think this would be a viable approach for your problem here or
are there additional problems why the image checking can't be done
during prepare?

Kevin