From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Tikhomirov Date: Wed, 19 Aug 2026 12:43:31 +0200 Subject: Re: [Devel] [PATCH VZ10 v2 03/10] drivers/md/dm-qcow2: update metadata on whole cluster discard In-Reply-To: <20260812202005.288994-4-andrey.zhadchenko@virtuozzo.com> References: <20260812202005.288994-1-andrey.zhadchenko@virtuozzo.com> <20260812202005.288994-4-andrey.zhadchenko@virtuozzo.com> Message-ID: <81b8eae2-9ef6-454d-a9e5-44151f4d025e@virtuozzo.com> List-Id: 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 > --- > 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.