Re: [PATCH v9 28/34] qcow2: Add subcluster support to qcow2_co_pwrite_zeroes()

2020-07-03 Thread Max Reitz 
On 03.07.20 00:40, Alberto Garcia wrote:
> On Thu 02 Jul 2020 04:28:57 PM CEST, Max Reitz wrote:
>>> +/* For full clusters use zero_in_l2_slice() instead */
>>> +assert(nb_subclusters > 0 && nb_subclusters < 
>>> s->subclusters_per_cluster);
>>> +assert(sc + nb_subclusters <= s->subclusters_per_cluster);
>>
>> Maybe we should also assert that @offset is aligned to the subcluster
>> size.
> 
> It doesn't hurt but the only caller already guarantees that already ...

Sure, but it also guarantees the rest of these conditions, doesn’t it? :)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v9 28/34] qcow2: Add subcluster support to qcow2_co_pwrite_zeroes()

2020-07-02 Thread Alberto Garcia 
On Thu 02 Jul 2020 04:28:57 PM CEST, Max Reitz wrote:
>> +/* For full clusters use zero_in_l2_slice() instead */
>> +assert(nb_subclusters > 0 && nb_subclusters < 
>> s->subclusters_per_cluster);
>> +assert(sc + nb_subclusters <= s->subclusters_per_cluster);
>
> Maybe we should also assert that @offset is aligned to the subcluster
> size.

It doesn't hurt but the only caller already guarantees that already ...

>> @@ -4367,12 +4367,13 @@ static int coroutine_fn 
>> qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>  uint64_t zero_start = QEMU_ALIGN_UP(old_length, s->cluster_size);
>
> Can we instead align this to just subclusters?

I think so, good catch.

Berto

Re: [PATCH v9 28/34] qcow2: Add subcluster support to qcow2_co_pwrite_zeroes()

2020-07-02 Thread Max Reitz 
On 28.06.20 13:02, Alberto Garcia wrote:
> This works now at the subcluster level and pwrite_zeroes_alignment is
> updated accordingly.
> 
> qcow2_cluster_zeroize() is turned into qcow2_subcluster_zeroize() with
> the following changes:
> 
>- The request can now be subcluster-aligned.
> 
>- The cluster-aligned body of the request is still zeroized using
>  zero_in_l2_slice() as before.
> 
>- The subcluster-aligned head and tail of the request are zeroized
>  with the new zero_l2_subclusters() function.
> 
> There is just one thing to take into account for a possible future
> improvement: compressed clusters cannot be partially zeroized so
> zero_l2_subclusters() on the head or the tail can return -ENOTSUP.
> This makes the caller repeat the *complete* request and write actual
> zeroes to disk. This is sub-optimal because
> 
>1) if the head area was compressed we would still be able to use
>   the fast path for the body and possibly the tail.
> 
>2) if the tail area was compressed we are writing zeroes to the
>   head and the body areas, which are already zeroized.
> 
> Signed-off-by: Alberto Garcia 
> Reviewed-by: Eric Blake 
> ---
>  block/qcow2.h |  4 +--
>  block/qcow2-cluster.c | 80 +++
>  block/qcow2.c | 27 ---
>  3 files changed, 90 insertions(+), 21 deletions(-)

[...]

> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index deff838fe8..1641976028 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -2015,12 +2015,58 @@ static int zero_in_l2_slice(BlockDriverState *bs, 
> uint64_t offset,
>  return nb_clusters;
>  }
>  
> -int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
> -  uint64_t bytes, int flags)
> +static int zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
> +   unsigned nb_subclusters)
> +{
> +BDRVQcow2State *s = bs->opaque;
> +uint64_t *l2_slice;
> +uint64_t old_l2_bitmap, l2_bitmap;
> +int l2_index, ret, sc = offset_to_sc_index(s, offset);
> +
> +/* For full clusters use zero_in_l2_slice() instead */
> +assert(nb_subclusters > 0 && nb_subclusters < 
> s->subclusters_per_cluster);
> +assert(sc + nb_subclusters <= s->subclusters_per_cluster);

Maybe we should also assert that @offset is aligned to the subcluster size.

[...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 86258fbc22..4edc3c72b9 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -4367,12 +4367,13 @@ static int coroutine_fn 
> qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>  uint64_t zero_start = QEMU_ALIGN_UP(old_length, s->cluster_size);

Can we instead align this to just subclusters?

>  
>  /*
> - * Use zero clusters as much as we can. qcow2_cluster_zeroize()
> + * Use zero clusters as much as we can. qcow2_subcluster_zeroize()
>   * requires a cluster-aligned start. The end may be unaligned if it 
> is

s/cluster/subcluster/?

Max

>   * at the end of the image (which it is here).
>   */
>  if (offset > zero_start) {
> -ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 
> 0);
> +ret = qcow2_subcluster_zeroize(bs, zero_start, offset - 
> zero_start,
> +   0);
>  if (ret < 0) {
>  error_setg_errno(errp, -ret, "Failed to zero out new 
> clusters");
>  goto fail;
> 



signature.asc
Description: OpenPGP digital signature

[PATCH v9 28/34] qcow2: Add subcluster support to qcow2_co_pwrite_zeroes()

2020-06-28 Thread Alberto Garcia 
This works now at the subcluster level and pwrite_zeroes_alignment is
updated accordingly.

qcow2_cluster_zeroize() is turned into qcow2_subcluster_zeroize() with
the following changes:

   - The request can now be subcluster-aligned.

   - The cluster-aligned body of the request is still zeroized using
 zero_in_l2_slice() as before.

   - The subcluster-aligned head and tail of the request are zeroized
 with the new zero_l2_subclusters() function.

There is just one thing to take into account for a possible future
improvement: compressed clusters cannot be partially zeroized so
zero_l2_subclusters() on the head or the tail can return -ENOTSUP.
This makes the caller repeat the *complete* request and write actual
zeroes to disk. This is sub-optimal because

   1) if the head area was compressed we would still be able to use
  the fast path for the body and possibly the tail.

   2) if the tail area was compressed we are writing zeroes to the
  head and the body areas, which are already zeroized.

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
---
 block/qcow2.h |  4 +--
 block/qcow2-cluster.c | 80 +++
 block/qcow2.c | 27 ---
 3 files changed, 90 insertions(+), 21 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 4fad40b96b..4ef4ae4ab0 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -898,8 +898,8 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, 
QCowL2Meta *m);
 int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
   uint64_t bytes, enum qcow2_discard_type type,
   bool full_discard);
-int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
-  uint64_t bytes, int flags);
+int qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset,
+ uint64_t bytes, int flags);
 
 int qcow2_expand_zero_clusters(BlockDriverState *bs,
BlockDriverAmendStatusCB *status_cb,
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index deff838fe8..1641976028 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -2015,12 +2015,58 @@ static int zero_in_l2_slice(BlockDriverState *bs, 
uint64_t offset,
 return nb_clusters;
 }
 
-int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
-  uint64_t bytes, int flags)
+static int zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
+   unsigned nb_subclusters)
+{
+BDRVQcow2State *s = bs->opaque;
+uint64_t *l2_slice;
+uint64_t old_l2_bitmap, l2_bitmap;
+int l2_index, ret, sc = offset_to_sc_index(s, offset);
+
+/* For full clusters use zero_in_l2_slice() instead */
+assert(nb_subclusters > 0 && nb_subclusters < s->subclusters_per_cluster);
+assert(sc + nb_subclusters <= s->subclusters_per_cluster);
+
+ret = get_cluster_table(bs, offset, _slice, _index);
+if (ret < 0) {
+return ret;
+}
+
+switch (qcow2_get_cluster_type(bs, get_l2_entry(s, l2_slice, l2_index))) {
+case QCOW2_CLUSTER_COMPRESSED:
+ret = -ENOTSUP; /* We cannot partially zeroize compressed clusters */
+goto out;
+case QCOW2_CLUSTER_NORMAL:
+case QCOW2_CLUSTER_UNALLOCATED:
+break;
+default:
+g_assert_not_reached();
+}
+
+old_l2_bitmap = l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index);
+
+l2_bitmap |=  QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
+l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
+
+if (old_l2_bitmap != l2_bitmap) {
+set_l2_bitmap(s, l2_slice, l2_index, l2_bitmap);
+qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
+}
+
+ret = 0;
+out:
+qcow2_cache_put(s->l2_table_cache, (void **) _slice);
+
+return ret;
+}
+
+int qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset,
+ uint64_t bytes, int flags)
 {
 BDRVQcow2State *s = bs->opaque;
 uint64_t end_offset = offset + bytes;
 uint64_t nb_clusters;
+unsigned head, tail;
 int64_t cleared;
 int ret;
 
@@ -2035,8 +2081,8 @@ int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t 
offset,
 }
 
 /* Caller must pass aligned values, except at image end */
-assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
-assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
+assert(offset_into_subcluster(s, offset) == 0);
+assert(offset_into_subcluster(s, end_offset) == 0 ||
end_offset >= bs->total_sectors << BDRV_SECTOR_BITS);
 
 /* The zero flag is only supported by version 3 and newer */
@@ -2044,11 +2090,26 @@ int qcow2_cluster_zeroize(BlockDriverState *bs, 
uint64_t offset,
 return -ENOTSUP;
 }
 
-/* Each L2 slice is handled by its own loop iteration */
-nb_clusters = size_to_clusters(s, bytes);
+head = MIN(end_offset,