Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw
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
Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw
On 22.06.20 19:36, Alberto Garcia wrote: [...] > You would still have problems with images created with raw data files > but without preallocation. Yeah, sure, but, well. Bug in qemu. :/ Max signature.asc Description: OpenPGP digital signature
Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw
On 6/22/20 10:48 AM, Max Reitz wrote: As I noted in my reply to myself, data-file-raw is an autoclear flag. That means, an old version of qemu that doesn’t recognize the flag must read the same data as a new version. It follows that the the L2 tables must be a 1:1 mapping. (Or the flag can’t be an autoclear flag.) Yes, that argument is the strongest I've seen for why both creation and resize with a data-file-raw image should require metadata preallocation. In other words, we never want to expose different guest data merely because we opened the file with an older version (the older version must either see the same data [an autoclear bit is fine], or must know that it cannot reliably open the image [an incompatible bit is needed]). Being able to read sounds like a nice to have feature, but what about writing? I hope that the image is not writable by older versions that do not understand data_file. Otherwise older qemu versions can corrupt the image silently. It’s an autoclear flag. That means such versions of qemu will automatically clear the flag. Well, an older version is only required to clear the flag if it modifies the image; if it opens the image read-only, it may leave the autoclear flag set. So you are more realistically guaranteed that the autoclear flag is only cleared when writing to the image with a version that did not understand the autoclear flag. Following that line of thought further - reopening the image under a qemu that once again understands the data-file-raw flag will now see that the file is no longer raw because the flag was cleared. That is, if you are expecting data-file-raw and no longer see the flag, then you KNOW that the qcow2 file was modified by an older program that didn't necessarily preserve the 1:1 correspondence, and it is up to you what to do next (refuse to use the file, do a pass over the qcow2 file to flush contents back to the raw file, or any number of other reactions...). So autoclear flags are useful for features that are optional, but that may be broken when the image is written to by versions of qemu that don’t understand them.) More importantly, autoclear flags are useful for features where older software can safely read the image without data loss, but where older software modifying the image may lose whatever property the bit was guaranteeing when interpreted correctly. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw
On Mon 22 Jun 2020 05:06:53 PM CEST, Max Reitz wrote: >>> 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.) >> >> But you wouldn't fix that preallocating the metadata either, you >> would need to fill the device with zeroes. > > What it fixes is that reading the qcow2 image and the raw device > returns the same data. > > Initially, I also thought that we should initialize raw data files to > be zero during creation, but Eric changed my mind: > > https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00223.html Then in that case you would indeed need to preallocate all the metadata, both on creation and on resize. Then if you read the image with an older version of QEMU you would get the exact same contents in both cases. You would still have problems with images created with raw data files but without preallocation. Coincidentally, data_file and data_file_raw were both introduced in QEMU 4.0 so in practice there's no official QEMU release that can read an image with an external data file but does not support data_file_raw. But in theory it could happen of course. Berto
Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw
On 22.06.20 17:15, Nir Soffer wrote:
> On Mon, Jun 22, 2020 at 6:07 PM Max Reitz wrote:
>>
>> On 22.06.20 16:46, Alberto Garcia wrote:
>>> On Mon 22 Jun 2020 11:35:59 AM 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).
>>>
>>> What happens if you first create the image (which would be preallocated
>>> with this patch), then you resize it and finally you assign the backing
>>> file? Would the resized part be preallocated?
>>
>> Good point, when resizing an image with data-file-raw we also need to
>> preallocate the L2 tables.
>>
> 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.)
>>>
>>> But you wouldn't fix that preallocating the metadata either, you would
>>> need to fill the device with zeroes.
>>
>> What it fixes is that reading the qcow2 image and the raw device returns
>> the same data.
>>
>> Initially, I also thought that we should initialize raw data files to be
>> zero during creation, but Eric changed my mind:
>>
>> https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00223.html
>>
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.)
>>>
>>> The way I interpret it is that regardless of whether you read the data
>>> through the qcow2 file or directly from the data file you should get the
>>> same results, but how that should be reflected in the L1/L2 metadata is
>>> not specified.
>>
>> That’s an absolute given, but the question is what does “reading through
>> the qcow2 file” mean. Respecting the metadata? Ignoring it? Something
>> in between?
>>
>> As I noted in my reply to myself, data-file-raw is an autoclear flag.
>> That means, an old version of qemu that doesn’t recognize the flag must
>> read the same data as a new version. It follows that the the L2 tables
>> must be a 1:1 mapping. (Or the flag can’t be an autoclear flag.)
>
> Being able to read sounds like a nice to have feature, but what about writing?
>
> I hope that the image is not writable by older versions that do not understand
> data_file. Otherwise older qemu versions can corrupt the image silently.
It’s an autoclear flag. That means such versions of qemu will
automatically clear the flag.
(To elaborate, there are three kinds of flags for qcow2 images:
Incompatible flags, compatible flags, and autoclear flags.
When qemu encounters an image with an unknown incompatible flag, it
refuses to open the image.
When it encounters an unknown compatible flag, it just ignores that flag
(but keeps it set).
When it e
Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw
On Mon, Jun 22, 2020 at 6:07 PM Max Reitz wrote:
>
> On 22.06.20 16:46, Alberto Garcia wrote:
> > On Mon 22 Jun 2020 11:35:59 AM 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).
> >
> > What happens if you first create the image (which would be preallocated
> > with this patch), then you resize it and finally you assign the backing
> > file? Would the resized part be preallocated?
>
> Good point, when resizing an image with data-file-raw we also need to
> preallocate the L2 tables.
>
> >>> 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.)
> >
> > But you wouldn't fix that preallocating the metadata either, you would
> > need to fill the device with zeroes.
>
> What it fixes is that reading the qcow2 image and the raw device returns
> the same data.
>
> Initially, I also thought that we should initialize raw data files to be
> zero during creation, but Eric changed my mind:
>
> https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00223.html
>
> >> 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.)
> >
> > The way I interpret it is that regardless of whether you read the data
> > through the qcow2 file or directly from the data file you should get the
> > same results, but how that should be reflected in the L1/L2 metadata is
> > not specified.
>
> That’s an absolute given, but the question is what does “reading through
> the qcow2 file” mean. Respecting the metadata? Ignoring it? Something
> in between?
>
> As I noted in my reply to myself, data-file-raw is an autoclear flag.
> That means, an old version of qemu that doesn’t recognize the flag must
> read the same data as a new version. It follows that the the L2 tables
> must be a 1:1 mapping. (Or the flag can’t be an autoclear flag.)
Being able to read sounds like a nice to have feature, but what about writing?
I hope that the image is not writable by older versions that do not understand
data_file. Otherwise older qemu versions can corrupt the image silently.
Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw
On 22.06.20 16:46, Alberto Garcia wrote:
> On Mon 22 Jun 2020 11:35:59 AM 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).
>
> What happens if you first create the image (which would be preallocated
> with this patch), then you resize it and finally you assign the backing
> file? Would the resized part be preallocated?
Good point, when resizing an image with data-file-raw we also need to
preallocate the L2 tables.
>>> 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.)
>
> But you wouldn't fix that preallocating the metadata either, you would
> need to fill the device with zeroes.
What it fixes is that reading the qcow2 image and the raw device returns
the same data.
Initially, I also thought that we should initialize raw data files to be
zero during creation, but Eric changed my mind:
https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00223.html
>> 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.)
>
> The way I interpret it is that regardless of whether you read the data
> through the qcow2 file or directly from the data file you should get the
> same results, but how that should be reflected in the L1/L2 metadata is
> not specified.
That’s an absolute given, but the question is what does “reading through
the qcow2 file” mean. Respecting the metadata? Ignoring it? Something
in between?
As I noted in my reply to myself, data-file-raw is an autoclear flag.
That means, an old version of qemu that doesn’t recognize the flag must
read the same data as a new version. It follows that the the L2 tables
must be a 1:1 mapping. (Or the flag can’t be an autoclear flag.)
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw
On Mon 22 Jun 2020 11:35:59 AM 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).
What happens if you first create the image (which would be preallocated
with this patch), then you resize it and finally you assign the backing
file? Would the resized part be preallocated?
>> 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.)
But you wouldn't fix that preallocating the metadata either, you would
need to fill the device with zeroes.
> 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.)
The way I interpret it is that regardless of whether you read the data
through the qcow2 file or directly from the data file you should get the
same results, but how that should be reflected in the L1/L2 metadata is
not specified.
Berto
Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw
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.
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw
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
Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw
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, 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.
Berto
[PATCH 1/2] qcow2: Force preallocation with data-file-raw
Setting the qcow2 data-file-raw bit means that you can ignore the
qcow2 metadata when reading from the external data file. It does not
mean that you have to ignore it, though. Therefore, the data read must
be the same regardless of whether you interpret the metadata or whether
you ignore it, and thus the L1/L2 tables must all be present and give a
1:1 mapping.
This patch changes 244's output: First, the qcow2 file is larger right
after creation, because of metadata preallocation. Second, the qemu-img
map output changes: Everything that was not explicitly discarded or
zeroed is now a data area.
Signed-off-by: Max Reitz
---
block/qcow2.c | 22 ++
tests/qemu-iotests/244.out | 9 -
2 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 0cd2e6757e..2a588d8091 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3460,6 +3460,28 @@ qcow2_co_create(BlockdevCreateOptions *create_options,
Error **errp)
ret = -EINVAL;
goto out;
}
+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;
+
+/*
+ * Cannot use preallocation with backing files, but giving a
+ * backing file when specifying data_file_raw is an error
+ * anyway.
+ */
+assert(!qcow2_opts->has_backing_file);
+}
if (qcow2_opts->data_file) {
if (version < 3) {
diff --git a/tests/qemu-iotests/244.out b/tests/qemu-iotests/244.out
index dbab7359a9..24f02363dd 100644
--- a/tests/qemu-iotests/244.out
+++ b/tests/qemu-iotests/244.out
@@ -83,7 +83,7 @@ qcow2 file size after I/O: 327680
=== Standalone image with external data file (valid raw) ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
data_file=TEST_DIR/t.IMGFMT.data data_file_raw=on
-qcow2 file size before I/O: 196616
+qcow2 file size before I/O: 327680
wrote 4194304/4194304 bytes at offset 1048576
4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -93,11 +93,10 @@ wrote 3145728/3145728 bytes at offset 3145728
3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
No errors were found on the image.
-[{ "start": 0, "length": 1048576, "depth": 0, "zero": true, "data": false},
-{ "start": 1048576, "length": 1048576, "depth": 0, "zero": false, "data":
true, "offset": 1048576},
+[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true,
"offset": 0},
{ "start": 2097152, "length": 2097152, "depth": 0, "zero": true, "data":
false},
-{ "start": 4194304, "length": 1048576, "depth": 0, "zero": true, "data":
false, "offset": 4194304},
-{ "start": 5242880, "length": 61865984, "depth": 0, "zero": true, "data":
false}]
+{ "start": 4194304, "length": 2097152, "depth": 0, "zero": true, "data":
false, "offset": 4194304},
+{ "start": 6291456, "length": 60817408, "depth": 0, "zero": false, "data":
true, "offset": 6291456}]
read 1048576/1048576 bytes at offset 0
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
--
2.26.2
