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

[Max Reitz]

On 17.03.20 19: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;

Isn’t this bitmap only valid for standard clusters?  In this case, the
whole cluster is unallocated, so the bitmap shouldn’t be relevant, AFAIU.

Max

> +                if (bitmap == 0) {
> +                    continue;
> +                }
> +            } else if (!bs->backing) {
>                  continue;
>              }
>              break;

signature.asc
Description: OpenPGP digital signature