[Devel] [PATCH VZ10 v2 00/10] dm-qcow2: improve discard and read-only merge handling

[Devel] [PATCH VZ10 v2 00/10] dm-qcow2: improve discard and read-only merge handling

[Andrey Zhadchenko]

This series makes discard update qcow2 metadata and permits backward
merge from read-only source images.

First patch improves revert_cluster_alloc: this function didn't expect
L2 entries to contain subcluster descriptions (when ext2_l2 is set).

The second patch renames COW machinery to a more general 'replace entry'.

Next five patches improve discard support in general: now it clears
metadata and have a subcluster granularity.

The next two patches support backward merge when the image we merge
from is read-only. Such merges now leave the source mappings and
metadata unchanged. Previously the module ignored file mode and
submitted writes anyway.

Last patch makes merge respect all metadata-zeroed blocks. For now it is
writing zeroes as data, but we will improve this to metadata when we
support REQ_OP_WRITE_ZEROES.


v2:
 - patch#6: expand qio_discard_updates_metadata() condition to return
true when backing file is present and fully covered sublcu zero bit isn't
already set (prepare_cluster_discard() already handled this case but
if forgot about it here)

Andrey Zhadchenko (10):
  drivers/md/dm-qcow2: fix revert_cluster_alloc() for ext_l2 case
  drivers/md/dm-qcow2: generalize COW index update machinery
  drivers/md/dm-qcow2: update metadata on whole cluster discard
  drivers/md/dm-qcow2: never trigger COW or allocation on discard
  drivers/md/dm-qcow2: set L2_READS_ALL_ZEROES after some discards
  drivers/md/dm-qcow2: update metadata on subclusters discard
  drivers/md/dm-qcow2: unmap cluster when discard clears last allocated
    subclusters
  drivers/md/dm-qcow2: do discards during backward merge only for
    writable image
  drivers/md/dm-qcow2: allow shared L1 entries during merge from RO
    image
  drivers/md/dm-qcow2: respect zeroes during merge

 drivers/md/dm-qcow2-cmd.c    |  21 +-
 drivers/md/dm-qcow2-map.c    | 390 ++++++++++++++++++++++++++++-------
 drivers/md/dm-qcow2-target.c |   2 +-
 drivers/md/dm-qcow2.h        |  19 +-
 4 files changed, 337 insertions(+), 95 deletions(-)

-- 
2.43.5

[Devel] [PATCH VZ10 v2 01/10] drivers/md/dm-qcow2: fix revert_cluster_alloc() for ext_l2 case

[Andrey Zhadchenko]

This function walks over all changed u64 values in md. With ext_l2
half of them holds subcluster description. Firstly it reverts these
values to saved pe_page, which is fine, but then it tries to revert
r1r2 changes. It makes no sense when the value is a subcluster
description.
Teach the function to skip subcluster descriptions: do it based on
a new lx_level in struct wb_desc. Add new argument to
prepare_l_entry_update() and set it there.
The warning also could have tripped for ext_l2 entries, so drop
it entirely.
The function effectively reverts not only cluster alloc, so rename
it to revert_l_entries_update() to mimic prepare_l_entries_update.

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    | 28 ++++++++++++++++++----------
 drivers/md/dm-qcow2-target.c |  2 +-
 drivers/md/dm-qcow2.h        |  1 +
 3 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index db21efb45e17a..2cd8e174049f8 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -1297,18 +1297,19 @@ static void do_md_page_read_complete(int ret, struct qcow2 *qcow2,
 }
 
 /* Be careful with dirty_or_writeback()/etc! Check races. */
-static void revert_clusters_alloc(struct qcow2 *qcow2, struct wb_desc *wbd)
+static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
 {
 	struct qcow2_map_item r1, r2;
 	struct page *pe_page;
+	bool skip_odd;
 	u64 pos, old;
 	int i, ret;
 
+	skip_odd = qcow2->ext_l2 && wbd->lx_level == L2_LEVEL;
+
 	lockdep_assert_held(&qcow2->md_pages_lock);
 	for_each_set_bit(i, wbd->changed_indexes, LX_INDEXES_PER_PAGE) {
 		pos = get_u64_from_be_page(wbd->md->page, i);
-		WARN_ON_ONCE(!(pos & ~LX_REFCOUNT_EXACTLY_ONE) ||
-			     !(pos & LX_REFCOUNT_EXACTLY_ONE));
 
 		/* Here we restore prealloced and compressed clu mappings */
 		pe_page = wbd->pe_page;
@@ -1321,6 +1322,9 @@ static void revert_clusters_alloc(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 */
+
 		spin_unlock(&qcow2->md_pages_lock);
 		pos &= ~LX_REFCOUNT_EXACTLY_ONE;
 
@@ -1369,7 +1373,7 @@ static void complete_wbd(struct qcow2 *qcow2, struct wb_desc *wbd)
 		unsigned long flags;
 
 		spin_lock_irqsave(&qcow2->md_pages_lock, flags);
-		revert_clusters_alloc(qcow2, wbd);
+		revert_l_entries_update(qcow2, wbd);
 		clear_writeback_status(qcow2, wbd->md, wbd->ret,
 				       &wait_list, &end_list);
 		spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
@@ -2433,7 +2437,7 @@ static loff_t allocate_cluster(struct qcow2 *qcow2, struct qio *qio,
 #define LU_IGN_CHANGED_IND	(1 << 3)
 static int prepare_l_entry_update(struct qcow2 *qcow2, struct qio *qio,
 				  struct md_page *md, u32 index_in_page,
-				  u64 *pval, u32 arg_mask)
+				  u64 *pval, u32 arg_mask, u8 lx_level)
 {
 	bool wants_pe_page = (arg_mask & LU_WANTS_PE_PAGE);
 	struct wb_desc *new_wbd = NULL;
@@ -2453,6 +2457,7 @@ static int prepare_l_entry_update(struct qcow2 *qcow2, struct qio *qio,
 		if (!new_wbd)
 			return -ENOMEM;
 		new_wbd->md = md;
+		new_wbd->lx_level = lx_level;
 	} else if (wants_pe_page && !md->wbd->pe_page) {
 		pe_page = alloc_page(GFP_NOIO|__GFP_ZERO);
 		if (!pe_page)
@@ -2515,7 +2520,8 @@ static int prepare_l1l2_allocation(struct qcow2 *qcow2, struct qio *qio,
 		/* Allocate cluster for L2 entries, and prepare L1 update */
 		ret = prepare_l_entry_update(qcow2, qio, map->l1.md,
 					     map->l1.index_in_page, &val,
-					     LU_SET_ONE_MASK|LU_WANTS_ALLOC);
+					     LU_SET_ONE_MASK|LU_WANTS_ALLOC,
+					     L1_LEVEL);
 		if (ret <= 0)
 			return ret;
 
@@ -2541,7 +2547,8 @@ static int prepare_l1l2_allocation(struct qcow2 *qcow2, struct qio *qio,
 
 		ret = prepare_l_entry_update(qcow2, qio, map->l2.md,
 					     map->l2.index_in_page,
-					     &map->data_clu_pos, arg_mask);
+					     &map->data_clu_pos, arg_mask,
+					     L2_LEVEL);
 		if (ret <= 0)
 			return ret;
 
@@ -2562,7 +2569,7 @@ static int prepare_l1l2_allocation(struct qcow2 *qcow2, struct qio *qio,
 
 	return prepare_l_entry_update(qcow2, qio, map->l2.md,
 				      map->l2.index_in_page + 1,
-				      &val, arg_mask);
+				      &val, arg_mask, L2_LEVEL);
 }
 
 /*
@@ -3975,7 +3982,7 @@ static void process_cow_indexes_write(struct qcow2 *qcow2,
 		ret = prepare_l_entry_update(qcow2, qio, lx_md,
 					     ext->lx_index_in_page,
 					     &ext->allocated_clu_pos,
-					     arg_mask);
+					     arg_mask, ext->cow_level);
 		if (ret < 0) {
 			qio->bi_status = errno_to_blk_status(ret);
 			qio_endio(qio);
@@ -3986,7 +3993,8 @@ static void process_cow_indexes_write(struct qcow2 *qcow2,
 			arg_mask &= ~LU_SET_ONE_MASK;
 			ret = prepare_l_entry_update(qcow2, qio, lx_md,
 					     ext->lx_index_in_page + 1,
-					   &ext->new_ext_l2, arg_mask);
+					     &ext->new_ext_l2, arg_mask,
+					     L2_LEVEL);
 			WARN_ON_ONCE(ret < 0);
 		}
 
diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index 3f65897ce9da2..0f1d3e3c5258e 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -174,7 +174,7 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
 	int i;
 
 	/*
-	 * We need second iteration, since revert_clusters_alloc()
+	 * We need second iteration, since revert_l_entries_update()
 	 * may start timer again after failed wb.
 	 */
 	for (i = 0; i < 2; i++) {
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index 86f0688e7345a..aa3487007523f 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -118,6 +118,7 @@ struct wb_desc {
 	struct list_head dependent_list;
 	int nr_submitted;
 	bool completed;
+	u8 lx_level;
 	int ret;
 };
 
-- 
2.43.5

[Devel] [PATCH VZ10 v2 02/10] drivers/md/dm-qcow2: generalize COW index update machinery

[Andrey Zhadchenko]

The two-stage L1/L2 entry update pipeline is not COW-specific:
backward merge already uses it to zero L2 entries, and upcoming
discard support will be the third user. Rename it to reflect
what it does.

No functional changes.

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 | 100 +++++++++++++++++++-------------------
 drivers/md/dm-qcow2.h     |  10 ++--
 2 files changed, 55 insertions(+), 55 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 2cd8e174049f8..08d46eb177dd1 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2579,10 +2579,10 @@ static int prepare_l1l2_allocation(struct qcow2 *qcow2, struct qio *qio,
  * we have to wait all previous READs. We do that around
  * index wb. See md->wpc_noread_count update details.
  */
-static int prepare_l_entry_cow(struct qcow2 *qcow2, struct qcow2_map *map,
-			       struct qio *qio, struct md_page *md,
-			       u32 index_in_page, loff_t cow_clu_pos,
-			       loff_t cow_clu_end, u8 cow_level)
+static int prepare_l_entry_replace(struct qcow2 *qcow2, struct qcow2_map *map,
+				   struct qio *qio, struct md_page *md,
+				   u32 index_in_page, loff_t unuse_clu_pos,
+				   loff_t unuse_clu_end, u8 lx_level)
 {
 	struct lock_desc *lockd = NULL;
 	struct qio_ext *ext;
@@ -2591,9 +2591,9 @@ static int prepare_l_entry_cow(struct qcow2 *qcow2, struct qcow2_map *map,
 		return -ENOMEM;
 
 	ext = qio->ext;
-	ext->cow_clu_pos = cow_clu_pos;
-	ext->cow_clu_end = cow_clu_end;
-	ext->cow_level = cow_level;
+	ext->unuse_clu_pos = unuse_clu_pos;
+	ext->unuse_clu_end = unuse_clu_end;
+	ext->lx_level = lx_level;
 
 	spin_lock_irq(&qcow2->md_pages_lock);
 	if (!md->lockd) {
@@ -2614,23 +2614,23 @@ static int prepare_l_entry_cow(struct qcow2 *qcow2, struct qcow2_map *map,
 	return 1;
 }
 
-static int prepare_l1l2_cow(struct qcow2 *qcow2, struct qio *qio,
-			    struct qcow2_map *map)
+static int prepare_l1l2_replace(struct qcow2 *qcow2, struct qio *qio,
+				struct qcow2_map *map)
 {
 	if (WARN_ON_ONCE(!(map->level & L1_LEVEL)))
 		return -EIO; /* Sanity check: L1 must be cached */
 
 	if (!(map->level & L2_LEVEL)) {
-		return prepare_l_entry_cow(qcow2, map, qio, map->l1.md,
-					   map->l1.index_in_page,
-					   map->cow_clu_pos,
-					   map->cow_clu_end, L1_LEVEL);
+		return prepare_l_entry_replace(qcow2, map, qio, map->l1.md,
+					       map->l1.index_in_page,
+					       map->cow_clu_pos,
+					       map->cow_clu_end, L1_LEVEL);
 	}
 
-	return prepare_l_entry_cow(qcow2, map, qio, map->l2.md,
-				  map->l2.index_in_page,
-				  map->cow_clu_pos,
-				  map->cow_clu_end, L2_LEVEL);
+	return prepare_l_entry_replace(qcow2, map, qio, map->l2.md,
+				       map->l2.index_in_page,
+				       map->cow_clu_pos,
+				       map->cow_clu_end, L2_LEVEL);
 }
 
 static void backward_merge_write_complete(struct qcow2_target *tgt, struct qio *unused,
@@ -2648,7 +2648,7 @@ static void backward_merge_write_complete(struct qcow2_target *tgt, struct qio *
 	WARN_ON_ONCE(qio->flags & QIO_IS_DISCARD_FL);
 	qio->flags |= QIO_IS_DISCARD_FL;
 
-	qio->queue_list_id = QLIST_COW_INDEXES;
+	qio->queue_list_id = QLIST_INDEXES_WRITE;
 	qcow2_dispatch_qios(qcow2, qio, NULL);
 }
 
@@ -2704,7 +2704,7 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
 	if (!op_is_write((*qio)->bi_op)) {
 		/*
 		 * READ qio may data may be contained in several deltas.
-		 * We can't read lower delta after prepare_l1l2_cow()
+		 * We can't read lower delta after prepare_l1l2_replace()
 		 * prepares us.
 		 */
 		aux_qio = qcow2_alloc_qio(qcow2->tgt->qio_pool, true);
@@ -2725,10 +2725,10 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
 	}
 
 	/*
-	 * Mark as COW, as this completely defers any parallel qios.
-	 * @qio is COW status holder.
+	 * Lock the entry, as this completely defers any parallel qios.
+	 * @qio is the lock holder.
 	 */
-	ret = prepare_l1l2_cow(qcow2, *qio, map);
+	ret = prepare_l1l2_replace(qcow2, *qio, map);
 	if (ret < 0) {
 		(*qio)->bi_status = errno_to_blk_status(ret);
 		goto endio;
@@ -2736,13 +2736,13 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
 
 	if (!map->clu_is_cow) {
 		/* Forced set these to unuse them after discard */
-		(*qio)->ext->cow_clu_pos = map->data_clu_pos;
-		(*qio)->ext->cow_clu_end = map->data_clu_pos + qcow2->clu_size;
+		(*qio)->ext->unuse_clu_pos = map->data_clu_pos;
+		(*qio)->ext->unuse_clu_end = map->data_clu_pos + qcow2->clu_size;
 	}
 
 	return 1;
 endio:
-	qio_endio(*qio); /* Breaks COW set in prepare_l1l2_cow() */
+	qio_endio(*qio); /* Releases the lock set in prepare_l1l2_replace() */
 	return 0;
 }
 
@@ -3348,7 +3348,7 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
 		   (!qio_is_fully_alloced(qcow2, *qio, map) || map->clu_is_cow)) {
 		if (map->clu_is_cow) {
 			/* COW to compressed or shared with snapshot cluster */
-			ret = prepare_l1l2_cow(qcow2, *qio, map);
+			ret = prepare_l1l2_replace(qcow2, *qio, map);
 		} else if ((map->level & L2_LEVEL) &&
 		    qio_border_is_inside_unmapped_unit(qcow2, *qio, map) &&
 		    maybe_mapped_in_lower_delta(qcow2, *qio)) {
@@ -3358,7 +3358,7 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
 			 * snapshots). Here is data COW on L2_LEVEL.
 			 */
 			map->backing_file_cow = true;
-			ret = prepare_l1l2_cow(qcow2, *qio, map);
+			ret = prepare_l1l2_replace(qcow2, *qio, map);
 		} else if (unlikely(op_is_discard((*qio)->bi_op) &&
 				    (map->level & L2_LEVEL))) {
 			if (!map->data_clu_alloced) {
@@ -3837,7 +3837,7 @@ static void cow_data_write_endio(struct qcow2_target *tgt, struct qio *unused,
 		qio->bi_status = bi_status;
 		qio_endio(qio);
 	} else {
-		qio->queue_list_id = QLIST_COW_INDEXES;
+		qio->queue_list_id = QLIST_INDEXES_WRITE;
 		qcow2_dispatch_qios(qcow2, qio, NULL);
 	}
 }
@@ -3885,7 +3885,7 @@ static void sliced_cow_data_write_complete(struct qcow2_target *tgt, struct qio
 		qio->bi_status = bi_status;
 		qio_endio(qio);
 	} else {
-		qio->queue_list_id = QLIST_COW_INDEXES;
+		qio->queue_list_id = QLIST_INDEXES_WRITE;
 		qcow2_dispatch_qios(qcow2, qio, NULL);
 	}
 }
@@ -3919,7 +3919,7 @@ static void process_cow_data_write(struct qcow2 *qcow2, struct list_head *cow_li
 		ext = qio->ext;
 
 		if (ext->only_set_ext_l2) {
-			WARN_ON_ONCE(ext->cow_level != L2_LEVEL);
+			WARN_ON_ONCE(ext->lx_level != L2_LEVEL);
 			pos = ext->allocated_clu_pos;
 			goto submit;
 		}
@@ -3938,14 +3938,14 @@ static void process_cow_data_write(struct qcow2 *qcow2, struct list_head *cow_li
 		ext->allocated_clu_pos = pos;
 		ext->cleanup_mask |= FREE_ALLOCATED_CLU;
 submit:
-		if (ext->cow_level == L2_LEVEL)
+		if (ext->lx_level == L2_LEVEL)
 			submit_sliced_cow_data_write(qcow2, qio, pos);
 		else
 			submit_cow_data_write(qcow2, qio, pos);
 	}
 }
 
-static void process_cow_indexes_write(struct qcow2 *qcow2,
+static void process_indexes_write(struct qcow2 *qcow2,
 				      struct list_head *qio_list)
 {
 	struct qcow2_bvec *qvec;
@@ -3965,7 +3965,7 @@ static void process_cow_indexes_write(struct qcow2 *qcow2,
 		lx_md = ext->lx_md;
 
 		/* Return back to the same stage in case of writeback */
-		qio->queue_list_id = QLIST_COW_INDEXES;
+		qio->queue_list_id = QLIST_INDEXES_WRITE;
 		if (delay_if_writeback(qcow2, lx_md, -1, &qio, true))
 			continue;
 
@@ -3974,7 +3974,7 @@ static void process_cow_indexes_write(struct qcow2 *qcow2,
 
 		arg_mask = (discard ? 0 : LU_SET_ONE_MASK) | LU_WANTS_PE_PAGE;
 		if (ext->only_set_ext_l2) {
-			WARN_ON_ONCE(ext->cow_level != L2_LEVEL);
+			WARN_ON_ONCE(ext->lx_level != L2_LEVEL);
 			goto set_ext_l2;
 		}
 
@@ -3982,14 +3982,14 @@ static void process_cow_indexes_write(struct qcow2 *qcow2,
 		ret = prepare_l_entry_update(qcow2, qio, lx_md,
 					     ext->lx_index_in_page,
 					     &ext->allocated_clu_pos,
-					     arg_mask, ext->cow_level);
+					     arg_mask, ext->lx_level);
 		if (ret < 0) {
 			qio->bi_status = errno_to_blk_status(ret);
 			qio_endio(qio);
 			continue;
 		}
 set_ext_l2:
-		if (qcow2->ext_l2 && ext->cow_level == L2_LEVEL) {
+		if (qcow2->ext_l2 && ext->lx_level == L2_LEVEL) {
 			arg_mask &= ~LU_SET_ONE_MASK;
 			ret = prepare_l_entry_update(qcow2, qio, lx_md,
 					     ext->lx_index_in_page + 1,
@@ -3999,7 +3999,7 @@ static void process_cow_indexes_write(struct qcow2 *qcow2,
 		}
 
 		/* Next stage */
-		qio->queue_list_id = QLIST_COW_END;
+		qio->queue_list_id = QLIST_INDEXES_END;
 
 		spin_lock_irq(&qcow2->md_pages_lock);
 		/*
@@ -4015,8 +4015,8 @@ static void process_cow_indexes_write(struct qcow2 *qcow2,
 	}
 }
 
-/* Finalize successful COW */
-static void process_cow_end(struct qcow2 *qcow2, struct list_head *qio_list)
+/* Finalize successful L1/L2 entry replace */
+static void process_indexes_end(struct qcow2 *qcow2, struct list_head *qio_list)
 {
 	struct dm_target *ti = qcow2->tgt->ti;
 	u32 mask, clu_size = qcow2->clu_size;
@@ -4036,7 +4036,7 @@ next:		qio = qio_list_pop(qio_list);
 			ext->cleanup_mask &= ~FREE_ALLOCATED_CLU;
 
 		/* Should be already set... */
-		qio->queue_list_id = QLIST_COW_END;
+		qio->queue_list_id = QLIST_INDEXES_END;
 		/*
 		 * Wait last user before we (possible) mark clusters
 		 * unused. In real only compressed COW requires this.
@@ -4044,8 +4044,8 @@ next:		qio = qio_list_pop(qio_list);
 		if (delay_if_has_wpc_readers(qcow2, ext->lx_md, &qio))
 			goto next;
 
-		pos = ext->cow_clu_pos;
-		for (; pos < ext->cow_clu_end; pos += clu_size) {
+		pos = ext->unuse_clu_pos;
+		for (; pos < ext->unuse_clu_end; pos += clu_size) {
 			ret = __handle_r1r2_maps(qcow2, pos, &qio, &r1, &r2);
 			if (ret == 0) /* We never shrink md pages, impossible */
 				goto next;
@@ -4053,7 +4053,7 @@ next:		qio = qio_list_pop(qio_list);
 				QC_ERR(ti, "clu at %lld leaked", pos);
 			else
 				dec_cluster_usage(qcow2, r2.md, r2.index_in_page, pos);
-			ext->cow_clu_pos += clu_size;
+			ext->unuse_clu_pos += clu_size;
 		}
 
 		mask = MD_INDEX_SET_UNLOCKED|DEC_WPC_NOREAD_COUNT;
@@ -4061,7 +4061,7 @@ next:		qio = qio_list_pop(qio_list);
 			mask |= FREE_QIO_DATA_QVEC;
 		WARN_ON_ONCE(ext->cleanup_mask != mask); /* Sanity check */
 
-		if (ext->cow_level == L1_LEVEL) {
+		if (ext->lx_level == L1_LEVEL) {
 			finalize_qio_ext(qio);
 			/* COW on L1 completed, it's time for COW on L2 */
 			qio->queue_list_id = QLIST_DEFERRED;
@@ -4101,8 +4101,8 @@ void do_qcow2_work(struct work_struct *ws)
 	LIST_HEAD(zread_qios);
 	LIST_HEAD(bwrite_qios);
 	LIST_HEAD(cow_data_qios);
-	LIST_HEAD(cow_indexes_qios);
-	LIST_HEAD(cow_end_qios);
+	LIST_HEAD(indexes_write_qios);
+	LIST_HEAD(indexes_end_qios);
 	LIST_HEAD(resubmit_qios);
 	LIST_HEAD(seek_qios);
 	unsigned int pflags = current->flags;
@@ -4114,8 +4114,8 @@ void do_qcow2_work(struct work_struct *ws)
 	list_splice_init(&qcow2->qios[QLIST_ZREAD], &zread_qios);
 	list_splice_init(&qcow2->qios[QLIST_BMERGE_WRITE], &bwrite_qios);
 	list_splice_init(&qcow2->qios[QLIST_COW_DATA], &cow_data_qios);
-	list_splice_init(&qcow2->qios[QLIST_COW_INDEXES], &cow_indexes_qios);
-	list_splice_init(&qcow2->qios[QLIST_COW_END], &cow_end_qios);
+	list_splice_init(&qcow2->qios[QLIST_INDEXES_WRITE], &indexes_write_qios);
+	list_splice_init(&qcow2->qios[QLIST_INDEXES_END], &indexes_end_qios);
 	list_splice_init(&qcow2->resubmit_qios, &resubmit_qios);
 	list_splice_init(&qcow2->qios[QLIST_SEEK], &seek_qios);
 	spin_unlock_irq(&qcow2->deferred_lock);
@@ -4125,8 +4125,8 @@ void do_qcow2_work(struct work_struct *ws)
 	process_compressed_read(&zread_qios, &cow_data_qios);
 	process_backward_merge_write(qcow2, &bwrite_qios);
 	process_cow_data_write(qcow2, &cow_data_qios);
-	process_cow_indexes_write(qcow2, &cow_indexes_qios);
-	process_cow_end(qcow2, &cow_end_qios);
+	process_indexes_write(qcow2, &indexes_write_qios);
+	process_indexes_end(qcow2, &indexes_end_qios);
 	process_resubmit_qios(qcow2, &resubmit_qios);
 	process_seek_qios(qcow2, &seek_qios);
 
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index aa3487007523f..230e7a4a34e76 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -228,8 +228,8 @@ enum {
 	QLIST_ZREAD,
 	QLIST_BMERGE_WRITE,
 	QLIST_COW_DATA,
-	QLIST_COW_INDEXES,
-	QLIST_COW_END,
+	QLIST_INDEXES_WRITE,
+	QLIST_INDEXES_END,
 	QLIST_SEEK,
 
 	QLIST_COUNT,
@@ -307,13 +307,13 @@ struct qio_ext {
 	u32 lx_index_in_page, r2_index_in_page;
 	u64 allocated_clu_pos;
 
-	loff_t cow_clu_pos;
-	loff_t cow_clu_end;
+	loff_t unuse_clu_pos;
+	loff_t unuse_clu_end;
 	u64 new_ext_l2;
 	u32 cow_mask;
 	bool only_set_ext_l2:1;
 
-	u8 cow_level;
+	u8 lx_level;
 
 #define MD_INDEX_SET_UNLOCKED	(1ULL << 0)
 #define DEC_WPC_NOREAD_COUNT	(1ULL << 1)
-- 
2.43.5

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

[Andrey Zhadchenko]

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
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;
-- 
2.43.5

[Devel] [PATCH VZ10 v2 04/10] drivers/md/dm-qcow2: never trigger COW or allocation on discard

[Andrey Zhadchenko]

In handle_metadata() the partial cluster discard fallback is checked
only after the COW branches. As a result a discard, whose range
touches a shared (snapshot or compressed) cluster or crosses an
unmapped unit border over a backing file, is routed into the data COW
machinery. COW then tries to submit the discard payload as a data
write, but discard qios carry no bvec array: the iov_iter is built
from garbage, which triggers the WARN_ON() in __submit_rw_mapped()
and writes random kernel memory into the COW'ed cluster.

Check for discard before the COW branches: discard is advisory and
carries no data, so it must never COW or allocate. End discard
early while handling metadata if possible. Otherwise do discard
operations before COWs.

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 | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 8d2c4e78a4c55..dcc65735881d4 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -3431,9 +3431,18 @@ 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 (unlikely(op_is_discard((*qio)->bi_op))) {
+		/*
+		 * Discard carries no data and advisory. If it does not
+		 * trigger metadata changes or to-be-discarded cluster
+		 * is not present, end early.
+		 */
+		if (qio_discard_updates_metadata(qcow2, *qio, map)) {
+			ret = prepare_cluster_discard(qcow2, qio, map);
+		} else if (!map->data_clu_alloced || map->clu_is_cow) {
+			qio_endio(*qio);
+			ret = 0;
+		}
 	} else if (write &&
 		   (!qio_is_fully_alloced(qcow2, *qio, map) || map->clu_is_cow)) {
 		if (map->clu_is_cow) {
@@ -3449,13 +3458,6 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
 			 */
 			map->backing_file_cow = true;
 			ret = prepare_l1l2_replace(qcow2, *qio, map);
-		} else if (unlikely(op_is_discard((*qio)->bi_op) &&
-				    (map->level & L2_LEVEL))) {
-			if (!map->data_clu_alloced) {
-				qio_endio(*qio);
-				ret = 0;
-			}
-			/* Otherwise issue_discard(). */
 		} else {
 			/* Wants L1 or L2 entry allocation */
 			ret = prepare_l1l2_allocation(qcow2, *qio, map);
@@ -3569,8 +3571,8 @@ 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)) {
+	/* Process discard early. Don't do COW just to discard */
+	if (unlikely(op_is_discard(qio->bi_op))) {
 		issue_discard(&map, qio);
 		return;
 	}
@@ -3588,8 +3590,6 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
 			submit_read_whole_cow_clu(&map, qio);
 		else if (unlikely(map.clu_is_cow || map.backing_file_cow))
 			submit_read_sliced_cow_clu(&map, qio);
-		else if (unlikely(op_is_discard(qio->bi_op)))
-			issue_discard(&map, qio);
 		else
 			perform_rw_mapped(&map, qio);
 	}
-- 
2.43.5

[Devel] [PATCH VZ10 v2 05/10] drivers/md/dm-qcow2: set L2_READS_ALL_ZEROES after some discards

[Andrey Zhadchenko]

Discard does not guarantee zero data. Therefore, to safely erase
data, users may write zeroes and then discard. Imagine we have
a backing file. If we write zeroes and do the discard, next read
will give the stale data from backing image.
Probably this is rather an edge case, but to be sure let's just
set L2 entry to L2_READS_ALL_ZEROES so the device looks more
consistent to users.

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 | 24 +++++++++++++++++++++---
 drivers/md/dm-qcow2.h     |  1 +
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index dcc65735881d4..3e3d10c3ebec4 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -1309,6 +1309,7 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
 	struct page *pe_page;
 	bool skip_odd;
 	u64 pos, old;
+	bool cleared;
 	int i, ret;
 
 	skip_odd = qcow2->ext_l2 && wbd->lx_level == L2_LEVEL;
@@ -1317,6 +1318,8 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
 	for_each_set_bit(i, wbd->changed_indexes, LX_INDEXES_PER_PAGE) {
 		pos = get_u64_from_be_page(wbd->md->page, i);
 
+		cleared = !(pos & ~(u64)L2_READS_ALL_ZEROES);
+
 		/* Here we restore prealloced and compressed clu mappings */
 		pe_page = wbd->pe_page;
 		if (pe_page) { /* Only L2 has this. */
@@ -1330,7 +1333,7 @@ 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)
+		if (cleared)
 			continue; /* no cluster was allocated */
 
 		spin_unlock(&qcow2->md_pages_lock);
@@ -3368,12 +3371,16 @@ static bool qio_discard_updates_metadata(struct qcow2 *qcow2, struct qio *qio,
  *
  * Discard covering the whole cluster replaces the entry and unuses
  * the discarded (or COW source) cluster.
+ * If the backing is present, set 'reads as zeroes' to avoid exposing
+ * stale data.
  */
 static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
 				   struct qcow2_map *map)
 {
+	bool zeroes = maybe_mapped_in_lower_delta(qcow2, *qio);
 	u32 index_in_page = map->l2.index_in_page;
 	struct md_page *md = map->l2.md;
+	u64 new_ext_l2 = map->ext_l2;
 	loff_t unuse_pos, unuse_end;
 	struct qio_ext *ext;
 	int ret;
@@ -3399,6 +3406,8 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
 		unuse_pos = map->data_clu_pos;
 		unuse_end = map->data_clu_pos + qcow2->clu_size;
 	}
+	if (zeroes && qcow2->ext_l2)
+		new_ext_l2 = (u64)U32_MAX << 32;
 
 	ret = prepare_l_entry_replace(qcow2, map, *qio, md, index_in_page,
 				      unuse_pos, unuse_end, L2_LEVEL);
@@ -3408,6 +3417,10 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
 	ext = (*qio)->ext;
 	ext->lx_md = md;
 
+	ext->new_ext_l2 = new_ext_l2;
+	if (zeroes && !qcow2->ext_l2)
+		ext->set_all_zeroes = true;
+
 	(*qio)->flags |= QIO_IS_DISCARD_FL;
 	return 1;
 }
@@ -4050,6 +4063,7 @@ static void process_indexes_write(struct qcow2 *qcow2,
 	struct qio *qio;
 	bool discard;
 	u32 arg_mask;
+	u64 entry;
 	int ret;
 
 	while (1) {
@@ -4074,11 +4088,15 @@ static void process_indexes_write(struct qcow2 *qcow2,
 			goto set_ext_l2;
 		}
 
+		entry = ext->allocated_clu_pos;
+		if (unlikely(ext->set_all_zeroes))
+			entry = L2_READS_ALL_ZEROES;
+
 		/* XXX: check prealloced_pos ==> revert */
 		ret = prepare_l_entry_update(qcow2, qio, lx_md,
 					     ext->lx_index_in_page,
-					     &ext->allocated_clu_pos,
-					     arg_mask, ext->lx_level);
+					     &entry, arg_mask,
+					     ext->lx_level);
 		if (ret < 0) {
 			qio->bi_status = errno_to_blk_status(ret);
 			qio_endio(qio);
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index 8a24e04130e4d..0f006f1ae48cc 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -312,6 +312,7 @@ struct qio_ext {
 	u64 new_ext_l2;
 	u32 cow_mask;
 	bool only_set_ext_l2:1;
+	bool set_all_zeroes:1;
 
 	u8 lx_level;
 
-- 
2.43.5

[Devel] [PATCH VZ10 v2 06/10] drivers/md/dm-qcow2: update metadata on subclusters discard

[Andrey Zhadchenko]

On images with extended L2 entries a discard smaller than the whole
cluster only punches a hole in the image file, while the subcluster
allocation bitmap still reports the range as allocated.

Extend prepare_cluster_discard() to also handle discards covering
whole subclusters of a mapped non-compressed cluster: clear the
"allocated" bits of the covered subclusters in the extended L2 entry
via the existing only_set_ext_l2 machinery. If the range may be
mapped in lower delta, set the "reads as zeroes" bits instead, so the
discarded range doesn't expose stale lower data. The cluster itself
remains allocated, so refcounts are not touched and the unuse stage
has nothing to do (empty unuse range). The covered range is hole
punched unless the cluster is shared with a snapshot.

ext_l2 and unuse range handling are in different if blocks for a while:
next patch will cover the case when we need uncoupling.

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>
---
v2:
 - expand qio_discard_updates_metadata() condition to return true
when backing file is present and fully covered sublcu zero bit isn't
already set

 drivers/md/dm-qcow2-map.c | 70 ++++++++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 12 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 3e3d10c3ebec4..9b40a985ac3cc 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -107,6 +107,21 @@ static bool qio_covers_full_clu(struct qcow2 *qcow2, struct qio *qio)
 	       qio->bi_iter.bi_size == qcow2->clu_size;
 }
 
+/* Mask of subclusters fully covered by qio (qio is trimmed inward) */
+static u32 qio_full_subclus_mask(struct qcow2 *qcow2, struct qio *qio)
+{
+	u32 off = bytes_off_in_cluster(qcow2, qio);
+	u32 first_bit = DIV_ROUND_UP(off, qcow2->subclu_size);
+	u32 end_bit = (off + qio->bi_iter.bi_size) / qcow2->subclu_size;
+
+	WARN_ON_ONCE(!qcow2->ext_l2);
+
+	if (end_bit <= first_bit)
+		return 0;
+
+	return GENMASK(end_bit - 1, first_bit);
+}
+
 static loff_t compressed_clu_end_pos(loff_t start, sector_t compressed_sectors)
 {
 	if (start % SECTOR_SIZE == 0)
@@ -3357,11 +3372,21 @@ static void issue_discard(struct qcow2_map *map, struct qio *qio)
 static bool qio_discard_updates_metadata(struct qcow2 *qcow2, struct qio *qio,
 					 struct qcow2_map *map)
 {
+	u32 mask;
+
 	if (!(map->level & L2_LEVEL) || !map->data_clu_alloced)
 		return false;
 	if (qio_covers_full_clu(qcow2, qio))
 		return true;
-	return false;
+	if (!qcow2->ext_l2 || map->compressed)
+		return false;
+
+	mask = qio_full_subclus_mask(qcow2, qio);
+	if ((u32)map->ext_l2 & mask)
+		return true;
+
+	return maybe_mapped_in_lower_delta(qcow2, qio) &&
+	       (mask & ~(u32)(map->ext_l2 >> 32));
 }
 
 /*
@@ -3371,6 +3396,9 @@ static bool qio_discard_updates_metadata(struct qcow2 *qcow2, struct qio *qio,
  *
  * Discard covering the whole cluster replaces the entry and unuses
  * the discarded (or COW source) cluster.
+ * Subclusters fully covered by the discard are marked unallocated in
+ * ext_l2 bitmap: the cluster itself remains allocated, so refcounts
+ * are not touched (hence empty unuse range).
  * If the backing is present, set 'reads as zeroes' to avoid exposing
  * stale data.
  */
@@ -3380,8 +3408,9 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
 	bool zeroes = maybe_mapped_in_lower_delta(qcow2, *qio);
 	u32 index_in_page = map->l2.index_in_page;
 	struct md_page *md = map->l2.md;
-	u64 new_ext_l2 = map->ext_l2;
 	loff_t unuse_pos, unuse_end;
+	bool whole_clu;
+	u64 new_ext_l2 = 0;
 	struct qio_ext *ext;
 	int ret;
 
@@ -3397,17 +3426,32 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
 	}
 	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;
+	whole_clu = qio_covers_full_clu(qcow2, *qio);
+	if (whole_clu) {
+		if (zeroes && qcow2->ext_l2)
+			new_ext_l2 = (u64)U32_MAX << 32;
+	} else {
+		u64 mask = qio_full_subclus_mask(qcow2, *qio);
+
+		new_ext_l2 = map->ext_l2 & ~(mask << 32 | mask);
+		if (zeroes)
+			new_ext_l2 |= mask << 32;
+	}
+
+	if (whole_clu) {
+		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 */
+			unuse_pos = map->data_clu_pos;
+			unuse_end = map->data_clu_pos + qcow2->clu_size;
+		}
 	} 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;
+		/* Subclusters become unallocated, cluster remains */
+		unuse_pos = unuse_end = 0;
 	}
-	if (zeroes && qcow2->ext_l2)
-		new_ext_l2 = (u64)U32_MAX << 32;
 
 	ret = prepare_l_entry_replace(qcow2, map, *qio, md, index_in_page,
 				      unuse_pos, unuse_end, L2_LEVEL);
@@ -3418,7 +3462,9 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
 	ext->lx_md = md;
 
 	ext->new_ext_l2 = new_ext_l2;
-	if (zeroes && !qcow2->ext_l2)
+	if (!whole_clu)
+		ext->only_set_ext_l2 = true;
+	else if (zeroes && !qcow2->ext_l2)
 		ext->set_all_zeroes = true;
 
 	(*qio)->flags |= QIO_IS_DISCARD_FL;
-- 
2.43.5

[Devel] [PATCH VZ10 v2 07/10] drivers/md/dm-qcow2: unmap cluster when discard clears last allocated subclusters

[Andrey Zhadchenko]

A subclusters discard only clears bits in the extended L2 bitmap and
keeps the cluster mapped. Teach dm-qcow2 to clear L2 entry if every
subcluster is discarded.

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 | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 9b40a985ac3cc..dbf5464eb9ab0 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -122,6 +122,17 @@ static u32 qio_full_subclus_mask(struct qcow2 *qcow2, struct qio *qio)
 	return GENMASK(end_bit - 1, first_bit);
 }
 
+static bool qio_discard_unmaps_cluster(struct qcow2 *qcow2, struct qio *qio,
+				       struct qcow2_map *map)
+{
+	if (qio_covers_full_clu(qcow2, qio))
+		return true;
+	if (!qcow2->ext_l2 || map->compressed)
+		return false;
+	return (u32)map->ext_l2 &&
+	       !((u32)map->ext_l2 & ~qio_full_subclus_mask(qcow2, qio));
+}
+
 static loff_t compressed_clu_end_pos(loff_t start, sector_t compressed_sectors)
 {
 	if (start % SECTOR_SIZE == 0)
@@ -2044,9 +2055,10 @@ static int parse_metadata(struct qcow2 *qcow2, struct qio **qio,
 	if (!write)
 		return 0;
 
-	/* discards also need to update r1r2 */
+	/* cluster unmapping discards also need to update r1r2 */
 	if (!map->clu_is_cow &&
-	    !(op_is_discard((*qio)->bi_op) && qio_covers_full_clu(qcow2, *qio)))
+	    !(op_is_discard((*qio)->bi_op) &&
+	      qio_discard_unmaps_cluster(qcow2, *qio, map)))
 		return 0;
 
 	/* Now refcounters table/block */
@@ -3399,6 +3411,8 @@ static bool qio_discard_updates_metadata(struct qcow2 *qcow2, struct qio *qio,
  * Subclusters fully covered by the discard are marked unallocated in
  * ext_l2 bitmap: the cluster itself remains allocated, so refcounts
  * are not touched (hence empty unuse range).
+ * Discarding last subclusters in a cluster clears it from L2 like a
+ * whole cluster discard.
  * If the backing is present, set 'reads as zeroes' to avoid exposing
  * stale data.
  */
@@ -3409,7 +3423,7 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
 	u32 index_in_page = map->l2.index_in_page;
 	struct md_page *md = map->l2.md;
 	loff_t unuse_pos, unuse_end;
-	bool whole_clu;
+	bool whole_clu, unmap;
 	u64 new_ext_l2 = 0;
 	struct qio_ext *ext;
 	int ret;
@@ -3427,6 +3441,8 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
 	spin_unlock_irq(&qcow2->md_pages_lock);
 
 	whole_clu = qio_covers_full_clu(qcow2, *qio);
+	unmap = qio_discard_unmaps_cluster(qcow2, *qio, map);
+
 	if (whole_clu) {
 		if (zeroes && qcow2->ext_l2)
 			new_ext_l2 = (u64)U32_MAX << 32;
@@ -3438,7 +3454,7 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
 			new_ext_l2 |= mask << 32;
 	}
 
-	if (whole_clu) {
+	if (unmap) {
 		if (map->clu_is_cow) {
 			/* Cluster is shared or compressed. Decrement refcount. */
 			unuse_pos = map->cow_clu_pos;
@@ -3462,7 +3478,7 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
 	ext->lx_md = md;
 
 	ext->new_ext_l2 = new_ext_l2;
-	if (!whole_clu)
+	if (!unmap)
 		ext->only_set_ext_l2 = true;
 	else if (zeroes && !qcow2->ext_l2)
 		ext->set_all_zeroes = true;
-- 
2.43.5

[Devel] [PATCH VZ10 v2 08/10] drivers/md/dm-qcow2: do discards during backward merge only for writable image

[Andrey Zhadchenko]

Backward merge discards every merged cluster from the image we merge
from: the L2 entry is zeroed and refcounts are decremented, which
dirties the image metadata. This requires the image file to be opened
for write, while merge-in-the-middle images usually opened read-only.

Skip the discard step when the merged image is read-only:
 - complete the merge qio right after its data is written to the lower
delta, without the L1/L2 entry update;
 - process READs on such image in the regular way. Previously reads would
cause out-of-order merge for present cluster and then requeue. Without
discard it will loop.
 - do not break COW at L1. Note that due to how merge machinery works,
we can't merge without unuse and internal snaphots (to be addressed
in the next patches).
 - do not clear the dirty bit on merge completion: the image was never
modified.

Just in case add warning and end qio if we somehow encounter non-service
write qio for read-only images during the merge.

https://virtuozzo.atlassian.net/browse/VSTOR-138288
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-qcow2-cmd.c | 23 +++++++++++++++--------
 drivers/md/dm-qcow2-map.c | 27 ++++++++++++++++++++++++++-
 drivers/md/dm-qcow2.h     |  5 +++++
 3 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
index c15c46a0fe8b7..f2c8f51e17af6 100644
--- a/drivers/md/dm-qcow2-cmd.c
+++ b/drivers/md/dm-qcow2-cmd.c
@@ -264,7 +264,9 @@ static int qcow2_merge_backward_start(struct qcow2_target *tgt, int efd, u32 dep
 	lower = qcow2->lower;
 	if (!lower)
 		return -ENOENT;
-	if (!(lower->file->f_mode & FMODE_WRITE))
+	if (!qcow2_file_is_writable(lower))
+		return -EACCES;
+	if (!qcow2_file_is_writable(qcow2) && qcow2->hdr.nb_snapshots)
 		return -EACCES;
 	if (qcow2->clu_size != lower->clu_size)
 		return -EOPNOTSUPP;
@@ -315,11 +317,14 @@ void qcow2_merge_backward_work(struct work_struct *work)
 	 * there would be problems with unusing them:
 	 * we'd have to freeze IO going to all data clusters
 	 * under every L1 entry related to several snapshots.
+	 * Readonly images skip this stage.
 	 */
-	ret = qcow2_break_l1cow(tgt, qcow2);
-	if (ret) {
-		QC_ERR(tgt->ti, "Can't break L1 COW");
-		goto out_err;
+	if (qcow2_file_is_writable(qcow2)) {
+		ret = qcow2_break_l1cow(tgt, qcow2);
+		if (ret) {
+			QC_ERR(tgt->ti, "Can't break L1 COW");
+			goto out_err;
+		}
 	}
 
 	backward_merge_update_stage(tgt, BACKWARD_MERGE_STAGE_SET_DIRTY);
@@ -388,9 +393,11 @@ static int qcow2_merge_backward_complete(struct qcow2_target *tgt)
 	qcow2_flush_deferred_activity(tgt, qcow2); /* Delayed md pages */
 	qcow2->lower = NULL;
 
-	ret = qcow2_set_image_file_features(qcow2, false);
-	if (ret < 0)
-		QC_ERR(tgt->ti, "Can't unuse merged img (%d)", ret);
+	if (qcow2_file_is_writable(qcow2)) {
+		ret = qcow2_set_image_file_features(qcow2, false);
+		if (ret < 0)
+			QC_ERR(tgt->ti, "Can't unuse merged img (%d)", ret);
+	}
 	qcow2_destroy(qcow2);
 
 	tgt->backward_merge.state = BACKWARD_MERGE_STOPPED;
diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index dbf5464eb9ab0..ebe68e8d0d3d8 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2688,6 +2688,12 @@ static void backward_merge_write_complete(struct qcow2_target *tgt, struct qio *
 		return;
 	}
 
+	/* Skip discard for read-only source images */
+	if (!qcow2_file_is_writable(qcow2)) {
+		qio_endio(qio);
+		return;
+	}
+
 	WARN_ON_ONCE(qio->flags & QIO_IS_DISCARD_FL);
 	qio->flags |= QIO_IS_DISCARD_FL;
 
@@ -2731,6 +2737,17 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
 	struct qio *aux_qio;
 	int ret;
 
+	/* Readonly image mappings remain stable, so reads just go through */
+	if (!qcow2_file_is_writable(qcow2)) {
+		if (!op_is_write((*qio)->bi_op))
+			return 1;
+		if (WARN_ON_ONCE(!fake_merge_qio(*qio))) {
+			(*qio)->bi_status = BLK_STS_IOERR;
+			qio_endio(*qio);
+			return 0;
+		}
+	}
+
 	if (!map->data_clu_alloced) {
 		WARN_ON_ONCE(map->clu_is_cow); /* Strange COW at L1 */
 		if (fake_merge_qio(*qio)) {
@@ -3639,7 +3656,15 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
 	if (!handle_metadata(qcow2, &qio, &map))
 		return;
 
-	if (unlikely(qcow2->backward_merge_in_process)) {
+	/*
+	 * Merge machinery makes out of order merges for present
+	 * clusters when it sees the reads. But if the merge does
+	 * not discard the cluser mapping, it will spin endlessly.
+	 * So process only actual merge qios or reads from writable
+	 * images.
+	 */
+	if (unlikely(qcow2->backward_merge_in_process) &&
+	    (fake_merge_qio(qio) || qcow2_file_is_writable(qcow2))) {
 		submit_top_delta_read(&map, qio);
 		return;
 	}
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index 0f006f1ae48cc..d9b8c38e093f5 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -460,6 +460,11 @@ static inline bool qcow2_wants_check(struct qcow2_target *tgt)
 	return !!(tgt->md_writeback_error|tgt->truncate_error);
 }
 
+static inline bool qcow2_file_is_writable(struct qcow2 *qcow2)
+{
+	return qcow2->file->f_mode & FMODE_WRITE;
+}
+
 static inline void remap_to_clu(struct qcow2 *qcow2, struct qio *qio, loff_t clu_pos)
 {
 	qio->bi_iter.bi_sector &= (to_sector(qcow2->clu_size) - 1);
-- 
2.43.5

[Devel] [PATCH VZ10 v2 09/10] drivers/md/dm-qcow2: allow shared L1 entries during merge from RO image

[Andrey Zhadchenko]

For RO images with internal snapshots L1 entries are shared.
Write-mode metadata parsing stops at such entries to avoid
modifying a shared L2 table, which made prepare_backward_merge()
see the cluster as unallocated and skip it with
"nothing to merge": the merged result would silently lose all
data under shared L1 entries.
Allow such scenario in parse_l1() and ease WARN in
prepare_backward_merge().

https://virtuozzo.atlassian.net/browse/VSTOR-138288
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-qcow2-cmd.c |  2 --
 drivers/md/dm-qcow2-map.c | 18 ++++++++++++++++--
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
index f2c8f51e17af6..fa3762c58372b 100644
--- a/drivers/md/dm-qcow2-cmd.c
+++ b/drivers/md/dm-qcow2-cmd.c
@@ -266,8 +266,6 @@ static int qcow2_merge_backward_start(struct qcow2_target *tgt, int efd, u32 dep
 		return -ENOENT;
 	if (!qcow2_file_is_writable(lower))
 		return -EACCES;
-	if (!qcow2_file_is_writable(qcow2) && qcow2->hdr.nb_snapshots)
-		return -EACCES;
 	if (qcow2->clu_size != lower->clu_size)
 		return -EOPNOTSUPP;
 	if (lower->hdr.size < qcow2->hdr.size)
diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index ebe68e8d0d3d8..baa6778f1686c 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -1726,6 +1726,14 @@ static bool qio_is_fully_alloced(struct qcow2 *qcow2, struct qio *qio,
 	return !(subclus_mask & ~alloced_mask);
 }
 
+static bool qio_may_modify_image(struct qcow2 *qcow2, struct qio *qio)
+{
+	if (qcow2_file_is_writable(qcow2))
+		return true;
+
+	return !fake_merge_qio(qio);
+}
+
 static loff_t parse_l1(struct qcow2 *qcow2, struct qcow2_map *map,
 		       struct qio **qio, bool write)
 
@@ -1762,7 +1770,8 @@ static loff_t parse_l1(struct qcow2 *qcow2, struct qcow2_map *map,
 		goto out;
 	if (delay_if_dirty(qcow2, l1->md, l1->index_in_page, qio))
 		goto out;
-	if (write && map->clu_is_cow)
+	/* Don't refuse L1 parse for merge qios with readonly disks */
+	if (write && map->clu_is_cow && qio_may_modify_image(qcow2, *qio))
 		goto out; /* Avoid to return pos */
 
 	ret = pos;
@@ -2061,6 +2070,10 @@ static int parse_metadata(struct qcow2 *qcow2, struct qio **qio,
 	      qio_discard_unmaps_cluster(qcow2, *qio, map)))
 		return 0;
 
+	/* Don't need refcount table if we don't modify the image */
+	if (!qio_may_modify_image(qcow2, *qio))
+		return 0;
+
 	/* Now refcounters table/block */
 	ret = qcow2_handle_r1r2_maps(qcow2, pos, qio, &map->r1,
 			       &map->r2, map->compressed);
@@ -2749,7 +2762,8 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
 	}
 
 	if (!map->data_clu_alloced) {
-		WARN_ON_ONCE(map->clu_is_cow); /* Strange COW at L1 */
+		/* Strange COW at L1, except the merge from RO image */
+		WARN_ON_ONCE(map->clu_is_cow && qio_may_modify_image(qcow2, *qio));
 		if (fake_merge_qio(*qio)) {
 			/* Nothing is to merge */
 			goto endio;
-- 
2.43.5

[Devel] [PATCH VZ10 v2 10/10] drivers/md/dm-qcow2: respect zeroes during merge

[Andrey Zhadchenko]

The merge machinery used to skip unallocated clusters, therefore
breaking all clusters with zero bits and non-zero backing.
Technically this bug was present even before discard changes, as
qcow2 image could have 'reads as zero' bits before being inserted
into dm-qcow2.
Properly commit all zero clusters (albeit with data for now).

https://virtuozzo.atlassian.net/browse/VSTOR-138288
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-qcow2-map.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index baa6778f1686c..87629328f8968 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2744,6 +2744,13 @@ static void requeue_if_ok(struct qcow2_target *tgt, struct qio *unused,
 	qcow2_dispatch_qios(qio->qcow2, qio, NULL);
 }
 
+static bool clu_zeroes_present(struct qcow2 *qcow2, struct qcow2_map *map)
+{
+	if (qcow2->ext_l2)
+		return (map->ext_l2 >> 32) != 0;
+	return map->all_zeroes;
+}
+
 static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
 				  struct qcow2_map *map, bool write)
 {
@@ -2761,7 +2768,7 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
 		}
 	}
 
-	if (!map->data_clu_alloced) {
+	if (!map->data_clu_alloced && !clu_zeroes_present(qcow2, map)) {
 		/* Strange COW at L1, except the merge from RO image */
 		WARN_ON_ONCE(map->clu_is_cow && qio_may_modify_image(qcow2, *qio));
 		if (fake_merge_qio(*qio)) {
@@ -2808,7 +2815,7 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
 		goto endio;
 	}
 
-	if (!map->clu_is_cow) {
+	if (!map->clu_is_cow && map->data_clu_alloced) {
 		/* Forced set these to unuse them after discard */
 		(*qio)->ext->unuse_clu_pos = map->data_clu_pos;
 		(*qio)->ext->unuse_clu_end = map->data_clu_pos + qcow2->clu_size;
-- 
2.43.5

Re: [Devel] [PATCH VZ10 v2 01/10] drivers/md/dm-qcow2: fix revert_cluster_alloc() for ext_l2 case

[Pavel Tikhomirov]

On 8/12/26 22:19, Andrey Zhadchenko wrote:
> This function walks over all changed u64 values in md. With ext_l2
> half of them holds subcluster description. Firstly it reverts these
> values to saved pe_page, which is fine, but then it tries to revert
> r1r2 changes. It makes no sense when the value is a subcluster
> description.
> Teach the function to skip subcluster descriptions: do it based on
> a new lx_level in struct wb_desc. Add new argument to
> prepare_l_entry_update() and set it there.
> The warning also could have tripped for ext_l2 entries, so drop
> it entirely.
> The function effectively reverts not only cluster alloc, so rename
> it to revert_l_entries_update() to mimic prepare_l_entries_update.

prepare_l_entrie(s)_update is probably a misspelled prepare_l_entry_update

> 
> 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    | 28 ++++++++++++++++++----------
>  drivers/md/dm-qcow2-target.c |  2 +-
>  drivers/md/dm-qcow2.h        |  1 +
>  3 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index db21efb45e17a..2cd8e174049f8 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -1297,18 +1297,19 @@ static void do_md_page_read_complete(int ret, struct qcow2 *qcow2,
>  }
>  
>  /* Be careful with dirty_or_writeback()/etc! Check races. */
> -static void revert_clusters_alloc(struct qcow2 *qcow2, struct wb_desc *wbd)
> +static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>  {
>  	struct qcow2_map_item r1, r2;
>  	struct page *pe_page;
> +	bool skip_odd;
>  	u64 pos, old;
>  	int i, ret;
>  
> +	skip_odd = qcow2->ext_l2 && wbd->lx_level == L2_LEVEL;
> +
>  	lockdep_assert_held(&qcow2->md_pages_lock);
>  	for_each_set_bit(i, wbd->changed_indexes, LX_INDEXES_PER_PAGE) {
>  		pos = get_u64_from_be_page(wbd->md->page, i);
> -		WARN_ON_ONCE(!(pos & ~LX_REFCOUNT_EXACTLY_ONE) ||
> -			     !(pos & LX_REFCOUNT_EXACTLY_ONE));
>  
>  		/* Here we restore prealloced and compressed clu mappings */
>  		pe_page = wbd->pe_page;
> @@ -1321,6 +1322,9 @@ static void revert_clusters_alloc(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 */
> +
>  		spin_unlock(&qcow2->md_pages_lock);
>  		pos &= ~LX_REFCOUNT_EXACTLY_ONE;
>  
> @@ -1369,7 +1373,7 @@ static void complete_wbd(struct qcow2 *qcow2, struct wb_desc *wbd)
>  		unsigned long flags;
>  
>  		spin_lock_irqsave(&qcow2->md_pages_lock, flags);
> -		revert_clusters_alloc(qcow2, wbd);
> +		revert_l_entries_update(qcow2, wbd);
>  		clear_writeback_status(qcow2, wbd->md, wbd->ret,
>  				       &wait_list, &end_list);
>  		spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
> @@ -2433,7 +2437,7 @@ static loff_t allocate_cluster(struct qcow2 *qcow2, struct qio *qio,
>  #define LU_IGN_CHANGED_IND	(1 << 3)
>  static int prepare_l_entry_update(struct qcow2 *qcow2, struct qio *qio,
>  				  struct md_page *md, u32 index_in_page,
> -				  u64 *pval, u32 arg_mask)
> +				  u64 *pval, u32 arg_mask, u8 lx_level)
>  {
>  	bool wants_pe_page = (arg_mask & LU_WANTS_PE_PAGE);
>  	struct wb_desc *new_wbd = NULL;
> @@ -2453,6 +2457,7 @@ static int prepare_l_entry_update(struct qcow2 *qcow2, struct qio *qio,
>  		if (!new_wbd)
>  			return -ENOMEM;
>  		new_wbd->md = md;
> +		new_wbd->lx_level = lx_level;
>  	} else if (wants_pe_page && !md->wbd->pe_page) {
>  		pe_page = alloc_page(GFP_NOIO|__GFP_ZERO);
>  		if (!pe_page)
> @@ -2515,7 +2520,8 @@ static int prepare_l1l2_allocation(struct qcow2 *qcow2, struct qio *qio,
>  		/* Allocate cluster for L2 entries, and prepare L1 update */
>  		ret = prepare_l_entry_update(qcow2, qio, map->l1.md,
>  					     map->l1.index_in_page, &val,
> -					     LU_SET_ONE_MASK|LU_WANTS_ALLOC);
> +					     LU_SET_ONE_MASK|LU_WANTS_ALLOC,
> +					     L1_LEVEL);
>  		if (ret <= 0)
>  			return ret;
>  
> @@ -2541,7 +2547,8 @@ static int prepare_l1l2_allocation(struct qcow2 *qcow2, struct qio *qio,
>  
>  		ret = prepare_l_entry_update(qcow2, qio, map->l2.md,
>  					     map->l2.index_in_page,
> -					     &map->data_clu_pos, arg_mask);
> +					     &map->data_clu_pos, arg_mask,
> +					     L2_LEVEL);
>  		if (ret <= 0)
>  			return ret;
>  
> @@ -2562,7 +2569,7 @@ static int prepare_l1l2_allocation(struct qcow2 *qcow2, struct qio *qio,
>  
>  	return prepare_l_entry_update(qcow2, qio, map->l2.md,
>  				      map->l2.index_in_page + 1,
> -				      &val, arg_mask);
> +				      &val, arg_mask, L2_LEVEL);
>  }
>  
>  /*
> @@ -3975,7 +3982,7 @@ static void process_cow_indexes_write(struct qcow2 *qcow2,
>  		ret = prepare_l_entry_update(qcow2, qio, lx_md,
>  					     ext->lx_index_in_page,
>  					     &ext->allocated_clu_pos,
> -					     arg_mask);
> +					     arg_mask, ext->cow_level);
>  		if (ret < 0) {
>  			qio->bi_status = errno_to_blk_status(ret);
>  			qio_endio(qio);
> @@ -3986,7 +3993,8 @@ static void process_cow_indexes_write(struct qcow2 *qcow2,
>  			arg_mask &= ~LU_SET_ONE_MASK;
>  			ret = prepare_l_entry_update(qcow2, qio, lx_md,
>  					     ext->lx_index_in_page + 1,
> -					   &ext->new_ext_l2, arg_mask);
> +					     &ext->new_ext_l2, arg_mask,
> +					     L2_LEVEL);
>  			WARN_ON_ONCE(ret < 0);
>  		}
>  
> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> index 3f65897ce9da2..0f1d3e3c5258e 100644
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c
> @@ -174,7 +174,7 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
>  	int i;
>  
>  	/*
> -	 * We need second iteration, since revert_clusters_alloc()
> +	 * We need second iteration, since revert_l_entries_update()
>  	 * may start timer again after failed wb.
>  	 */
>  	for (i = 0; i < 2; i++) {
> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
> index 86f0688e7345a..aa3487007523f 100644
> --- a/drivers/md/dm-qcow2.h
> +++ b/drivers/md/dm-qcow2.h
> @@ -118,6 +118,7 @@ struct wb_desc {
>  	struct list_head dependent_list;
>  	int nr_submitted;
>  	bool completed;
> +	u8 lx_level;
>  	int ret;
>  };
>  

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

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

[Pavel Tikhomirov]

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.

Re: [Devel] [PATCH VZ10 v2 00/10] dm-qcow2: improve discard and read-only merge handling

[Vasileios Almpanis]

> This series makes discard update qcow2 metadata and permits backward
> merge from read-only source images.
> 
> First patch improves revert_cluster_alloc: this function didn't expect
> L2 entries to contain subcluster descriptions (when ext2_l2 is set).
> 
> The second patch renames COW machinery to a more general 'replace entry'.
> 
> Next five patches improve discard support in general: now it clears
> metadata and have a subcluster granularity.
> 
> The next two patches support backward merge when the image we merge
> from is read-only. Such merges now leave the source mappings and
> metadata unchanged. Previously the module ignored file mode and
> submitted writes anyway.
> 
> Last patch makes merge respect all metadata-zeroed blocks. For now it is
> writing zeroes as data, but we will improve this to metadata when we
> support REQ_OP_WRITE_ZEROES.
Generally with my limited knowledge is looks good.
> 
> v2:
>  - patch#6: expand qio_discard_updates_metadata() condition to return
> true when backing file is present and fully covered sublcu zero bit isn't
> already set (prepare_cluster_discard() already handled this case but
> if forgot about it here)
> 
> Andrey Zhadchenko (10):
>   drivers/md/dm-qcow2: fix revert_cluster_alloc() for ext_l2 case
>   drivers/md/dm-qcow2: generalize COW index update machinery
>   drivers/md/dm-qcow2: update metadata on whole cluster discard
>   drivers/md/dm-qcow2: never trigger COW or allocation on discard
>   drivers/md/dm-qcow2: set L2_READS_ALL_ZEROES after some discards
>   drivers/md/dm-qcow2: update metadata on subclusters discard
>   drivers/md/dm-qcow2: unmap cluster when discard clears last allocated
>     subclusters
>   drivers/md/dm-qcow2: do discards during backward merge only for
>     writable image
>   drivers/md/dm-qcow2: allow shared L1 entries during merge from RO
>     image
>   drivers/md/dm-qcow2: respect zeroes during merge
> 
>  drivers/md/dm-qcow2-cmd.c    |  21 +-
>  drivers/md/dm-qcow2-map.c    | 390 ++++++++++++++++++++++++++++-------
>  drivers/md/dm-qcow2-target.c |   2 +-
>  drivers/md/dm-qcow2.h        |  19 +-
>  4 files changed, 337 insertions(+), 95 deletions(-)
> 
> --
> 2.43.5

Reviewed-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>

-- 
Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>

Re: [Devel] [PATCH VZ10 v2 04/10] drivers/md/dm-qcow2: never trigger COW or allocation on discard

[Vasileios Almpanis]

> In handle_metadata() the partial cluster discard fallback is checked
> only after the COW branches. As a result a discard, whose range
> touches a shared (snapshot or compressed) cluster or crosses an
> unmapped unit border over a backing file, is routed into the data COW
> machinery. COW then tries to submit the discard payload as a data
> write, but discard qios carry no bvec array: the iov_iter is built
> from garbage, which triggers the WARN_ON() in __submit_rw_mapped()
> and writes random kernel memory into the COW'ed cluster.
> 
> Check for discard before the COW branches: discard is advisory and
> carries no data, so it must never COW or allocate. End discard
> early while handling metadata if possible. Otherwise do discard
> operations before COWs.
> 
> 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>
>
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index 8d2c4e78a4c5..dcc65735881d 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -3431,9 +3431,18 @@ 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 (unlikely(op_is_discard((*qio)->bi_op))) {
> +		/*
> +		 * Discard carries no data and advisory. If it does not
NIT: no data and is advisory
> +		 * trigger metadata changes or to-be-discarded cluster
> +		 * is not present, end early.
> +		 */
> +		if (qio_discard_updates_metadata(qcow2, *qio, map)) {
> +			ret = prepare_cluster_discard(qcow2, qio, map);
> +		} else if (!map->data_clu_alloced || map->clu_is_cow) {
> +			qio_endio(*qio);
> +			ret = 0;
> +		}
>  	} else if (write &&
>  		   (!qio_is_fully_alloced(qcow2, *qio, map) || map->clu_is_cow)) {
>  		if (map->clu_is_cow) {
> @@ -3449,13 +3458,6 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
>  			 */
>  			map->backing_file_cow = true;
>  			ret = prepare_l1l2_replace(qcow2, *qio, map);
> -		} else if (unlikely(op_is_discard((*qio)->bi_op) &&
> -				    (map->level & L2_LEVEL))) {
> -			if (!map->data_clu_alloced) {
> -				qio_endio(*qio);
> -				ret = 0;
> -			}
> -			/* Otherwise issue_discard(). */
>  		} else {
>  			/* Wants L1 or L2 entry allocation */
>  			ret = prepare_l1l2_allocation(qcow2, *qio, map);
> @@ -3569,8 +3571,8 @@ 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)) {
> +	/* Process discard early. Don't do COW just to discard */
> +	if (unlikely(op_is_discard(qio->bi_op))) {
>  		issue_discard(&map, qio);
>  		return;
>  	}
> @@ -3588,8 +3590,6 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
>  			submit_read_whole_cow_clu(&map, qio);
>  		else if (unlikely(map.clu_is_cow || map.backing_file_cow))
>  			submit_read_sliced_cow_clu(&map, qio);
> -		else if (unlikely(op_is_discard(qio->bi_op)))
> -			issue_discard(&map, qio);
>  		else
>  			perform_rw_mapped(&map, qio);
>  	}
side note: for single-bio discard requests bi_io_vec is actually
a garbage pointer set in prepare_one_embedded_qio in the else 
branch. We set bvec to rq->bio->bi_inline_vecs which is never null its a
flexible array. I think we guard in all places for discards based on op so
we don't meet any issue but just wanted to point it out.

-- 
Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>

Re: [Devel] [PATCH VZ10 v2 02/10] drivers/md/dm-qcow2: generalize COW index update machinery

[Vasileios Almpanis]

> The two-stage L1/L2 entry update pipeline is not COW-specific:
> backward merge already uses it to zero L2 entries, and upcoming
> discard support will be the third user. Rename it to reflect
> what it does.
> 
> No functional changes.
> 
> 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>
>
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index 2cd8e174049f..08d46eb177dd 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -2579,10 +2579,10 @@ static int prepare_l1l2_allocation(struct qcow2 *qcow2, struct qio *qio,
>   * we have to wait all previous READs. We do that around
>   * index wb. See md->wpc_noread_count update details.
>   */
> -static int prepare_l_entry_cow(struct qcow2 *qcow2, struct qcow2_map *map,
> -			       struct qio *qio, struct md_page *md,
> -			       u32 index_in_page, loff_t cow_clu_pos,
> -			       loff_t cow_clu_end, u8 cow_level)
> +static int prepare_l_entry_replace(struct qcow2 *qcow2, struct qcow2_map *map,
> +				   struct qio *qio, struct md_page *md,
> +				   u32 index_in_page, loff_t unuse_clu_pos,
> +				   loff_t unuse_clu_end, u8 lx_level)
>  {
>  	struct lock_desc *lockd = NULL;
>  	struct qio_ext *ext;
> @@ -2591,9 +2591,9 @@ static int prepare_l_entry_cow(struct qcow2 *qcow2, struct qcow2_map *map,
>  		return -ENOMEM;
>  
>  	ext = qio->ext;
> -	ext->cow_clu_pos = cow_clu_pos;
> -	ext->cow_clu_end = cow_clu_end;
> -	ext->cow_level = cow_level;
> +	ext->unuse_clu_pos = unuse_clu_pos;
> +	ext->unuse_clu_end = unuse_clu_end;
> +	ext->lx_level = lx_level;
>  
>  	spin_lock_irq(&qcow2->md_pages_lock);
>  	if (!md->lockd) {
> @@ -2614,23 +2614,23 @@ static int prepare_l_entry_cow(struct qcow2 *qcow2, struct qcow2_map *map,
>  	return 1;
>  }
>  
> -static int prepare_l1l2_cow(struct qcow2 *qcow2, struct qio *qio,
> -			    struct qcow2_map *map)
> +static int prepare_l1l2_replace(struct qcow2 *qcow2, struct qio *qio,
> +				struct qcow2_map *map)
>  {
>  	if (WARN_ON_ONCE(!(map->level & L1_LEVEL)))
>  		return -EIO; /* Sanity check: L1 must be cached */
>  
>  	if (!(map->level & L2_LEVEL)) {
> -		return prepare_l_entry_cow(qcow2, map, qio, map->l1.md,
> -					   map->l1.index_in_page,
> -					   map->cow_clu_pos,
> -					   map->cow_clu_end, L1_LEVEL);
> +		return prepare_l_entry_replace(qcow2, map, qio, map->l1.md,
> +					       map->l1.index_in_page,
> +					       map->cow_clu_pos,
> +					       map->cow_clu_end, L1_LEVEL);
>  	}
>  
> -	return prepare_l_entry_cow(qcow2, map, qio, map->l2.md,
> -				  map->l2.index_in_page,
> -				  map->cow_clu_pos,
> -				  map->cow_clu_end, L2_LEVEL);
> +	return prepare_l_entry_replace(qcow2, map, qio, map->l2.md,
> +				       map->l2.index_in_page,
> +				       map->cow_clu_pos,
> +				       map->cow_clu_end, L2_LEVEL);
>  }
>  
>  static void backward_merge_write_complete(struct qcow2_target *tgt, struct qio *unused,
> @@ -2648,7 +2648,7 @@ static void backward_merge_write_complete(struct qcow2_target *tgt, struct qio *
>  	WARN_ON_ONCE(qio->flags & QIO_IS_DISCARD_FL);
>  	qio->flags |= QIO_IS_DISCARD_FL;
>  
> -	qio->queue_list_id = QLIST_COW_INDEXES;
> +	qio->queue_list_id = QLIST_INDEXES_WRITE;
>  	qcow2_dispatch_qios(qcow2, qio, NULL);
>  }
>  
> @@ -2704,7 +2704,7 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
>  	if (!op_is_write((*qio)->bi_op)) {
>  		/*
>  		 * READ qio may data may be contained in several deltas.
> -		 * We can't read lower delta after prepare_l1l2_cow()
> +		 * We can't read lower delta after prepare_l1l2_replace()
>  		 * prepares us.
>  		 */
>  		aux_qio = qcow2_alloc_qio(qcow2->tgt->qio_pool, true);
> @@ -2725,10 +2725,10 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
>  	}
>  
>  	/*
> -	 * Mark as COW, as this completely defers any parallel qios.
> -	 * @qio is COW status holder.
> +	 * Lock the entry, as this completely defers any parallel qios.
> +	 * @qio is the lock holder.
>  	 */
> -	ret = prepare_l1l2_cow(qcow2, *qio, map);
> +	ret = prepare_l1l2_replace(qcow2, *qio, map);
>  	if (ret < 0) {
>  		(*qio)->bi_status = errno_to_blk_status(ret);
>  		goto endio;
> @@ -2736,13 +2736,13 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
>  
>  	if (!map->clu_is_cow) {
>  		/* Forced set these to unuse them after discard */
> -		(*qio)->ext->cow_clu_pos = map->data_clu_pos;
> -		(*qio)->ext->cow_clu_end = map->data_clu_pos + qcow2->clu_size;
> +		(*qio)->ext->unuse_clu_pos = map->data_clu_pos;
> +		(*qio)->ext->unuse_clu_end = map->data_clu_pos + qcow2->clu_size;
>  	}
>  
>  	return 1;
>  endio:
> -	qio_endio(*qio); /* Breaks COW set in prepare_l1l2_cow() */
> +	qio_endio(*qio); /* Releases the lock set in prepare_l1l2_replace() */
>  	return 0;
>  }
>  
> @@ -3348,7 +3348,7 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
>  		   (!qio_is_fully_alloced(qcow2, *qio, map) || map->clu_is_cow)) {
>  		if (map->clu_is_cow) {
>  			/* COW to compressed or shared with snapshot cluster */
> -			ret = prepare_l1l2_cow(qcow2, *qio, map);
> +			ret = prepare_l1l2_replace(qcow2, *qio, map);
>  		} else if ((map->level & L2_LEVEL) &&
>  		    qio_border_is_inside_unmapped_unit(qcow2, *qio, map) &&
>  		    maybe_mapped_in_lower_delta(qcow2, *qio)) {
> @@ -3358,7 +3358,7 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
>  			 * snapshots). Here is data COW on L2_LEVEL.
>  			 */
>  			map->backing_file_cow = true;
> -			ret = prepare_l1l2_cow(qcow2, *qio, map);
> +			ret = prepare_l1l2_replace(qcow2, *qio, map);
>  		} else if (unlikely(op_is_discard((*qio)->bi_op) &&
>  				    (map->level & L2_LEVEL))) {
>  			if (!map->data_clu_alloced) {
> @@ -3837,7 +3837,7 @@ static void cow_data_write_endio(struct qcow2_target *tgt, struct qio *unused,
>  		qio->bi_status = bi_status;
>  		qio_endio(qio);
>  	} else {
> -		qio->queue_list_id = QLIST_COW_INDEXES;
> +		qio->queue_list_id = QLIST_INDEXES_WRITE;
>  		qcow2_dispatch_qios(qcow2, qio, NULL);
>  	}
>  }
> @@ -3885,7 +3885,7 @@ static void sliced_cow_data_write_complete(struct qcow2_target *tgt, struct qio
>  		qio->bi_status = bi_status;
>  		qio_endio(qio);
>  	} else {
> -		qio->queue_list_id = QLIST_COW_INDEXES;
> +		qio->queue_list_id = QLIST_INDEXES_WRITE;
>  		qcow2_dispatch_qios(qcow2, qio, NULL);
>  	}
>  }
> @@ -3919,7 +3919,7 @@ static void process_cow_data_write(struct qcow2 *qcow2, struct list_head *cow_li
>  		ext = qio->ext;
>  
>  		if (ext->only_set_ext_l2) {
> -			WARN_ON_ONCE(ext->cow_level != L2_LEVEL);
> +			WARN_ON_ONCE(ext->lx_level != L2_LEVEL);
>  			pos = ext->allocated_clu_pos;
>  			goto submit;
>  		}
> @@ -3938,14 +3938,14 @@ static void process_cow_data_write(struct qcow2 *qcow2, struct list_head *cow_li
>  		ext->allocated_clu_pos = pos;
>  		ext->cleanup_mask |= FREE_ALLOCATED_CLU;
>  submit:
> -		if (ext->cow_level == L2_LEVEL)
> +		if (ext->lx_level == L2_LEVEL)
>  			submit_sliced_cow_data_write(qcow2, qio, pos);
>  		else
>  			submit_cow_data_write(qcow2, qio, pos);
>  	}
>  }
>  
> -static void process_cow_indexes_write(struct qcow2 *qcow2,
> +static void process_indexes_write(struct qcow2 *qcow2,
>  				      struct list_head *qio_list)
>  {
NIT: qvec is unused. Its only set here, so lets just drop it since we touch this
function.

-- 
Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>

Re: [Devel] [PATCH VZ10 v2 05/10] drivers/md/dm-qcow2: set L2_READS_ALL_ZEROES after some discards

[Pavel Tikhomirov]

On 8/12/26 22:20, Andrey Zhadchenko wrote:
> Discard does not guarantee zero data. Therefore, to safely erase
> data, users may write zeroes and then discard. Imagine we have
> a backing file. If we write zeroes and do the discard, next read
> will give the stale data from backing image.
> Probably this is rather an edge case, but to be sure let's just
> set L2 entry to L2_READS_ALL_ZEROES so the device looks more
> consistent to users.
> 
> 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>

I'm not fully understating it all together, but this sounds like a regression fix for:

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

rather then a fix of something preexisting, before this patch we had allocated cluster
with punch-holed data there and never went to backing image, after it we start see stale
data. So suggestion here is to somehow reorder or merge this fix into the introducing
patch in the series to fix bisectability.

AI thoughts on (1):

A whole-cluster discard on a delta with a lower image now returns the backing image's data on the next read. The new path zeroes the L2 entry (and the ext_l2 bitmap), so parse_l2() leaves data_clu_alloced and all_zeroes clear, qio_unmapped_size() reports the whole cluster, and calc_front_qio_bytes() sets try_lower = maybe_mapped_in_lower_delta() ? process_read_qio() redirects the read to qcow2->lower.


> ---
>  drivers/md/dm-qcow2-map.c | 24 +++++++++++++++++++++---
>  drivers/md/dm-qcow2.h     |  1 +
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index dcc65735881d4..3e3d10c3ebec4 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -1309,6 +1309,7 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>  	struct page *pe_page;
>  	bool skip_odd;
>  	u64 pos, old;
> +	bool cleared;
>  	int i, ret;
>  
>  	skip_odd = qcow2->ext_l2 && wbd->lx_level == L2_LEVEL;
> @@ -1317,6 +1318,8 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>  	for_each_set_bit(i, wbd->changed_indexes, LX_INDEXES_PER_PAGE) {
>  		pos = get_u64_from_be_page(wbd->md->page, i);
>  
> +		cleared = !(pos & ~(u64)L2_READS_ALL_ZEROES);
> +
>  		/* Here we restore prealloced and compressed clu mappings */
>  		pe_page = wbd->pe_page;
>  		if (pe_page) { /* Only L2 has this. */
> @@ -1330,7 +1333,7 @@ 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)
> +		if (cleared)
>  			continue; /* no cluster was allocated */
>  
>  		spin_unlock(&qcow2->md_pages_lock);
> @@ -3368,12 +3371,16 @@ static bool qio_discard_updates_metadata(struct qcow2 *qcow2, struct qio *qio,
>   *
>   * Discard covering the whole cluster replaces the entry and unuses
>   * the discarded (or COW source) cluster.
> + * If the backing is present, set 'reads as zeroes' to avoid exposing
> + * stale data.
>   */
>  static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>  				   struct qcow2_map *map)
>  {
> +	bool zeroes = maybe_mapped_in_lower_delta(qcow2, *qio);
>  	u32 index_in_page = map->l2.index_in_page;
>  	struct md_page *md = map->l2.md;
> +	u64 new_ext_l2 = map->ext_l2;
>  	loff_t unuse_pos, unuse_end;
>  	struct qio_ext *ext;
>  	int ret;
> @@ -3399,6 +3406,8 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>  		unuse_pos = map->data_clu_pos;
>  		unuse_end = map->data_clu_pos + qcow2->clu_size;
>  	}
> +	if (zeroes && qcow2->ext_l2)
> +		new_ext_l2 = (u64)U32_MAX << 32;
>  
>  	ret = prepare_l_entry_replace(qcow2, map, *qio, md, index_in_page,
>  				      unuse_pos, unuse_end, L2_LEVEL);
> @@ -3408,6 +3417,10 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>  	ext = (*qio)->ext;
>  	ext->lx_md = md;
>  
> +	ext->new_ext_l2 = new_ext_l2;
> +	if (zeroes && !qcow2->ext_l2)
> +		ext->set_all_zeroes = true;
> +
>  	(*qio)->flags |= QIO_IS_DISCARD_FL;
>  	return 1;
>  }
> @@ -4050,6 +4063,7 @@ static void process_indexes_write(struct qcow2 *qcow2,
>  	struct qio *qio;
>  	bool discard;
>  	u32 arg_mask;
> +	u64 entry;
>  	int ret;
>  
>  	while (1) {
> @@ -4074,11 +4088,15 @@ static void process_indexes_write(struct qcow2 *qcow2,
>  			goto set_ext_l2;
>  		}
>  
> +		entry = ext->allocated_clu_pos;
> +		if (unlikely(ext->set_all_zeroes))
> +			entry = L2_READS_ALL_ZEROES;
> +
>  		/* XXX: check prealloced_pos ==> revert */
>  		ret = prepare_l_entry_update(qcow2, qio, lx_md,
>  					     ext->lx_index_in_page,
> -					     &ext->allocated_clu_pos,
> -					     arg_mask, ext->lx_level);
> +					     &entry, arg_mask,
> +					     ext->lx_level);
>  		if (ret < 0) {
>  			qio->bi_status = errno_to_blk_status(ret);
>  			qio_endio(qio);
> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
> index 8a24e04130e4d..0f006f1ae48cc 100644
> --- a/drivers/md/dm-qcow2.h
> +++ b/drivers/md/dm-qcow2.h
> @@ -312,6 +312,7 @@ struct qio_ext {
>  	u64 new_ext_l2;
>  	u32 cow_mask;
>  	bool only_set_ext_l2:1;
> +	bool set_all_zeroes:1;
>  
>  	u8 lx_level;
>  

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

Re: [Devel] [PATCH VZ10 v2 05/10] drivers/md/dm-qcow2: set L2_READS_ALL_ZEROES after some discards

[Andrey Zhadchenko]

On 8/19/26 13:05, Pavel Tikhomirov wrote:
> 
> 
> On 8/12/26 22:20, Andrey Zhadchenko wrote:
>> Discard does not guarantee zero data. Therefore, to safely erase
>> data, users may write zeroes and then discard. Imagine we have
>> a backing file. If we write zeroes and do the discard, next read
>> will give the stale data from backing image.
>> Probably this is rather an edge case, but to be sure let's just
>> set L2 entry to L2_READS_ALL_ZEROES so the device looks more
>> consistent to users.
>>
>> 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>
> 
> I'm not fully understating it all together, but this sounds like a regression fix for:
> 
> [PATCH VZ10 v2 03/10] drivers/md/dm-qcow2: update metadata on whole cluster discard (1)
> 
> rather then a fix of something preexisting, before this patch we had allocated cluster
> with punch-holed data there and never went to backing image, after it we start see stale
> data. So suggestion here is to somehow reorder or merge this fix into the introducing
> patch in the series to fix bisectability.

Yes and no :)
Yes in the sense that this 'problem' didn't exist beforehand. But 
previously discard always led to COW and small discards would pull the 
whole cluster from backing image.
No in the sense that contents after DISCARD are formally undefined so we 
are just being extra cautious here. So this can be viewed as an extra 
feature.

So I have split the code and arranged the patches for easier 
feature-by-feature review. I can still merge this patch in metadata 
processing patch if you wish.

Also note that even after this patch reads after discard may still show 
lower delta data. For example discarding less than a cluster (or 
subcluster after next patches) wouldn't cause 'reads as zeroes' metadata 
changes. If the image has no allocated clu/subclu subsequent reads will 
return lower delta data. It is again fine, as discard is advisory.


> 
> AI thoughts on (1):
> 
> A whole-cluster discard on a delta with a lower image now returns the backing image's data on the next read. The new path zeroes the L2 entry (and the ext_l2 bitmap), so parse_l2() leaves data_clu_alloced and all_zeroes clear, qio_unmapped_size() reports the whole cluster, and calc_front_qio_bytes() sets try_lower = maybe_mapped_in_lower_delta() ? process_read_qio() redirects the read to qcow2->lower.

 From what I see in calc_front_qio_bytes():

...
         arg->try_lower = false;

         size = qio_all_zeroes_size(qcow2, qio, map);
         if (size) {
                 arg->zeroes = true;
                 return size;
         }
...

So no, I don't think this leaks.


> 
> 
>> ---
>>   drivers/md/dm-qcow2-map.c | 24 +++++++++++++++++++++---
>>   drivers/md/dm-qcow2.h     |  1 +
>>   2 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
>> index dcc65735881d4..3e3d10c3ebec4 100644
>> --- a/drivers/md/dm-qcow2-map.c
>> +++ b/drivers/md/dm-qcow2-map.c
>> @@ -1309,6 +1309,7 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>>   	struct page *pe_page;
>>   	bool skip_odd;
>>   	u64 pos, old;
>> +	bool cleared;
>>   	int i, ret;
>>   
>>   	skip_odd = qcow2->ext_l2 && wbd->lx_level == L2_LEVEL;
>> @@ -1317,6 +1318,8 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>>   	for_each_set_bit(i, wbd->changed_indexes, LX_INDEXES_PER_PAGE) {
>>   		pos = get_u64_from_be_page(wbd->md->page, i);
>>   
>> +		cleared = !(pos & ~(u64)L2_READS_ALL_ZEROES);
>> +
>>   		/* Here we restore prealloced and compressed clu mappings */
>>   		pe_page = wbd->pe_page;
>>   		if (pe_page) { /* Only L2 has this. */
>> @@ -1330,7 +1333,7 @@ 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)
>> +		if (cleared)
>>   			continue; /* no cluster was allocated */
>>   
>>   		spin_unlock(&qcow2->md_pages_lock);
>> @@ -3368,12 +3371,16 @@ static bool qio_discard_updates_metadata(struct qcow2 *qcow2, struct qio *qio,
>>    *
>>    * Discard covering the whole cluster replaces the entry and unuses
>>    * the discarded (or COW source) cluster.
>> + * If the backing is present, set 'reads as zeroes' to avoid exposing
>> + * stale data.
>>    */
>>   static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>>   				   struct qcow2_map *map)
>>   {
>> +	bool zeroes = maybe_mapped_in_lower_delta(qcow2, *qio);
>>   	u32 index_in_page = map->l2.index_in_page;
>>   	struct md_page *md = map->l2.md;
>> +	u64 new_ext_l2 = map->ext_l2;
>>   	loff_t unuse_pos, unuse_end;
>>   	struct qio_ext *ext;
>>   	int ret;
>> @@ -3399,6 +3406,8 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>>   		unuse_pos = map->data_clu_pos;
>>   		unuse_end = map->data_clu_pos + qcow2->clu_size;
>>   	}
>> +	if (zeroes && qcow2->ext_l2)
>> +		new_ext_l2 = (u64)U32_MAX << 32;
>>   
>>   	ret = prepare_l_entry_replace(qcow2, map, *qio, md, index_in_page,
>>   				      unuse_pos, unuse_end, L2_LEVEL);
>> @@ -3408,6 +3417,10 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>>   	ext = (*qio)->ext;
>>   	ext->lx_md = md;
>>   
>> +	ext->new_ext_l2 = new_ext_l2;
>> +	if (zeroes && !qcow2->ext_l2)
>> +		ext->set_all_zeroes = true;
>> +
>>   	(*qio)->flags |= QIO_IS_DISCARD_FL;
>>   	return 1;
>>   }
>> @@ -4050,6 +4063,7 @@ static void process_indexes_write(struct qcow2 *qcow2,
>>   	struct qio *qio;
>>   	bool discard;
>>   	u32 arg_mask;
>> +	u64 entry;
>>   	int ret;
>>   
>>   	while (1) {
>> @@ -4074,11 +4088,15 @@ static void process_indexes_write(struct qcow2 *qcow2,
>>   			goto set_ext_l2;
>>   		}
>>   
>> +		entry = ext->allocated_clu_pos;
>> +		if (unlikely(ext->set_all_zeroes))
>> +			entry = L2_READS_ALL_ZEROES;
>> +
>>   		/* XXX: check prealloced_pos ==> revert */
>>   		ret = prepare_l_entry_update(qcow2, qio, lx_md,
>>   					     ext->lx_index_in_page,
>> -					     &ext->allocated_clu_pos,
>> -					     arg_mask, ext->lx_level);
>> +					     &entry, arg_mask,
>> +					     ext->lx_level);
>>   		if (ret < 0) {
>>   			qio->bi_status = errno_to_blk_status(ret);
>>   			qio_endio(qio);
>> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
>> index 8a24e04130e4d..0f006f1ae48cc 100644
>> --- a/drivers/md/dm-qcow2.h
>> +++ b/drivers/md/dm-qcow2.h
>> @@ -312,6 +312,7 @@ struct qio_ext {
>>   	u64 new_ext_l2;
>>   	u32 cow_mask;
>>   	bool only_set_ext_l2:1;
>> +	bool set_all_zeroes:1;
>>   
>>   	u8 lx_level;
>>   
> 

Re: [Devel] [PATCH VZ10 v2 05/10] drivers/md/dm-qcow2: set L2_READS_ALL_ZEROES after some discards

[Pavel Tikhomirov]

On 8/20/26 11:04, Andrey Zhadchenko wrote:
> 
> 
> On 8/19/26 13:05, Pavel Tikhomirov wrote:
>>
>>
>> On 8/12/26 22:20, Andrey Zhadchenko wrote:
>>> Discard does not guarantee zero data. Therefore, to safely erase
>>> data, users may write zeroes and then discard. Imagine we have
>>> a backing file. If we write zeroes and do the discard, next read
>>> will give the stale data from backing image.
>>> Probably this is rather an edge case, but to be sure let's just
>>> set L2 entry to L2_READS_ALL_ZEROES so the device looks more
>>> consistent to users.
>>>
>>> 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>
>>
>> I'm not fully understating it all together, but this sounds like a regression fix for:
>>
>> [PATCH VZ10 v2 03/10] drivers/md/dm-qcow2: update metadata on whole cluster discard (1)
>>
>> rather then a fix of something preexisting, before this patch we had allocated cluster
>> with punch-holed data there and never went to backing image, after it we start see stale
>> data. So suggestion here is to somehow reorder or merge this fix into the introducing
>> patch in the series to fix bisectability.
> 
> Yes and no :)
> Yes in the sense that this 'problem' didn't exist beforehand. But previously discard always led to COW and small discards would pull the whole cluster from backing image.
> No in the sense that contents after DISCARD are formally undefined so we are just being extra cautious here. So this can be viewed as an extra feature.
> 
> So I have split the code and arranged the patches for easier feature-by-feature review. I can still merge this patch in metadata processing patch if you wish.
> 
> Also note that even after this patch reads after discard may still show lower delta data. For example discarding less than a cluster (or subcluster after next patches) wouldn't cause 'reads as zeroes' metadata changes. If the image has no allocated clu/subclu subsequent reads will return lower delta data. It is again fine, as discard is advisory.


Ok let's leave it as is then.

But yeh from user perspective if I have device and I wrote some data to it at some offset and then I discard this data and write zeroes. And after that I miraculously see the old state of the device instead of zeroes, that can be really confusing. At least as far as I understand it. I guess if it's by design then that's ok.

> 
> 
>>
>> AI thoughts on (1):
>>
>> A whole-cluster discard on a delta with a lower image now returns the backing image's data on the next read. The new path zeroes the L2 entry (and the ext_l2 bitmap), so parse_l2() leaves data_clu_alloced and all_zeroes clear, qio_unmapped_size() reports the whole cluster, and calc_front_qio_bytes() sets try_lower = maybe_mapped_in_lower_delta() ? process_read_qio() redirects the read to qcow2->lower.
> 
> From what I see in calc_front_qio_bytes():
> 
> ...
> ??????? arg->try_lower = false;
> 
> ??????? size = qio_all_zeroes_size(qcow2, qio, map);
> ??????? if (size) {
> ??????????????? arg->zeroes = true;
> ??????????????? return size;
> ??????? }
> ...
> 
> So no, I don't think this leaks.
> 
> 
>>
>>
>>> ---
>>> ? drivers/md/dm-qcow2-map.c | 24 +++++++++++++++++++++---
>>> ? drivers/md/dm-qcow2.h???? |? 1 +
>>> ? 2 files changed, 22 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
>>> index dcc65735881d4..3e3d10c3ebec4 100644
>>> --- a/drivers/md/dm-qcow2-map.c
>>> +++ b/drivers/md/dm-qcow2-map.c
>>> @@ -1309,6 +1309,7 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>>> ????? struct page *pe_page;
>>> ????? bool skip_odd;
>>> ????? u64 pos, old;
>>> +??? bool cleared;
>>> ????? int i, ret;
>>> ? ????? skip_odd = qcow2->ext_l2 && wbd->lx_level == L2_LEVEL;
>>> @@ -1317,6 +1318,8 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>>> ????? for_each_set_bit(i, wbd->changed_indexes, LX_INDEXES_PER_PAGE) {
>>> ????????? pos = get_u64_from_be_page(wbd->md->page, i);
>>> ? +??????? cleared = !(pos & ~(u64)L2_READS_ALL_ZEROES);
>>> +
>>> ????????? /* Here we restore prealloced and compressed clu mappings */
>>> ????????? pe_page = wbd->pe_page;
>>> ????????? if (pe_page) { /* Only L2 has this. */
>>> @@ -1330,7 +1333,7 @@ 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)
>>> +??????? if (cleared)
>>> ????????????? continue; /* no cluster was allocated */
>>> ? ????????? spin_unlock(&qcow2->md_pages_lock);
>>> @@ -3368,12 +3371,16 @@ static bool qio_discard_updates_metadata(struct qcow2 *qcow2, struct qio *qio,
>>> ?? *
>>> ?? * Discard covering the whole cluster replaces the entry and unuses
>>> ?? * the discarded (or COW source) cluster.
>>> + * If the backing is present, set 'reads as zeroes' to avoid exposing
>>> + * stale data.
>>> ?? */
>>> ? static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>>> ???????????????????? struct qcow2_map *map)
>>> ? {
>>> +??? bool zeroes = maybe_mapped_in_lower_delta(qcow2, *qio);
>>> ????? u32 index_in_page = map->l2.index_in_page;
>>> ????? struct md_page *md = map->l2.md;
>>> +??? u64 new_ext_l2 = map->ext_l2;
>>> ????? loff_t unuse_pos, unuse_end;
>>> ????? struct qio_ext *ext;
>>> ????? int ret;
>>> @@ -3399,6 +3406,8 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>>> ????????? unuse_pos = map->data_clu_pos;
>>> ????????? unuse_end = map->data_clu_pos + qcow2->clu_size;
>>> ????? }
>>> +??? if (zeroes && qcow2->ext_l2)
>>> +??????? new_ext_l2 = (u64)U32_MAX << 32;
>>> ? ????? ret = prepare_l_entry_replace(qcow2, map, *qio, md, index_in_page,
>>> ??????????????????????? unuse_pos, unuse_end, L2_LEVEL);
>>> @@ -3408,6 +3417,10 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>>> ????? ext = (*qio)->ext;
>>> ????? ext->lx_md = md;
>>> ? +??? ext->new_ext_l2 = new_ext_l2;
>>> +??? if (zeroes && !qcow2->ext_l2)
>>> +??????? ext->set_all_zeroes = true;
>>> +
>>> ????? (*qio)->flags |= QIO_IS_DISCARD_FL;
>>> ????? return 1;
>>> ? }
>>> @@ -4050,6 +4063,7 @@ static void process_indexes_write(struct qcow2 *qcow2,
>>> ????? struct qio *qio;
>>> ????? bool discard;
>>> ????? u32 arg_mask;
>>> +??? u64 entry;
>>> ????? int ret;
>>> ? ????? while (1) {
>>> @@ -4074,11 +4088,15 @@ static void process_indexes_write(struct qcow2 *qcow2,
>>> ????????????? goto set_ext_l2;
>>> ????????? }
>>> ? +??????? entry = ext->allocated_clu_pos;
>>> +??????? if (unlikely(ext->set_all_zeroes))
>>> +??????????? entry = L2_READS_ALL_ZEROES;
>>> +
>>> ????????? /* XXX: check prealloced_pos ==> revert */
>>> ????????? ret = prepare_l_entry_update(qcow2, qio, lx_md,
>>> ?????????????????????????? ext->lx_index_in_page,
>>> -???????????????????????? &ext->allocated_clu_pos,
>>> -???????????????????????? arg_mask, ext->lx_level);
>>> +???????????????????????? &entry, arg_mask,
>>> +???????????????????????? ext->lx_level);
>>> ????????? if (ret < 0) {
>>> ????????????? qio->bi_status = errno_to_blk_status(ret);
>>> ????????????? qio_endio(qio);
>>> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
>>> index 8a24e04130e4d..0f006f1ae48cc 100644
>>> --- a/drivers/md/dm-qcow2.h
>>> +++ b/drivers/md/dm-qcow2.h
>>> @@ -312,6 +312,7 @@ struct qio_ext {
>>> ????? u64 new_ext_l2;
>>> ????? u32 cow_mask;
>>> ????? bool only_set_ext_l2:1;
>>> +??? bool set_all_zeroes:1;
>>> ? ????? u8 lx_level;
>>> ? 
>>
> 

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

Re: [Devel] [PATCH VZ10 v2 09/10] drivers/md/dm-qcow2: allow shared L1 entries during merge from RO image

[Pavel Tikhomirov]

On 8/12/26 22:20, Andrey Zhadchenko wrote:
> For RO images with internal snapshots L1 entries are shared.
> Write-mode metadata parsing stops at such entries to avoid
> modifying a shared L2 table, which made prepare_backward_merge()
> see the cluster as unallocated and skip it with
> "nothing to merge": the merged result would silently lose all
> data under shared L1 entries.
> Allow such scenario in parse_l1() and ease WARN in
> prepare_backward_merge().
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-138288
> Feature: dm-qcow2: block device over QCOW2 files driver
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
> ---
>  drivers/md/dm-qcow2-cmd.c |  2 --
>  drivers/md/dm-qcow2-map.c | 18 ++++++++++++++++--
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
> index f2c8f51e17af6..fa3762c58372b 100644
> --- a/drivers/md/dm-qcow2-cmd.c
> +++ b/drivers/md/dm-qcow2-cmd.c
> @@ -266,8 +266,6 @@ static int qcow2_merge_backward_start(struct qcow2_target *tgt, int efd, u32 dep
>  		return -ENOENT;
>  	if (!qcow2_file_is_writable(lower))
>  		return -EACCES;
> -	if (!qcow2_file_is_writable(qcow2) && qcow2->hdr.nb_snapshots)
> -		return -EACCES;

Having all these add in one patch remove in next one blocks does not feel right.
Let's try to merge all fixups in final version.

>  	if (qcow2->clu_size != lower->clu_size)
>  		return -EOPNOTSUPP;
>  	if (lower->hdr.size < qcow2->hdr.size)
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index ebe68e8d0d3d8..baa6778f1686c 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -1726,6 +1726,14 @@ static bool qio_is_fully_alloced(struct qcow2 *qcow2, struct qio *qio,
>  	return !(subclus_mask & ~alloced_mask);
>  }
>  
> +static bool qio_may_modify_image(struct qcow2 *qcow2, struct qio *qio)
> +{
> +	if (qcow2_file_is_writable(qcow2))
> +		return true;
> +
> +	return !fake_merge_qio(qio);
> +}
> +
>  static loff_t parse_l1(struct qcow2 *qcow2, struct qcow2_map *map,
>  		       struct qio **qio, bool write)
>  
> @@ -1762,7 +1770,8 @@ static loff_t parse_l1(struct qcow2 *qcow2, struct qcow2_map *map,
>  		goto out;
>  	if (delay_if_dirty(qcow2, l1->md, l1->index_in_page, qio))
>  		goto out;
> -	if (write && map->clu_is_cow)
> +	/* Don't refuse L1 parse for merge qios with readonly disks */
> +	if (write && map->clu_is_cow && qio_may_modify_image(qcow2, *qio))
>  		goto out; /* Avoid to return pos */
>  
>  	ret = pos;
> @@ -2061,6 +2070,10 @@ static int parse_metadata(struct qcow2 *qcow2, struct qio **qio,
>  	      qio_discard_unmaps_cluster(qcow2, *qio, map)))
>  		return 0;
>  
> +	/* Don't need refcount table if we don't modify the image */
> +	if (!qio_may_modify_image(qcow2, *qio))
> +		return 0;
> +
>  	/* Now refcounters table/block */
>  	ret = qcow2_handle_r1r2_maps(qcow2, pos, qio, &map->r1,
>  			       &map->r2, map->compressed);
> @@ -2749,7 +2762,8 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
>  	}
>  
>  	if (!map->data_clu_alloced) {
> -		WARN_ON_ONCE(map->clu_is_cow); /* Strange COW at L1 */
> +		/* Strange COW at L1, except the merge from RO image */
> +		WARN_ON_ONCE(map->clu_is_cow && qio_may_modify_image(qcow2, *qio));
>  		if (fake_merge_qio(*qio)) {
>  			/* Nothing is to merge */
>  			goto endio;

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

Re: [Devel] [PATCH VZ10 v2 09/10] drivers/md/dm-qcow2: allow shared L1 entries during merge from RO image

[Pavel Tikhomirov]

On 8/21/26 12:43, Pavel Tikhomirov wrote:
> 
> 
> On 8/12/26 22:20, Andrey Zhadchenko wrote:
>> For RO images with internal snapshots L1 entries are shared.
>> Write-mode metadata parsing stops at such entries to avoid
>> modifying a shared L2 table, which made prepare_backward_merge()
>> see the cluster as unallocated and skip it with
>> "nothing to merge": the merged result would silently lose all
>> data under shared L1 entries.
>> Allow such scenario in parse_l1() and ease WARN in
>> prepare_backward_merge().
>>
>> https://virtuozzo.atlassian.net/browse/VSTOR-138288
>> Feature: dm-qcow2: block device over QCOW2 files driver
>> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>> ---
>>  drivers/md/dm-qcow2-cmd.c |  2 --
>>  drivers/md/dm-qcow2-map.c | 18 ++++++++++++++++--
>>  2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
>> index f2c8f51e17af6..fa3762c58372b 100644
>> --- a/drivers/md/dm-qcow2-cmd.c
>> +++ b/drivers/md/dm-qcow2-cmd.c
>> @@ -266,8 +266,6 @@ static int qcow2_merge_backward_start(struct qcow2_target *tgt, int efd, u32 dep
>>  		return -ENOENT;
>>  	if (!qcow2_file_is_writable(lower))
>>  		return -EACCES;
>> -	if (!qcow2_file_is_writable(qcow2) && qcow2->hdr.nb_snapshots)
>> -		return -EACCES;
> 
> Having all these add in one patch remove in next one blocks does not feel right.
> Let's try to merge all fixups in final version.

Sorry, maybe I'm wrong, and it's more of a separate feature here again.

> 
>>  	if (qcow2->clu_size != lower->clu_size)
>>  		return -EOPNOTSUPP;
>>  	if (lower->hdr.size < qcow2->hdr.size)
>> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
>> index ebe68e8d0d3d8..baa6778f1686c 100644
>> --- a/drivers/md/dm-qcow2-map.c
>> +++ b/drivers/md/dm-qcow2-map.c
>> @@ -1726,6 +1726,14 @@ static bool qio_is_fully_alloced(struct qcow2 *qcow2, struct qio *qio,
>>  	return !(subclus_mask & ~alloced_mask);
>>  }
>>  
>> +static bool qio_may_modify_image(struct qcow2 *qcow2, struct qio *qio)
>> +{
>> +	if (qcow2_file_is_writable(qcow2))
>> +		return true;
>> +
>> +	return !fake_merge_qio(qio);
>> +}
>> +
>>  static loff_t parse_l1(struct qcow2 *qcow2, struct qcow2_map *map,
>>  		       struct qio **qio, bool write)
>>  
>> @@ -1762,7 +1770,8 @@ static loff_t parse_l1(struct qcow2 *qcow2, struct qcow2_map *map,
>>  		goto out;
>>  	if (delay_if_dirty(qcow2, l1->md, l1->index_in_page, qio))
>>  		goto out;
>> -	if (write && map->clu_is_cow)
>> +	/* Don't refuse L1 parse for merge qios with readonly disks */
>> +	if (write && map->clu_is_cow && qio_may_modify_image(qcow2, *qio))
>>  		goto out; /* Avoid to return pos */
>>  
>>  	ret = pos;
>> @@ -2061,6 +2070,10 @@ static int parse_metadata(struct qcow2 *qcow2, struct qio **qio,
>>  	      qio_discard_unmaps_cluster(qcow2, *qio, map)))
>>  		return 0;
>>  
>> +	/* Don't need refcount table if we don't modify the image */
>> +	if (!qio_may_modify_image(qcow2, *qio))
>> +		return 0;
>> +
>>  	/* Now refcounters table/block */
>>  	ret = qcow2_handle_r1r2_maps(qcow2, pos, qio, &map->r1,
>>  			       &map->r2, map->compressed);
>> @@ -2749,7 +2762,8 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
>>  	}
>>  
>>  	if (!map->data_clu_alloced) {
>> -		WARN_ON_ONCE(map->clu_is_cow); /* Strange COW at L1 */
>> +		/* Strange COW at L1, except the merge from RO image */
>> +		WARN_ON_ONCE(map->clu_is_cow && qio_may_modify_image(qcow2, *qio));
>>  		if (fake_merge_qio(*qio)) {
>>  			/* Nothing is to merge */
>>  			goto endio;
> 

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

Re: [Devel] [PATCH VZ10 v2 00/10] dm-qcow2: improve discard and read-only merge handling

[Pavel Tikhomirov]

Looks good.

It's hard to understand it completely, but in general it ok. No obvious
code problems, no obvious logical problems.

Note that AI trips on some patches not passing check-patch, some minor
reformatting needed.

Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

On 8/12/26 22:19, Andrey Zhadchenko wrote:
> This series makes discard update qcow2 metadata and permits backward
> merge from read-only source images.
> 
> First patch improves revert_cluster_alloc: this function didn't expect
> L2 entries to contain subcluster descriptions (when ext2_l2 is set).
> 
> The second patch renames COW machinery to a more general 'replace entry'.
> 
> Next five patches improve discard support in general: now it clears
> metadata and have a subcluster granularity.
> 
> The next two patches support backward merge when the image we merge
> from is read-only. Such merges now leave the source mappings and
> metadata unchanged. Previously the module ignored file mode and
> submitted writes anyway.
> 
> Last patch makes merge respect all metadata-zeroed blocks. For now it is
> writing zeroes as data, but we will improve this to metadata when we
> support REQ_OP_WRITE_ZEROES.
> 
> 
> v2:
>  - patch#6: expand qio_discard_updates_metadata() condition to return
> true when backing file is present and fully covered sublcu zero bit isn't
> already set (prepare_cluster_discard() already handled this case but
> if forgot about it here)
> 
> Andrey Zhadchenko (10):
>   drivers/md/dm-qcow2: fix revert_cluster_alloc() for ext_l2 case
>   drivers/md/dm-qcow2: generalize COW index update machinery
>   drivers/md/dm-qcow2: update metadata on whole cluster discard
>   drivers/md/dm-qcow2: never trigger COW or allocation on discard
>   drivers/md/dm-qcow2: set L2_READS_ALL_ZEROES after some discards
>   drivers/md/dm-qcow2: update metadata on subclusters discard
>   drivers/md/dm-qcow2: unmap cluster when discard clears last allocated
>     subclusters
>   drivers/md/dm-qcow2: do discards during backward merge only for
>     writable image
>   drivers/md/dm-qcow2: allow shared L1 entries during merge from RO
>     image
>   drivers/md/dm-qcow2: respect zeroes during merge
> 
>  drivers/md/dm-qcow2-cmd.c    |  21 +-
>  drivers/md/dm-qcow2-map.c    | 390 ++++++++++++++++++++++++++++-------
>  drivers/md/dm-qcow2-target.c |   2 +-
>  drivers/md/dm-qcow2.h        |  19 +-
>  4 files changed, 337 insertions(+), 95 deletions(-)
> 

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