Re: [PATCH v2 03/11] qcow2: put discard requests in the common queue when discard-no-unref enabled

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

On 5/13/24 08:31, Andrey Drobyshev wrote:
> Normally discard requests are stored in the queue attached to BDRVQcow2State
> to be processed later at once.  Currently discard-no-unref option handling
> causes these requests to be processed straight away.  Let's fix that.
>
> Note that when doing regular discards qcow2_free_any_cluster() would check
> for the presence of external data files for us and redirect request to
> underlying data_file.  Here we want to do the same but avoid refcount updates,
> thus we perform the same checks.
>
> Suggested-by: Hanna Czenczek <hreitz@redhat.com>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   block/qcow2-cluster.c | 39 +++++++++++++++++++++++++++++----------
>   1 file changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 5f057ba2fd..7dff0bd5a1 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1893,6 +1893,28 @@ again:
>       return 0;
>   }
>   
> +/*
> + * Helper for adding a discard request to the queue without any refcount
> + * modifications.  If external data file is used redirects the request to
> + * the corresponding BdrvChild.
> + */
> +static inline void
> +discard_no_unref_any_file(BlockDriverState *bs, uint64_t offset,
> +                          uint64_t length, QCow2ClusterType ctype,
> +                          enum qcow2_discard_type dtype)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +
> +    if (s->discard_passthrough[dtype] &&
> +        (ctype == QCOW2_CLUSTER_NORMAL || ctype == QCOW2_CLUSTER_ZERO_ALLOC)) {
> +        if (has_data_file(bs)) {
> +            bdrv_pdiscard(s->data_file, offset, length);
> +        } else {
> +            qcow2_queue_discard(bs, offset, length);
> +        }
> +    }
> +}
> +
>   /*
>    * This discards as many clusters of nb_clusters as possible at once (i.e.
>    * all clusters in the same L2 slice) and returns the number of discarded
> @@ -1974,12 +1996,10 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters,
>           if (!keep_reference) {
>               /* Then decrease the refcount */
>               qcow2_free_any_cluster(bs, old_l2_entry, type);
> -        } else if (s->discard_passthrough[type] &&
> -                   (cluster_type == QCOW2_CLUSTER_NORMAL ||
> -                    cluster_type == QCOW2_CLUSTER_ZERO_ALLOC)) {
> +        } else {
>               /* If we keep the reference, pass on the discard still */
> -            bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK,
> -                          s->cluster_size);
> +            discard_no_unref_any_file(bs, old_l2_entry & L2E_OFFSET_MASK,
> +                                      s->cluster_size, cluster_type, type);
>           }
>       }
>   
> @@ -2088,12 +2108,11 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
>               if (!keep_reference) {
>                   /* Then decrease the refcount */
>                   qcow2_free_any_cluster(bs, old_l2_entry, QCOW2_DISCARD_REQUEST);
> -            } else if (s->discard_passthrough[QCOW2_DISCARD_REQUEST] &&
> -                       (type == QCOW2_CLUSTER_NORMAL ||
> -                        type == QCOW2_CLUSTER_ZERO_ALLOC)) {
> +            } else {
>                   /* If we keep the reference, pass on the discard still */
> -                bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK,
> -                            s->cluster_size);
> +                discard_no_unref_any_file(bs, old_l2_entry & L2E_OFFSET_MASK,
> +                                          s->cluster_size, type,
> +                                          QCOW2_DISCARD_REQUEST);
>               }
>           }
>       }
Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

-- 
Best regards,
Alexander Ivanov