Re: [RFC PATCH v2 00/26] Add subcluster allocation to qcow2

From: Max Reitz <mreitz@redhat.com>
To: Alberto Garcia <berto@igalia.com>, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	Anton Nefedov <anton.nefedov@virtuozzo.com>,
	qemu-block@nongnu.org,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	"Denis V . Lunev" <den@openvz.org>
Subject: Re: [RFC PATCH v2 00/26] Add subcluster allocation to qcow2
Date: Tue, 5 Nov 2019 14:32:36 +0100	[thread overview]
Message-ID: <7ff9d343-9148-6276-e068-f29d49f47fdc@redhat.com> (raw)
In-Reply-To: <cover.1572125022.git.berto@igalia.com>

[-- Attachment #1.1: Type: text/plain, Size: 2778 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]