[RFC,v2,00/26] Add subcluster allocation to qcow2

Message

Alberto Garcia Oct. 26, 2019, 9:25 p.m. UTC

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 includes a few tests, but I'm planning to add more for
the next revision.

Berto

v2:
- 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 v1:

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/26:[----] [-C] 'qcow2: Add calculate_l2_meta()'
002/26:[----] [--] 'qcow2: Split cluster_needs_cow() out of count_cow_clusters()'
003/26:[----] [--] 'qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC clusters in handle_copied()'
004/26:[----] [--] 'qcow2: Add get_l2_entry() and set_l2_entry()'
005/26:[----] [--] 'qcow2: Document the Extended L2 Entries feature'
006/26:[----] [--] 'qcow2: Add dummy has_subclusters() function'
007/26:[----] [--] 'qcow2: Add subcluster-related fields to BDRVQcow2State'
008/26:[----] [--] 'qcow2: Add offset_to_sc_index()'
009/26:[----] [--] 'qcow2: Add l2_entry_size()'
010/26:[----] [--] 'qcow2: Update get/set_l2_entry() and add get/set_l2_bitmap()'
011/26:[----] [--] 'qcow2: Add qcow2_get_subcluster_type()'
012/26:[0005] [FC] 'qcow2: Handle QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER'
013/26:[----] [--] 'qcow2: Add subcluster support to calculate_l2_meta()'
014/26:[----] [--] 'qcow2: Add subcluster support to qcow2_get_cluster_offset()'
015/26:[----] [--] 'qcow2: Add subcluster support to zero_in_l2_slice()'
016/26:[----] [--] 'qcow2: Add subcluster support to discard_in_l2_slice()'
017/26:[----] [--] 'qcow2: Add subcluster support to check_refcounts_l2()'
018/26:[----] [--] 'qcow2: Add subcluster support to expand_zero_clusters_in_l1()'
019/26:[----] [--] 'qcow2: Fix offset calculation in handle_dependencies()'
020/26:[----] [--] 'qcow2: Update L2 bitmap in qcow2_alloc_cluster_link_l2()'
021/26:[down] 'qcow2: Clear the L2 bitmap when allocating a compressed cluster'
022/26:[----] [--] 'qcow2: Add subcluster support to handle_alloc_space()'
023/26:[----] [--] 'qcow2: Restrict qcow2_co_pwrite_zeroes() to full clusters only'
024/26:[0007] [FC] 'qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit'
025/26:[down] 'qcow2: Allow preallocation and backing files if extended_l2 is set'
026/26:[down] 'iotests: Add tests for qcow2 images with extended L2 entries'

Alberto Garcia (26):
  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 qcow2_get_subcluster_type()
  qcow2: Handle QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER
  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: Allow preallocation and backing files if extended_l2 is set
  iotests: Add tests for qcow2 images with extended L2 entries

 block/qcow2-cluster.c            | 548 ++++++++++++++++++++-----------
 block/qcow2-refcount.c           |  38 ++-
 block/qcow2.c                    |  92 +++++-
 block/qcow2.h                    | 121 ++++++-
 docs/interop/qcow2.txt           |  68 +++-
 docs/qcow2-cache.txt             |  19 +-
 include/block/block_int.h        |   1 +
 qapi/block-core.json             |   6 +
 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       |   6 +-
 tests/qemu-iotests/242.out       |   5 +
 tests/qemu-iotests/255.out       |   8 +-
 tests/qemu-iotests/271           | 235 +++++++++++++
 tests/qemu-iotests/271.out       | 183 +++++++++++
 tests/qemu-iotests/common.filter |   1 +
 tests/qemu-iotests/group         |   1 +
 27 files changed, 1247 insertions(+), 340 deletions(-)
 create mode 100755 tests/qemu-iotests/271
 create mode 100644 tests/qemu-iotests/271.out

Comments

Max Reitz Nov. 5, 2019, 1:32 p.m. UTC | #1

On 26.10.19 23:25, 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 includes a few tests, but I'm planning to add more for
> the next revision.

I think what would help most with testing is if it were possible to
simply run the iotests with -o extended_l2=on.

In general, the RFC looks OK to me.  The one thing I dislike is that I
feel that it is a bit, well, uncourageous.  Now, after looking at the
series, I don’t know whether you really changed everything that needs to
be changed so it can deal with subclusters.

To me it feels like this is because you tried to keep everything as it
is and only do minimal changes.  That is usually a good thing, but here
I don’t know, because this way we can’t simply grep for places that need
fixing (because they use /\<cluster/ instead of /subcluster/).

To me it feels like with subclusters, the whole design should be
different.  Note that the following is just a very naïve idea, but
anyway: I feel like what we need to separate isn’t L2 entries vs.
clusters vs. subclusters (so a separation based on, well, syntax?) but a
separation based on offset vs. allocation status (so a separation based
on semantics).

So I imagine there would be one function that sets a whole cluster’s
(i.e., a group of subclusters) allocation offset; and another function
that sets individual subclusters’ allocation status.

Reversely, there should be a function to query a cluster’s/subcluster’s
allocation offset, and another to query a subcluster’s type.

To me it looks like the places where
QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER is handled separately from
QCOW2_CLUSTER_UNALLOCATED are places where we’re really not interested
in the subcluster’s type at all, but just whether there’s an allocation
or not.  This is the case in qcow2_get_cluster_offset(),
calculate_l2_meta(), and qcow2_co_block_status().

(These places should then use the function to query the allocation
offset and evaluate the result instead of querying the subcluster type.)


Does that sound in any way acceptable to you?  You have more insight
into this now and so maybe you know already that it can’t work.
(Or maybe it’s just too invasive.)


Right now, all I can do is grep for QCOW2_CLUSTER_ and set_l2_entry().
And all of those places look OK to me.  But I just can’t shake off the
uneasy feeling of not being able to really know whether this series
really got to all the places that need adjustment.

Max