| Message ID | cover.1577014346.git.berto@igalia.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add subcluster allocation to qcow2 | expand |
On 22.12.19 12:36, Alberto Garcia wrote: > Hi, > > here's the new version of the patches to add subcluster allocation > support to qcow2. > > Please refer to the cover letter of the first version for a full > description of the patches: > > https://lists.gnu.org/archive/html/qemu-block/2019-10/msg00983.html > > This version fixes many of the problems highlighted by Max. I decided > not to replace completely the cluster logic with subcluster logic in > all cases because I felt that sometimes it only complicated the code. > Let's see what you think :-) Looks good overall. :) So now I wonder on what your plans are after this series. Here are some things that come to my mind, and I wonder whether you plan to address them or whether there are more things to do still: - In v2, you had a patch for preallocation support with backing files. It didn’t quite work, which is why I think you dropped it for now (why not, it isn’t crucial). - There is a TODO on subcluster zeroing. - I think adding support to amend for switching extended_l2 on or off would make sense. But maybe it’s too complicated to be worth the effort. - As I noted in v2, I think it’d be great if it were possible to run the iotests with -o extended_l2=on. But I suppose this kind of depends on me adding data_file support to the Python tests first... Max
On Fri 21 Feb 2020 06:10:52 PM CET, Max Reitz wrote: > So now I wonder on what your plans are after this series. Apart from some fixes here and there, there are some things that I would live to solve: - I'm not 100% happy with the separation between QCow2ClusterType and QCow2SubclusterType. The former is a strict subset of the latter and doesn't carry any additional information, so I think there's no need to have both in the code. So I'm thinking to get rid of QCow2ClusterType altogether. - We discussed this already, and related to the previous point, in most places where the (sub)cluster type is checked what we want to know is whether there is a valid host address, or whether the data reads as zeroes, etc. So one possibility is to make qcow2_get_subcluster_type() return status flags like the existing BDRV_BLOCK_DATA, BDRV_BLOCK_OFFSET_VALID, ... and check those ones instead. Some functions become less verbose with this kind of approach, but I'm not sure that it works so well with others. - We also discussed this already, but qcow2_get_cluster_offset() returns an offset to the beginning of the cluster. This makes less sense when we start working at the subcluster level, but even at the moment the reality is that no one uses that offset. All callers use the final unaligned host offset. So I have a few patches that change that. > Here are some things that come to my mind, and I wonder whether you > plan to address them or whether there are more things to do still: > > - In v2, you had a patch for preallocation support with backing files. > It didn’t quite work, which is why I think you dropped it for now (why > not, it isn’t crucial). There was already a problem with preallocation and backing files ( https://lists.gnu.org/archive/html/qemu-block/2019-11/msg00691.html ) so I decided to withdraw the patches for subclusters and reevaluate the situation when that was sorted out. > - There is a TODO on subcluster zeroing. I'm not sure if I'll fix that now, but I'll give it a try when we all are happy with the rest the patches and the general design. > - I think adding support to amend for switching extended_l2 on or off > would make sense. But maybe it’s too complicated to be worth the > effort. I haven't thought about that, but it does sound too complicated to be worth it. > - As I noted in v2, I think it’d be great if it were possible to run > the iotests with -o extended_l2=on. But I suppose this kind of > depends on me adding data_file support to the Python tests first... Yes. Berto
Hi, here's the new version of the patches to add subcluster allocation support to qcow2. Please refer to the cover letter of the first version for a full description of the patches: https://lists.gnu.org/archive/html/qemu-block/2019-10/msg00983.html This version fixes many of the problems highlighted by Max. I decided not to replace completely the cluster logic with subcluster logic in all cases because I felt that sometimes it only complicated the code. Let's see what you think :-) Berto v3: - Patch 01: Rename host_offset to host_cluster_offset and make 'bytes' an unsigned int [Max] - Patch 03: Rename cluster_needs_cow to cluster_needs_new_alloc and count_cow_clusters to count_single_write_clusters. Update documentation and add more assertions and checks [Max] - Patch 09: Update qcow2_co_truncate() to properly support extended L2 entries [Max] - Patch 10: Forbid calling set_l2_bitmap() if the image does not have extended L2 entries [Max] - Patch 11 (new): Add QCow2SubclusterType [Max] - Patch 12 (new): Replace QCOW2_CLUSTER_* with QCOW2_SUBCLUSTER_* - Patch 13 (new): Handle QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC - Patch 14: Use QCow2SubclusterType instead of QCow2ClusterType [Max] - Patch 15: Use QCow2SubclusterType instead of QCow2ClusterType [Max] - Patch 19: Don't call set_l2_bitmap() if the image does not have extended L2 entries [Max] - Patch 21: Use smaller data types. - Patch 22: Don't call set_l2_bitmap() if the image does not have extended L2 entries [Max] - Patch 23: Use smaller data types. - Patch 25: Update test results and documentation. Move the check for the minimum subcluster size to validate_cluster_size(). - Patch 26 (new): Add subcluster support to qcow2_measure() - Patch 27: Add more tests v2: https://lists.gnu.org/archive/html/qemu-block/2019-10/msg01642.html - Patch 12: Update after the changes in 88f468e546. - Patch 21 (new): Clear the L2 bitmap when allocating a compressed cluster. Compressed clusters should have the bitmap all set to 0. - Patch 24: Document the new fields in the QAPI documentation [Eric]. - Patch 25: Allow qcow2 preallocation with backing files. - Patch 26: Add some tests for qcow2 images with extended L2 entries. v1: https://lists.gnu.org/archive/html/qemu-block/2019-10/msg00983.html Output of git backport-diff against v2: Key: [----] : patches are identical [####] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/27:[0013] [FC] 'qcow2: Add calculate_l2_meta()' 002/27:[----] [-C] 'qcow2: Split cluster_needs_cow() out of count_cow_clusters()' 003/27:[0083] [FC] 'qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC clusters in handle_copied()' 004/27:[----] [-C] 'qcow2: Add get_l2_entry() and set_l2_entry()' 005/27:[----] [--] 'qcow2: Document the Extended L2 Entries feature' 006/27:[----] [--] 'qcow2: Add dummy has_subclusters() function' 007/27:[----] [--] 'qcow2: Add subcluster-related fields to BDRVQcow2State' 008/27:[----] [--] 'qcow2: Add offset_to_sc_index()' 009/27:[0008] [FC] 'qcow2: Add l2_entry_size()' 010/27:[0008] [FC] 'qcow2: Update get/set_l2_entry() and add get/set_l2_bitmap()' 011/27:[down] 'qcow2: Add QCow2SubclusterType and qcow2_get_subcluster_type()' 012/27:[down] 'qcow2: Replace QCOW2_CLUSTER_* with QCOW2_SUBCLUSTER_*' 013/27:[down] 'qcow2: Handle QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC' 014/27:[0060] [FC] 'qcow2: Add subcluster support to calculate_l2_meta()' 015/27:[0091] [FC] 'qcow2: Add subcluster support to qcow2_get_cluster_offset()' 016/27:[----] [--] 'qcow2: Add subcluster support to zero_in_l2_slice()' 017/27:[----] [--] 'qcow2: Add subcluster support to discard_in_l2_slice()' 018/27:[----] [--] 'qcow2: Add subcluster support to check_refcounts_l2()' 019/27:[0008] [FC] 'qcow2: Add subcluster support to expand_zero_clusters_in_l1()' 020/27:[----] [--] 'qcow2: Fix offset calculation in handle_dependencies()' 021/27:[0007] [FC] 'qcow2: Update L2 bitmap in qcow2_alloc_cluster_link_l2()' 022/27:[0004] [FC] 'qcow2: Clear the L2 bitmap when allocating a compressed cluster' 023/27:[0002] [FC] 'qcow2: Add subcluster support to handle_alloc_space()' 024/27:[----] [-C] 'qcow2: Restrict qcow2_co_pwrite_zeroes() to full clusters only' 025/27:[0049] [FC] 'qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit' 026/27:[down] 'qcow2: Add subcluster support to qcow2_measure()' 027/27:[0046] [FC] 'iotests: Add tests for qcow2 images with extended L2 entries' Alberto Garcia (27): qcow2: Add calculate_l2_meta() qcow2: Split cluster_needs_cow() out of count_cow_clusters() qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC clusters in handle_copied() qcow2: Add get_l2_entry() and set_l2_entry() qcow2: Document the Extended L2 Entries feature qcow2: Add dummy has_subclusters() function qcow2: Add subcluster-related fields to BDRVQcow2State qcow2: Add offset_to_sc_index() qcow2: Add l2_entry_size() qcow2: Update get/set_l2_entry() and add get/set_l2_bitmap() qcow2: Add QCow2SubclusterType and qcow2_get_subcluster_type() qcow2: Replace QCOW2_CLUSTER_* with QCOW2_SUBCLUSTER_* qcow2: Handle QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC qcow2: Add subcluster support to calculate_l2_meta() qcow2: Add subcluster support to qcow2_get_cluster_offset() qcow2: Add subcluster support to zero_in_l2_slice() qcow2: Add subcluster support to discard_in_l2_slice() qcow2: Add subcluster support to check_refcounts_l2() qcow2: Add subcluster support to expand_zero_clusters_in_l1() qcow2: Fix offset calculation in handle_dependencies() qcow2: Update L2 bitmap in qcow2_alloc_cluster_link_l2() qcow2: Clear the L2 bitmap when allocating a compressed cluster qcow2: Add subcluster support to handle_alloc_space() qcow2: Restrict qcow2_co_pwrite_zeroes() to full clusters only qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit qcow2: Add subcluster support to qcow2_measure() iotests: Add tests for qcow2 images with extended L2 entries block/qcow2-cluster.c | 645 ++++++++++++++++++++----------- block/qcow2-refcount.c | 38 +- block/qcow2.c | 200 +++++++--- block/qcow2.h | 150 ++++++- docs/interop/qcow2.txt | 68 +++- docs/qcow2-cache.txt | 19 +- include/block/block_int.h | 1 + qapi/block-core.json | 7 + tests/qemu-iotests/031.out | 8 +- tests/qemu-iotests/036.out | 4 +- tests/qemu-iotests/049.out | 102 ++--- tests/qemu-iotests/060.out | 1 + tests/qemu-iotests/061.out | 20 +- tests/qemu-iotests/065 | 18 +- tests/qemu-iotests/082.out | 48 ++- tests/qemu-iotests/085.out | 38 +- tests/qemu-iotests/144.out | 4 +- tests/qemu-iotests/182.out | 2 +- tests/qemu-iotests/185.out | 8 +- tests/qemu-iotests/198.out | 2 + tests/qemu-iotests/206.out | 4 + tests/qemu-iotests/242.out | 5 + tests/qemu-iotests/255.out | 8 +- tests/qemu-iotests/271 | 256 ++++++++++++ tests/qemu-iotests/271.out | 208 ++++++++++ tests/qemu-iotests/273.out | 9 +- tests/qemu-iotests/common.filter | 1 + tests/qemu-iotests/group | 1 + 28 files changed, 1455 insertions(+), 420 deletions(-) create mode 100755 tests/qemu-iotests/271 create mode 100644 tests/qemu-iotests/271.out