Re: [PATCH v4 20/30] qcow2: Add subcluster support to discard_in_l2_slice()

[Vladimir Sementsov-Ogievskiy]

17.03.2020 21:16, Alberto Garcia wrote:
> Two changes are needed in this function:
>
> 1) A full discard deallocates a cluster so we can skip the operation if
>     it is already unallocated. With extended L2 entries however if any
>     of the subclusters has the 'all zeroes' bit set then we have to
>     clear it.
>
> 2) Setting the QCOW_OFLAG_ZERO bit of the L2 entry is forbidden if an
>     image has extended L2 entries. Instead, the individual 'all zeroes'
>     bits must be used.
>
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>   block/qcow2-cluster.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 746006a117..824c710760 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1790,12 +1790,20 @@ static int discard_in_l2_slice(BlockDriverState *bs, 
> uint64_t offset,
>            * TODO We might want to use bdrv_block_status(bs) here, but we're
>            * holding s->lock, so that doesn't work today.
>            *
> -         * If full_discard is true, the sector should not read back as zeroes,
> +         * If full_discard is true, the cluster should not read back as zeroes,
>            * but rather fall through to the backing file.
>            */
>           switch (qcow2_get_cluster_type(bs, old_l2_entry)) {
>           case QCOW2_CLUSTER_UNALLOCATED:
> -            if (full_discard || !bs->backing) {
> +            if (full_discard) {
> +                /* If the image has extended L2 entries we can only
> +                 * skip this operation if the L2 bitmap is zero. */
> +                uint64_t bitmap = has_subclusters(s) ?
> +                    get_l2_bitmap(s, l2_slice, l2_index + i) : 0;
> +                if (bitmap == 0) {
> +                    continue;
> +                }
> +            } else if (!bs->backing) {
>                   continue;
>               }

Hmm, so you do continue if full_discard is false AND bitmap != 0 & !bs->backing,
but you do not continue if full_discard is true AND bitmap != 0 & !bs->backing (as you 
will not go to "else") branch.

Seems it's a mistake.

I think, correct condition is

if (!bs->backing || full_discard && !get_l2_bitmap(s, l2_slice, l2_index + i))

, but, for doing so we also need


--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -565,6 +565,7 @@ static inline uint64_t get_l2_entry(BDRVQcow2State *s, 
uint64_t *l2_slice,
     return be64_to_cpu(l2_slice[idx]);
 }

+/* Return l2-entry bitmap if image has subclusters and 0 otherwise. */
 static inline uint64_t get_l2_bitmap(BDRVQcow2State *s, uint64_t *l2_slice,
                                      int idx)
 {
@@ -572,7 +573,6 @@ static inline uint64_t get_l2_bitmap(BDRVQcow2State *s, 
uint64_t *l2_slice,
         idx *= l2_entry_size(s) / sizeof(uint64_t);
         return be64_to_cpu(l2_slice[idx + 1]);
     } else {
-        /* For convenience only; the caller should ignore this value. */
         return 0;
     }
 }

or if you don't want, keep it explicit

if (!bs->backing || full_discard && (!has_subclusters(s) || !get_l2_bitmap(s, 
l2_slice, l2_index + i)))


=====

In case QCOW2_CLUSTER_ZERO_PLAIN, worth assert !has_subclusters(s) or mark 
image corrupted ?

>               break;
> @@ -1817,7 +1825,11 @@ static int discard_in_l2_slice(BlockDriverState *bs, 
> uint64_t offset,
>   
>           /* First remove L2 entries */
>           qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
> -        if (!full_discard && s->qcow_version >= 3) {
> +        if (has_subclusters(s)) {
> +            set_l2_entry(s, l2_slice, l2_index + i, 0);
> +            set_l2_bitmap(s, l2_slice, l2_index + i,
> +                          full_discard ? 0 : QCOW_L2_BITMAP_ALL_ZEROES);
> +        } else if (!full_discard && s->qcow_version >= 3) {
>               set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_ZERO);
>           } else {
>               set_l2_entry(s, l2_slice, l2_index + i, 0);


--
Best regards,
Vladimir