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

[Jean-Louis Dupond <jean-louis@dupond.be>]

Hi,

I hope this patchset can get merged soon, as it contains some good 
improvements.

Next to that, the change below only improves the performance on 
discards? It's not that something is broken/can cause issues in the 
current code?
Otherwise it might be a good idea to have this one merged as soon as 
possible.

Thanks for the work on this!

Jean-Louis

On 9/13/24 18:39, 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>
> Reviewed-by: Alexander Ivanov <alexander.ivanov@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);
>               }
>           }
>       }