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

-- 
2.20.1

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