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.)
Max
signature.asc
Description: OpenPGP digital signature
