Re: [RFC PATCH v3 00/27] Add subcluster allocation to qcow2

[Alberto Garcia]

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