Re: [PATCH 1/2] block: commit: Allow users to request only format driver names in backing file format

[Peter Krempa]

On Tue, Nov 28, 2023 at 20:10:10 +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 24.11.23 17:52, Peter Krempa wrote:
> > Introduce a new flag 'backing_file_format_no_protocol' for the
> > block-commit QMP command which instructs the internals to use 'raw'
> > instead of the protocol driver in case when a image is used without a
> > dummy 'raw' wrapper.
> > 
> > The flag is designed such that it can be always asserted by management
> > tools even when there isn't any update to backing files.
> > 
> > The flag will be used by libvirt so that the backing images still
> > reference the proper format even when libvirt will stop using the dummy
> > raw driver (raw driver with no other config). Libvirt needs this so that
> > the images stay compatible with older libvirt versions which didn't
> > expect that a protocol driver name can appear in the backing file format
> > field.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---

[...]

> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index ca390c5700..367e896905 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1810,6 +1810,14 @@
> >   #     Care should be taken when specifying the string, to specify a
> >   #     valid filename or protocol.  (Since 2.1)
> >   #
> > +# @backing-file-format-no-protocol: If true always use a 'format' driver 
> > name
> > +#     for the 'backing file format' field if updating the image header of 
> > the
> > +#     overlay of 'top'. Otherwise the real name of the driver of the 
> > backing
> > +#     image may be used which may be a protocol driver.
> > +#
> > +#     Can be used also when no image header will be updated.
> > +#     (default: false; since: 8.2)
> > +#
> 
> Hi Peter.

Hi,

Firstly to be honest, I consider the fact that qemu can put a protocol
driver into the header field named "backing file format" to be a bug.
After discussion with Kevin I understand the rationale of doing so, but
nevertheless, a backing protocol name is not a format and should have
had it's own header field.

> Hmm. Could this just be @backing-file-format ?

I don't really care too deeply about the name.

Calling it just @backing-file-format though would imply (as you write
below) that as argument the string to write into the metadata. More on
that below.

> As I understand, finally, that's just a string which we are going to put into 
> qcow2 metadata.

Yes.

> And from qcow2 point of view, it's rather strange to set 
> backing_file_format="raw" for backing image which is actually "qcow2".

Indeed, that would be wrong, but this is _NOT_ what this patch
actually does.

This patch ensures that only a *format* driver name is ever written to
the backing image. It overrides the fact that a *protocol* driver name
would be written into the field if the tail of the backing chain is a
raw image, which was instantiated in qemu without the dummy 'raw' driver
on top.

Since a raw driver with no configuration simply passes request to the
*protocol* driver below it it's not needed in most configs. In fact as
stefanha figured out a long time ago it's just simpy overhead.

We need a format name in the backing file format field as libvirt
assumed for a very long time that such a field would contain only format
drivers, and since I want to remove the overhead of the 'raw' driver I
need to ensure that images won't break for older libvirt.

>"raw" say nothing to the reader and may be even misleading. Same for qemu, 
>this seems just a strange thing.
> 
> Also, what I dislike, that new feature sounds like hardcoded "raw" is the 
> only format driver. If go this way, more honest name would be 
> @backing-file-raw.

Once again, that is not what's happening here. The field enables that
only a *format* driver is used. This is only a problem when the final
image is raw, but without the dummy 'raw' driver on top of it. Thus
that's the reason the workaround only ever writes 'raw' into it.
Otherwise the proper format name is in the 'driver' field already and is
not overwritten.

> And, if we allow user to give any backing-file name to be written into qcow2 
> metadata after the operation, why not just allow to set any backing-file-name 
> as well?

This IMO makes no sense to allow, but based on the reimagined design of
the 'backing file /format/' field it might appear that it does.

'block-commit' and 'block-stream, can't change the format of any of the
images, they simply move data, thus for any non-raw image in a chain it
must still use actual format name. Only place it makes sense is in the
same cases when the code in this patch triggers.

And then it still doesn't make sense to write anything besides:
1) The actual *format* of the image (thus 'raw')
2) The protocol driver of the last image (e.g. NBD)

But since 'block-stream'/'block-commit' don't move the image to any
other storage, allowing the user to write any other protocol also
doesn't make any sense to write any other protocol.

Thus the two cases above are the only sane, this patch allows that 1) is
written into the image.

> So, I think simple @backing-file-format argument, allowing to set any string 
> as backing format is a better concept.

While libvirt certainly has the data to do so, as noted in the paragraph
above it doesn't IMO make sense to allow write any arbitrary driver name
there.

And with design like this it allows libvirt to unconditionally use the
flag without thinking too much about what is the correct value.

> Moreover: in BlockdevCreateOptionsQcow2 we have backing-file and backing-fmt 
> options. So I think, we should follow this and name the new option for 
> block-job @backing-fmt.

This is different, you are creating an image and declaring it's backing
image format. With 'block-stream' and 'block-commit' the format of the
backing image won't change, just data will be shuffled around.