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