Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3742 bytes --]

Am 22.06.2020 um 11:48 hat Max Reitz geschrieben:
> On 22.06.20 11:35, Max Reitz wrote:
> > On 19.06.20 18:47, Alberto Garcia wrote:
> >> On Fri 19 Jun 2020 12:40:11 PM CEST, Max Reitz wrote:
> >>> +    if (qcow2_opts->data_file_raw &&
> >>> +        qcow2_opts->preallocation == PREALLOC_MODE_OFF)
> >>> +    {
> >>> +        /*
> >>> +         * data-file-raw means that "the external data file can be
> >>> +         * read as a consistent standalone raw image without looking
> >>> +         * at the qcow2 metadata."  It does not say that the metadata
> >>> +         * must be ignored, though (and the qcow2 driver in fact does
> >>> +         * not ignore it), so the L1/L2 tables must be present and
> >>> +         * give a 1:1 mapping, so you get the same result regardless
> >>> +         * of whether you look at the metadata or whether you ignore
> >>> +         * it.
> >>> +         */
> >>> +        qcow2_opts->preallocation = PREALLOC_MODE_METADATA;
> >>
> >> I'm not convinced by this,
> > 
> > Why not?
> > 
> > This is how I read the spec.  Furthermore, I see two problems that we
> > have right now that are fixed by this patch (namely (1) using a device
> > file as the external data file, which may have non-zero data at
> > creation; and (2) assigning a backing file at runtime must not show the
> > data).
> > 
> >> but your comment made me think of another
> >> possible alternative: in qcow2_get_cluster_offset(), if the cluster is
> >> unallocated and we are using a raw data file then we return _ZERO_PLAIN:
> >>
> >> --- a/block/qcow2-cluster.c
> >> +++ b/block/qcow2-cluster.c
> >> @@ -654,6 +654,10 @@ out:
> >>      assert(bytes_available - offset_in_cluster <= UINT_MAX);
> >>      *bytes = bytes_available - offset_in_cluster;
> >>  
> >> +    if (type == QCOW2_CLUSTER_UNALLOCATED && data_file_is_raw(bs)) {
> >> +        type = QCOW2_CLUSTER_ZERO_PLAIN;
> >> +    }
> >> +
> >>      return type;
> >>
> >> You could even add a '&& bs->backing' to the condition and emit a
> >> warning to make it more explicit.
> > 
> > No, this is wrong.  This still wouldn’t fix the problem of having a
> > device file as the external data file, when it already has non-zero data
> > during creation.  (Reading the qcow2 file would return zeroes, but
> > reading the device would not.)
> > 
> > So it would need to be QCOW2_CLUSTER_NORMAL.  Which is kind of the
> > point, when you think about it – with data-file-raw, all clusters must
> > always effectively be QCOW2_CLUSTER_NORMAL and be mapped 1:1.
> > 
> > Well, and that’s in turn the point of this patch.
> > 
> > I interpret the spec in that the metadata can be ignored, but it does
> > not need to be ignored.  So the L1/L2 tables must be 1:1 mapping of
> > QCOW2_CLUSTER_NORMAL entries.
> > 
> > We could also choose to interpret it as “With data-file-raw, the L1/L2
> > tables must be ignored”.  In that case, our qcow2 driver would need to
> > be modified to indeed fully ignore the L1/L2 tables with data-file-raw.
> >  (I certainly don’t interpret the spec this way, but I suppose we could
> > call it a bug fix and amend it.)
> 
> I just realized that this is not possible.  data-file-raw is an
> autoclear flag, so the image must appear the same to qemu versions that
> do not support it.
> 
> If we want to fully ignore the L1/L2 tables or interpret them some
> non-default way (like you’re proposing), we would have to add a new
> incompatible flag.

We could drop the data-file-raw autoclear flag (causing new QEMU
versions to downgrade any existing images) and call the new incompatible
flag data-file-raw (leaving the user interface unchanged).

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]