On 8/26/26 17:24, Markus Armbruster wrote: > "Denis V. Lunev" <[email protected]> writes: > >> On 8/25/26 11:37, Markus Armbruster wrote: >>> "Denis V. Lunev" <[email protected]> writes: >>> >>>> From: Denis V. Lunev <[email protected]> >>>> >>>> A dirty image must be repaired before anything allocates a cluster in >>>> it. qcow2_do_open() does that, but only for a node that is writable >>>> from the start. A node opened read-only skips it, and nothing revisits >>>> the question once that node becomes writable, which block-commit does >>>> routinely: commit_active_start() and commit_start() reopen the base >>>> read-write for the duration of the job. >>>> >>>> With lazy refcounts the on-disk refcount block then still accounts for >>>> the metadata clusters only, so the allocator restarts at the front of >>>> the image and hands out clusters that L2 entries point at. Two guest >>>> offsets end up sharing one host cluster. Nothing fails, the corrupt bit >>>> stays clear, and a clean close clears the dirty bit, so no later open >>>> repairs the image either. The bit also stays set for the whole writable >>>> session, so a node which is merely writable says nothing. >>>> >>>> Refusing the reopen instead is simpler and keeps it atomic, but it >>>> leaves nowhere to go: the base belongs to a chain the VM has open, so >>>> the qemu-img check -r such an error would ask for cannot take the write >>>> lock it needs. The repair does the trick in most cases anyway. >>>> >>>> Do the repair in qcow2_reopen_commit_post(), the earliest point where >>>> the node is writable. An inactive node is skipped: bdrv_activate() calls >>>> qcow2_do_open() again through qcow2_co_invalidate_cache(). >>>> >>>> commit_post cannot reject the reopen, so a failed repair takes the >>>> driver away from the node instead, which is what stops writes from >>>> aliasing live clusters. qcow2_signal_corruption() does that as well, >>>> but it also sends BLOCK_IMAGE_CORRUPTED and sets the corrupt bit, >>>> which qcow2_do_open() honours by refusing every later read-write open. >>>> The image is dirty and unrepaired, not corrupt, and qemu-img check -r >>>> still fixes it, so neither belongs here. Return the error and skip >>>> the bitmaps. >>>> >>>> Signed-off-by: Denis V. Lunev <[email protected]> >>>> Reviewed-by: Andrey Drobyshev <[email protected]> >>>> CC: Kevin Wolf <[email protected]> >>>> CC: Hanna Reitz <[email protected]> >>>> CC: Eric Blake <[email protected]> >>>> CC: Markus Armbruster <[email protected]> >>>> CC: Andrey Drobyshev <[email protected]> >>>> Cc: [email protected] >>> [...] >>> >>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>> index 199efc1e00..940249a5e5 100644 >>>> --- a/qapi/block-core.json >>>> +++ b/qapi/block-core.json >>>> @@ -1852,6 +1852,9 @@ >>> ## >>> # @change-backing-file: >>> # >>> # Change the backing file in the image file metadata. This does not >>> # cause QEMU to reopen the image file to reparse the backing filename >>> # (it may, however, perform a reopen to change permissions from r/o -> >>>> # r/w -> r/o, if needed). The new backing file string is written into >>>> # the image file metadata, and the QEMU internal strings are updated. >>>> # >>>> +# A dirty qcow2 image is repaired during that reopen, which blocks >>>> +# other requests and can fail the command. >>> Pardon my ignorance: what makes a qcow2 image dirty? >> usual obvious reasons are SIGKILL to qemu process (f.e. from OOM) >> or node crash. > So, you have to do some cleaning work before you can use it again, just > like a dirty filesystem. Correct? Correct. QEMU allows right now to open dirty images in read-only mode and it is OK to be used until we switch to RW. In this case real write to metadata corrupts image.
> Back to change-backing-file. It operates on an open image. Cleaning > happens when that image is read-only and dirty. Possible because you > can open dirty images read-only, and that doesn't clean them. Correct? Correct. >>> Can you give me an idea of what other requests could be blocked? >> Before the patch reopen was smooth - we have just opened the >> file again. After this patch in a very unlikely corner case >> potentially lengthy procedure has been added - full image >> consistency check. Guest IO is stuck until the check will be >> completed. >> >> The case is unfortunately real for production. >> >>> Double-checking: "that reopen" is the one to change permissions, >>> i.e. the parenthesis above. Correct? >> yes >> >>>> +# >>>> # @image-node-name: The name of the block driver state node of the >>>> # image to modify. The "device" argument is used to verify >>>> # "image-node-name" is in the chain described by "device". >>>> @@ -1891,6 +1894,9 @@ >>> ## >>> # @block-commit: >>> # >>> # Live commit of data from overlay image nodes into backing nodes - >>> # i.e., writes data between 'top' and 'base' into 'base'. >>> # >>> # If top == base, that is an error. If top has no overlays on top of >>> # it, or if it is in use by a writer, the job will not be completed by >>> # itself. The user needs to complete the job with the `job-complete` >>> # command after getting the ready event. (Since 2.0) >>> # >>> # If the base image is smaller than top, then the base image will be >>> # resized to be the same size as top. If top is smaller than the base >>> # image, the base will not be truncated. If you want the base image >>>> # size to match the size of the smaller top, you can safely truncate >>>> # it yourself once the commit operation successfully completes. >>>> # >>>> +# The base is opened read-write. A dirty qcow2 base is repaired > Reopend, I presume? The meaning here is repaired. The meaning here is the following: "The base is reopened read-write and if it is dirty it should be repaired before any single write is made. Guest stalls until repair is complete." >>>> +# first, which blocks other requests and can fail the command. >>>> +# >>>> # @job-id: identifier for the newly-created block job. If omitted, >>>> # the device name will be used. (Since 2.7) >>>> # > Like change-backing-file, block-commit operates on open images. It > copies down into a base image. If the base image is read-only, it is > reopened, and cleaning happens when it's dirty. Correct? Correct. > Can it happen in any other way? No at the best knowledge from me and Andrey. >>>> @@ -2902,6 +2908,9 @@ >>> ## >>> # @block-stream: >>> # >>> # Copy data from a backing file into a block device. >>> >>> [...] > Whereas block-commit copies down into a base image, block-stream copies > up from a base image. If the image copied to is read-only, it is > reopened, and cleaning happens when it's dirty. Correct? Correct. > Can it happen in any other way? No at the best knowledge from me and Andrey. >>>> # On successful completion the image file is updated to drop the >>>> # backing file and the `BLOCK_JOB_COMPLETED` event is emitted. >>>> # >>>> +# The top image is opened read-write. A dirty qcow2 image is repaired >>>> +# first, which blocks other requests and can fail the command. >>>> +# >>>> # In case @device is a filter node, `block-stream` modifies the first >>>> # non-filter overlay node below it to point to the new backing node >>>> # instead of modifying @device itself. >>>> @@ -4989,6 +4998,16 @@ >>> ## >>> # @blockdev-reopen: >>> # >>> # Reopens one or more block devices using the given set of options. >>> # Any option not specified will be reset to its default value >>> # regardless of its previous status. If an option cannot be changed >>> # or a particular driver does not support reopening then the command >>> # will return an error. All devices in the list are reopened in one >>>> # transaction, so if one of them fails then the whole transaction is >>>> # cancelled. > blockdev-reopen also operates on open images. Cleaning happens when > reopening a dirty read-only image read/write. Correct? Correct. > Can it happen in any other way? No. >>>> # >>>> +# An error is also returned when a device cannot be used once it has >>>> +# been reopened. Such a reopen is not undone, so an error does not >>>> +# always mean that nothing has changed. >>> Could that be a problem? >> Error reply as itself is not a problem. The situation as a whole >> is a real pain in the ass. >> >> The problem is that such images exists in production and there >> is not way to handle this without downtime. >> >> Under this patch we are trying to fix things hard and this is >> correct thing to do - on RW image we are doing exactly the >> same thing. If automation is unable to fix - the guest denies >> starting. >> >> Here we report an error and render guest as unusable. This >> case is expected to be extremely rare but technically possible. >> >> This is a problem. > I'll come back to this as soon as I understand when exactly cleaning may > happen. Right. Thanks. You are asking very good questions which are quite important and interesting :-) Simple "deny" policy is very bad from operations point of view. >>>> +# A node the driver gave up on >>>> +# serves nothing at all: it keeps its image open, makes >>>> +# `query-named-block-nodes` fail for as long as it is in the graph, >>>> +# and `blockdev-del` removes it only once nothing refers to it. >>>> +# >>>> +# Reopening a dirty qcow2 image read-write repairs it first, which >>>> +# blocks other requests and can fail the command. >>>> +# >>>> # The command receives a list of block devices to reopen. For each >>>> # one of them, the top-level @node-name option (from >>>> # `BlockdevOptions`) must be specified and is used to select the block >>> These documentation updates suggest the patch affects commands >>> change-backing-file, block-commit, block-stream, and blockdev-reopen. >>> Is that correct? >>> >>> Would it make sense to list them in the commit message? >> That is simple thing. Will add :-) >> >> Thank you, >> Den > Thanks! > Thanks!
