Am 05.06.2020 um 14:11 hat Max Reitz geschrieben: > On 05.06.20 13:14, Kevin Wolf wrote: > > Am 03.06.2020 um 15:53 hat Max Reitz geschrieben: > >> On 15.04.20 21:02, Alberto Garcia wrote: > >>> Although we cannot create these images with qemu-img it is still > >>> possible to do it using an external tool. QEMU should refuse to open > >>> them until the data-file-raw bit is cleared with 'qemu-img check'. > >>> > >>> Signed-off-by: Alberto Garcia <be...@igalia.com> > >>> --- > >>> block/qcow2.c | 39 ++++++++++++++++++++++++++++++++++++++ > >>> tests/qemu-iotests/244 | 13 +++++++++++++ > >>> tests/qemu-iotests/244.out | 14 ++++++++++++++ > >>> 3 files changed, 66 insertions(+) > >> > >> Sorry for the long delay. :/ > >> > >> The patch itself looks good, but I’m not sure whether it is extensive > >> enough. Let me just jump straight to the problem: > >> > >> $ ./qemu-img create -f qcow2 \ > >> -o data_file=foo.qcow2.raw,data_file_raw=on \ > >> foo.qcow2 64M > >> (Create some file empty foo.qcow2 with external data file that’s raw) > >> > >> $ ./qemu-img create -f qcow2 backing.qcow2 64M > >> $ ./qemu-io -c 'write -P 42 0 64M' backing.qcow2 > >> (Create some file filled with 42s) > >> > >> $ ./qemu-img compare foo.qcow2 foo.qcow2.raw > >> Images are identical. > >> (As expected, foo.qcow2 is identical to its raw data file) > >> > >> $ ./qemu-img compare --image-opts \ > >> file.filename=foo.qcow2,backing.file.filename=backing.qcow2 \ > >> file.filename=foo.qcow2.raw > >> Content mismatch at offset 0! > >> (Oops.) > >> > >> So when the user manually gives a backing file without one having been > >> given by the image file, we run into the same problem. Now I’m not > >> quite sure what the problem is here. We could make this patch more > >> extensive and also forbid this case. > > > > I guess what we should really be checking is that bs->backing is NULL > > after the node is fully opened. The challenging part is that the backing > > child isn't managed by the block driver, but by the generic block layer, > > and .brv_open() comes first. So we don't really have a place to check > > this. (And there is also the case that the image is originally opened > > with BDRV_O_NO_BACKING and the later bdrv_open_backing_file().) > > > >> But I think there actually shouldn’t be a problem. The qcow2 driver > >> shouldn’t fall back to a backing file for raw external data files. But > >> how exactly should that be implemented? I think the correct way would > >> be to preallocate all metadata whenever data_file_raw=on – the qcow2 > >> spec doesn’t say to ignore the metadata with data_file_raw=on, it just > >> says that the data read from the qcow2 file must match that read from > >> the external data file. > >> (I seem to remember I proposed this before, but I don’t know exactly...) > > > > I don't find preallocation convincing, mostly for two reasons. > > > > First is, old images or images created by another program could miss the > > preallocation, but we still shouldn't access the backing file. > > I’d take this patch anyway (because its motivation is just that other > programs might produce invalid images), and then not worry about the > case where we get an image produced by such another program (including > older versions of qemu) for which the user overrides the backing file at > runtime. > > > The other one is that discard breaks preallocation, > > The preallocation is about ensuring that there are no > fall-through-to-backing holes in the image. Discarding doesn’t change that. > > > so we would also > > have to make sure to have a special case in every operation that could > > end up discarding clusters (and to add it to every future operation we > > might add). > > > > It just sounds very brittle. > > > >> (In contrast, I don’t think it would be correct to just treat > >> unallocated clusters as zero whenever data_file_raw=on.) > >> > >> What do you think? Should we force preallocation with data_file_raw=on, > >> and then just take this patch, even though it still lets users give > >> backing files to a qcow2 file at runtime without error? (Except the > >> backing file wouldn’t have an effect, then.) > > > > Honestly, maybe passing a backing file at runtime to an image that > > doesn't logically have one is just a case of "then don't do that". > > Perhaps. > > But seeing I wondered whether I didn’t already propose this at some > point, there is another reason for preallocation: > > https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00644.html > https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00329.html > > All in all, I think data_file_raw should be interpretable as “You don’t > have to look at any metadata to know which data to read or write”. > (Maybe I’m wrong about that.) > Without any preallocation of metadata structure, it looks to me like we > break that promise. > > (Yes, we could also force-zero the external data file during creation, > and blame users who put a backing file on images that don’t have one – > both of which are not unreasonable! But we could also just preallocate > the metadata.)
Makes sense. I'm not against preallocation during image creation. I just think it shouldn't play a role in deciding whether an image is valid or not. Kevin
signature.asc
Description: PGP signature
