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
signature.asc
Description: PGP signature
