Re: [Devel] [PATCH VZ10 v2 03/10] drivers/md/dm-qcow2: update metadata on whole cluster discard

[Pavel Tikhomirov <ptikhomirov@virtuozzo.com>]

On 8/12/26 22:19, Andrey Zhadchenko wrote:
> Discard of a mapped cluster used to only punch a hole in the image
> file: the L2 entry and refcounts were left untouched, so the cluster
> remained allocated in qcow2 metadata forever.
> 
> Handle discards covering a whole cluster via the L1/L2 entry replace
> machinery: prepare_cluster_discard() locks the L2 entry, then
> issue_discard() punches the data cluster out of the image file and
> queues the qio to write the zeroed L2 entry (and extended L2 bitmap)
> in process_indexes_write(). Refcounts of the discarded cluster are
> decremented in process_indexes_end() after the L2 writeback, like COW
> does with its source clusters. Clusters shared with internal
> snapshots or holding compressed data are not punched: only their
> usage count is decremented.
> 
> Also teach revert_clusters_alloc() that a changed index may contain

revert_clusters_alloc is missing at this point, previous patches rename it.

> a discard-cleared entry, which has no allocation to revert.
> 
> Partial cluster discards keep the previous behavior: punch a hole
> without touching metadata.
> 
> Feature: dm-qcow2: block device over QCOW2 files driver
> https://virtuozzo.atlassian.net/browse/VSTOR-139406
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
> ---
>  drivers/md/dm-qcow2-map.c | 110 +++++++++++++++++++++++++++++++++++---
>  drivers/md/dm-qcow2.h     |   2 +-
>  2 files changed, 104 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index 08d46eb177dd1..8d2c4e78a4c55 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -101,6 +101,12 @@ static loff_t bio_sector_to_file_pos(struct qcow2 *qcow2, struct qio *qio,
>  	return map->data_clu_pos + bytes_off_in_cluster(qcow2, qio);
>  }
>  
> +static bool qio_covers_full_clu(struct qcow2 *qcow2, struct qio *qio)
> +{
> +	return bytes_off_in_cluster(qcow2, qio) == 0 &&
> +	       qio->bi_iter.bi_size == qcow2->clu_size;
> +}
> +
>  static loff_t compressed_clu_end_pos(loff_t start, sector_t compressed_sectors)
>  {
>  	if (start % SECTOR_SIZE == 0)
> @@ -1324,6 +1330,8 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>  		set_u64_to_be_page(wbd->md->page, i, 0);
>  		if (skip_odd && (i & 1))
>  			continue; /* pos contains ext_l2 part of L2 entry */
> +		if (!pos)
> +			continue; /* no cluster was allocated */
>  
>  		spin_unlock(&qcow2->md_pages_lock);
>  		pos &= ~LX_REFCOUNT_EXACTLY_ONE;
> @@ -2015,7 +2023,12 @@ static int parse_metadata(struct qcow2 *qcow2, struct qio **qio,
>  		return ret;
>  
>  	map->data_clu_pos = pos;
> -	if (!write || !map->clu_is_cow)
> +	if (!write)
> +		return 0;
> +
> +	/* discards also need to update r1r2 */
> +	if (!map->clu_is_cow &&
> +	    !(op_is_discard((*qio)->bi_op) && qio_covers_full_clu(qcow2, *qio)))
>  		return 0;
>  
>  	/* Now refcounters table/block */
> @@ -3317,12 +3330,86 @@ static void issue_discard(struct qcow2_map *map, struct qio *qio)
>  	int ret;
>  
>  	WARN_ON_ONCE(!(map->level & L2_LEVEL));
> -	pos = bio_sector_to_file_pos(qcow2, qio, map);
> -	ret = qcow2_punch_hole(qcow2->file, pos, qio->bi_iter.bi_size);
>  
> -	if (ret)
> -		qio->bi_status = errno_to_blk_status(ret);
> -	qio_endio(qio);
> +	if (!map->clu_is_cow) {
> +		pos = bio_sector_to_file_pos(qcow2, qio, map);
> +		ret = qcow2_punch_hole(qcow2->file, pos, qio->bi_iter.bi_size);
> +
> +		if (ret) {
> +			qio->bi_status = errno_to_blk_status(ret);
> +			qio_endio(qio);
> +			return;
> +		}
> +	}
> +
> +	/* Clear metadata if needed */
> +	if (qio->flags & QIO_IS_DISCARD_FL) {
> +		qio->queue_list_id = QLIST_INDEXES_WRITE;
> +		qcow2_dispatch_qios(qcow2, qio, NULL);
> +	} else {
> +		qio_endio(qio);
> +	}
> +}
> +
> +static bool qio_discard_updates_metadata(struct qcow2 *qcow2, struct qio *qio,
> +					 struct qcow2_map *map)
> +{
> +	if (!(map->level & L2_LEVEL) || !map->data_clu_alloced)
> +		return false;
> +	if (qio_covers_full_clu(qcow2, qio))
> +		return true;
> +	return false;
> +}
> +
> +/*
> + * Discard changes metadata: prepare L2 entry update. It gets locked
> + * here, new value is written in process_indexes_write(), and
> + * refcounts are handled after the L2 writeback in process_indexes_end().
> + *
> + * Discard covering the whole cluster replaces the entry and unuses
> + * the discarded (or COW source) cluster.
> + */
> +static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
> +				   struct qcow2_map *map)
> +{
> +	u32 index_in_page = map->l2.index_in_page;
> +	struct md_page *md = map->l2.md;
> +	loff_t unuse_pos, unuse_end;
> +	struct qio_ext *ext;
> +	int ret;
> +
> +	WARN_ON_ONCE(!(map->level & L2_LEVEL) || !map->data_clu_alloced);
> +
> +	spin_lock_irq(&qcow2->md_pages_lock);
> +	if (delay_if_dirty(qcow2, md, index_in_page, qio) ||
> +	    __delay_if_writeback(qcow2, md, index_in_page, qio, true) ||
> +	    (qcow2->ext_l2 &&
> +	     delay_if_dirty(qcow2, md, index_in_page + 1, qio))) {
> +		spin_unlock_irq(&qcow2->md_pages_lock);
> +		return 0;
> +	}
> +	spin_unlock_irq(&qcow2->md_pages_lock);
> +
> +	if (map->clu_is_cow) {
> +		/* Cluster is shared or compressed. Decrement refcount. */
> +		unuse_pos = map->cow_clu_pos;
> +		unuse_end = map->cow_clu_end;
> +	} else {
> +		/* Nobody else refers the cluster: unuse it after discard */
> +		unuse_pos = map->data_clu_pos;
> +		unuse_end = map->data_clu_pos + qcow2->clu_size;
> +	}
> +
> +	ret = prepare_l_entry_replace(qcow2, map, *qio, md, index_in_page,
> +				      unuse_pos, unuse_end, L2_LEVEL);
> +	if (ret <= 0)
> +		return ret;
> +
> +	ext = (*qio)->ext;
> +	ext->lx_md = md;
> +
> +	(*qio)->flags |= QIO_IS_DISCARD_FL;
> +	return 1;
>  }
>  
>  static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
> @@ -3344,6 +3431,9 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
>  		/* Nothing to COW or L1 is mapped exactly once */
>  		qio_endio(*qio);
>  		ret = 0;
> +	} else if (unlikely(op_is_discard((*qio)->bi_op)) &&
> +		   qio_discard_updates_metadata(qcow2, *qio, map)) {
> +		ret = prepare_cluster_discard(qcow2, qio, map);
>  	} else if (write &&
>  		   (!qio_is_fully_alloced(qcow2, *qio, map) || map->clu_is_cow)) {
>  		if (map->clu_is_cow) {
> @@ -3365,7 +3455,7 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
>  				qio_endio(*qio);
>  				ret = 0;
>  			}
> -			/* Otherwise issue_discard(). TODO: update L2 */
> +			/* Otherwise issue_discard(). */
>  		} else {
>  			/* Wants L1 or L2 entry allocation */
>  			ret = prepare_l1l2_allocation(qcow2, *qio, map);
> @@ -3479,6 +3569,12 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
>  
>  	write = op_is_write(qio->bi_op);
>  
> +	/* Discard with prepared metadata update, see prepare_cluster_discard() */
> +	if (unlikely(qio->flags & QIO_IS_DISCARD_FL)) {
> +		issue_discard(&map, qio);
> +		return;
> +	}
> +
>  	if (unlikely(map.compressed)) {
>  		/* Compressed qio never uses sub-clus */
>  		submit_read_compressed(&map, qio, write);
> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
> index 230e7a4a34e76..8a24e04130e4d 100644
> --- a/drivers/md/dm-qcow2.h
> +++ b/drivers/md/dm-qcow2.h
> @@ -340,7 +340,7 @@ struct qio {
>  	blk_status_t bi_status;
>  #define QIO_FREE_ON_ENDIO_FL	(1 << 0) /* Free this qio memory from qio_endio() */
>  #define QIO_IS_MERGE_FL		(1 << 3) /* This is service merge qio */
> -#define QIO_IS_DISCARD_FL	(1 << 4) /* This zeroes index on backward merge */
> +#define QIO_IS_DISCARD_FL	(1 << 4) /* This zeroes index (discard or backward merge) */
>  #define QIO_IS_L1COW_FL		(1 << 5) /* This qio only wants COW at L1 */
>  #define QIO_SPLIT_INHERITED_FLAGS (QIO_IS_DISCARD_FL)
>  	u8 flags;

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.