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 14/26] qcow2: Add subcluster support to qcow2_get_cluster_offset()
Date: Mon, 4 Nov 2019 15:58:57 +0100 [thread overview]
Message-ID: <673d72da-bf8c-3ffb-a324-79e93f88a140@redhat.com> (raw)
In-Reply-To: <6932c2ddfe19a564cad7c54246290e166525fc46.1572125022.git.berto@igalia.com>
[-- Attachment #1.1: Type: text/plain, Size: 5452 bytes --]
On 26.10.19 23:25, Alberto Garcia wrote:
> The logic of this function remains pretty much the same, except that
> it uses count_contiguous_subclusters(), which combines the logic of
> count_contiguous_clusters() / count_contiguous_clusters_unallocated()
> and checks individual subclusters.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block/qcow2-cluster.c | 111 ++++++++++++++++++++----------------------
> 1 file changed, 52 insertions(+), 59 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 990bc070af..e67559152f 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -372,66 +372,51 @@ fail:
> }
>
> /*
> - * Checks how many clusters in a given L2 slice are contiguous in the image
> - * file. As soon as one of the flags in the bitmask stop_flags changes compared
> - * to the first cluster, the search is stopped and the cluster is not counted
> - * as contiguous. (This allows it, for example, to stop at the first compressed
> - * cluster which may require a different handling)
> + * Return the number of contiguous subclusters of the exact same type
> + * in a given L2 slice, starting from cluster @l2_index, subcluster
> + * @sc_index. At most @nb_clusters are checked. Allocated clusters are
I’d add a note that reassures that @nb_clusters really is a number of
clusters, not subclusters. (Because if the variable were named “x”
(i.e., it’d be “At most @x are checked”), you’d assume it refers to a
number of subclusters.)
OTOH, what I don’t like so far about this series is that the “cluster
logic” is still everywhere when I think it should just be about
subclusters now. (Except in few places where it must be about clusters
as in something that can have a distinct host offset and/or has an own
L2 entry.) So maybe the parameter should really be @nb_subclusters.
But I’m not sure. For how this function is written right now, it makes
sense for it to be @nb_clusters, but I think it could be changed so it
would work with @nb_subclusters, too. (Just a single loop over the
subclusters and then loading l2_entry+l2_bitmap and checking the offset
at every cluster boundary.)
> + * also required to be contiguous in the image file.
> */
> -static int count_contiguous_clusters(BlockDriverState *bs, int nb_clusters,
> - int cluster_size, uint64_t *l2_slice, int l2_index, uint64_t stop_flags)
> +static int count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters,
> + unsigned sc_index, uint64_t *l2_slice,
> + int l2_index)
> {
> BDRVQcow2State *s = bs->opaque;
> - int i;
> - QCow2ClusterType first_cluster_type;
> - uint64_t mask = stop_flags | L2E_OFFSET_MASK | QCOW_OFLAG_COMPRESSED;
> - uint64_t first_entry = get_l2_entry(s, l2_slice, l2_index);
> - uint64_t offset = first_entry & mask;
> -
> - first_cluster_type = qcow2_get_cluster_type(bs, first_entry);
> - if (first_cluster_type == QCOW2_CLUSTER_UNALLOCATED) {
> - return 0;
> + int i, j, count = 0;
> + uint64_t l2_entry = get_l2_entry(s, l2_slice, l2_index);
> + uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index);
> + uint64_t expected_offset = l2_entry & L2E_OFFSET_MASK;
> + bool check_offset = true;
> + QCow2ClusterType type =
> + qcow2_get_subcluster_type(bs, l2_entry, l2_bitmap, sc_index);
> +
> + assert(type != QCOW2_CLUSTER_INVALID); /* The caller should check this */
> +
> + if (type == QCOW2_CLUSTER_COMPRESSED) {
> + return 1; /* Compressed clusters are always counted one by one */
Hm, yes, but cluster by cluster, not subcluster by subcluster, so this
should be s->subclusters_per_cluster, and perhaps sc_index should be
asserted to be 0. (Or it should be s->subclusters_per_cluster - sc_index.)
[...]
> @@ -514,8 +499,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
I suppose this is get_subcluster_offset now.
[...]
> @@ -587,6 +574,13 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
> goto fail;
> }
> switch (type) {
> + case QCOW2_CLUSTER_INVALID:
> + qcow2_signal_corruption(bs, true, -1, -1, "Invalid cluster entry found "
> + " (L2 offset: %#" PRIx64 ", L2 index: %#x)",
> + l2_offset, l2_index);
> + ret = -EIO;
> + goto fail;
> + break;
No need to break here.
> case QCOW2_CLUSTER_COMPRESSED:
> if (has_data_file(bs)) {
> qcow2_signal_corruption(bs, true, -1, -1, "Compressed cluster "
> @@ -602,16 +596,15 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
> break;
Here (QCOW2_CLUSTER_COMPRESSED), c is 1, even though it should count
subclusters.
Max
> case QCOW2_CLUSTER_ZERO_PLAIN:
> case QCOW2_CLUSTER_UNALLOCATED:
> - /* how many empty clusters ? */
> - c = count_contiguous_clusters_unallocated(bs, nb_clusters,
> - l2_slice, l2_index, type);
> + c = count_contiguous_subclusters(bs, nb_clusters, sc_index,
> + l2_slice, l2_index);
> *cluster_offset = 0;
> break;
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2019-11-04 15:00 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-26 21:25 [RFC PATCH v2 00/26] Add subcluster allocation to qcow2 Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 01/26] qcow2: Add calculate_l2_meta() Alberto Garcia
2019-10-28 12:50 ` Vladimir Sementsov-Ogievskiy
2019-10-30 15:56 ` Alberto Garcia
2019-10-30 12:04 ` Max Reitz
2019-10-30 16:02 ` Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 02/26] qcow2: Split cluster_needs_cow() out of count_cow_clusters() Alberto Garcia
2019-10-28 13:55 ` Vladimir Sementsov-Ogievskiy
2019-10-26 21:25 ` [RFC PATCH v2 03/26] qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC clusters in handle_copied() Alberto Garcia
2019-10-30 14:24 ` Max Reitz
2019-11-13 14:25 ` Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 04/26] qcow2: Add get_l2_entry() and set_l2_entry() Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 05/26] qcow2: Document the Extended L2 Entries feature Alberto Garcia
2019-10-30 16:23 ` Max Reitz
2019-10-30 22:38 ` Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 06/26] qcow2: Add dummy has_subclusters() function Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 07/26] qcow2: Add subcluster-related fields to BDRVQcow2State Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 08/26] qcow2: Add offset_to_sc_index() Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 09/26] qcow2: Add l2_entry_size() Alberto Garcia
2019-10-30 16:47 ` Max Reitz
2019-10-26 21:25 ` [RFC PATCH v2 10/26] qcow2: Update get/set_l2_entry() and add get/set_l2_bitmap() Alberto Garcia
2019-10-30 16:55 ` Max Reitz
2019-11-14 13:57 ` Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 11/26] qcow2: Add qcow2_get_subcluster_type() Alberto Garcia
2019-11-04 12:35 ` Max Reitz
2019-11-04 15:01 ` Max Reitz
2019-10-26 21:25 ` [RFC PATCH v2 12/26] qcow2: Handle QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER Alberto Garcia
2019-11-04 12:57 ` Max Reitz
2019-11-04 13:03 ` Alberto Garcia
2019-11-04 13:10 ` Max Reitz
2019-11-07 14:44 ` Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 13/26] qcow2: Add subcluster support to calculate_l2_meta() Alberto Garcia
2019-11-04 14:21 ` Max Reitz
2019-11-08 15:18 ` Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 14/26] qcow2: Add subcluster support to qcow2_get_cluster_offset() Alberto Garcia
2019-11-04 14:58 ` Max Reitz [this message]
2019-11-08 15:42 ` Alberto Garcia
2019-11-11 8:42 ` Max Reitz
2019-10-26 21:25 ` [RFC PATCH v2 15/26] qcow2: Add subcluster support to zero_in_l2_slice() Alberto Garcia
2019-11-04 15:04 ` Max Reitz
2019-11-04 15:10 ` Max Reitz
2019-11-14 15:31 ` Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 16/26] qcow2: Add subcluster support to discard_in_l2_slice() Alberto Garcia
2019-11-04 15:07 ` Max Reitz
2019-11-14 15:33 ` Alberto Garcia
2019-11-14 16:22 ` Max Reitz
2019-10-26 21:25 ` [RFC PATCH v2 17/26] qcow2: Add subcluster support to check_refcounts_l2() Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 18/26] qcow2: Add subcluster support to expand_zero_clusters_in_l1() Alberto Garcia
2019-11-05 11:05 ` Max Reitz
2019-11-14 15:43 ` Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 19/26] qcow2: Fix offset calculation in handle_dependencies() Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 20/26] qcow2: Update L2 bitmap in qcow2_alloc_cluster_link_l2() Alberto Garcia
2019-11-05 11:43 ` Max Reitz
2019-11-14 16:30 ` Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 21/26] qcow2: Clear the L2 bitmap when allocating a compressed cluster Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 22/26] qcow2: Add subcluster support to handle_alloc_space() Alberto Garcia
2019-11-05 12:05 ` Max Reitz
2019-10-26 21:25 ` [RFC PATCH v2 23/26] qcow2: Restrict qcow2_co_pwrite_zeroes() to full clusters only Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 24/26] qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit Alberto Garcia
2019-11-05 12:47 ` Max Reitz
2019-11-15 13:35 ` Alberto Garcia
2019-10-26 21:25 ` [RFC PATCH v2 25/26] qcow2: Allow preallocation and backing files if extended_l2 is set Alberto Garcia
2019-11-05 13:11 ` Max Reitz
2019-10-26 21:25 ` [RFC PATCH v2 26/26] iotests: Add tests for qcow2 images with extended L2 entries Alberto Garcia
2019-11-05 13:32 ` [RFC PATCH v2 00/26] Add subcluster allocation to qcow2 Max Reitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=673d72da-bf8c-3ffb-a324-79e93f88a140@redhat.com \
--to=mreitz@redhat.com \
--cc=anton.nefedov@virtuozzo.com \
--cc=berto@igalia.com \
--cc=den@openvz.org \
--cc=kwolf@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).