[Devel] [PATCH hci-8.1 v2 0/4] dm-ploop: fix acknowledged writes lost when discard and allocation share a BAT writeback

[Devel] [PATCH hci-8.1 v2 0/4] dm-ploop: fix acknowledged writes lost when discard and allocation share a BAT writeback

[Denis V. Lunev]

The hci-ploop fstrim test caught dm-ploop losing an acknowledged write
(VSTOR-151979). Since "dm-ploop: fix and rework md updates", an
allocation and a discard on the same BAT page can share one pending
index writeback, and its completion applies the written page by the
writeback's type only. A first write that rides in a DISCARD writeback
is on disk but never reaches the local BAT: it reads back as zeroes,
the next write allocates the cluster again and the data is gone. A
discard that rides in an ALLOC writeback leaves the local BAT mapped
while disk has zero, so writes after it are lost on the next open.
fstrim or guest TRIM racing ordinary writes is enough.

Patch 4 is the fix: the completion applies the changes of the pios
attached to the writeback, and of the relocation done on grow, instead
of filtering the page by type. Patches 1 and 2 fix older bugs on the
same path that the old completion partly hid: a failed BAT writeback
freed the wrong clusters, and the online merge of the latest snapshot
left BAT levels above the top one. Patch 3 drops the dead PLOOP_DELAYWB
code.

Tested on dev-cl04, 6.12.0-211.52.1.22.9.vz10 with the module built
out of tree:
- a reproducer racing cluster discards with first writes on one BAT
  page, 512 clusters a round: without the series 3 of 15360 first
  writes read back as zeroes right after the write returned and stayed
  lost after remount; with a 200 us sleep before bat_lock in the
  allocation path 1801 of 2560; the reverse order, a sleep in the
  discard path, left data written after a discard unmapped on disk.
  With the series nothing is lost in either order;
- the ploop pytest suite, ploop and qcow2 formats, passes;
- nothing exercises a failing BAT writeback, that path is checked by
  reading only.

Changes in v2, after Andrey's review:
- 2/4: also clear the deltas[] slot the top delta moved out of, as
  notify_delta_merged() does. The md_lock in the renumbering stays: it
  is per page, and notify_delta_merged() takes it the same way;
- 3/4: new, drops every PLOOP_DELAYWB block instead of only the one in
  the completion;
- 4/4: the PLOOP_DELAYWB removal moved to 3/4, otherwise unchanged;
- Reviewed-by added to 1/4, 2/4 and 4/4.
The ploop pytest suite and the reproducer were rerun on v2.

v1: http://lore.virtuozzo.com/kernel/20261027163300.2166639-1-den@openvz.org/

Denis V. Lunev (4):
  dm-ploop: fix freeing the wrong clusters when a BAT writeback fails
  dm-ploop: renumber BAT levels when the latest snapshot is merged
  dm-ploop: drop the dead PLOOP_DELAYWB code
  dm-ploop: fix lost BAT updates when alloc and discard share a
    writeback

 drivers/md/dm-ploop-cmd.c |  25 +++++-
 drivers/md/dm-ploop-map.c | 161 +++++++++++++++++---------------------
 drivers/md/dm-ploop.h     |   1 +
 3 files changed, 95 insertions(+), 92 deletions(-)


base-commit: b499efa17aa9fb8f20512d1831d1cb61b22e60d3
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 v2 1/4] dm-ploop: fix freeing the wrong clusters when a BAT writeback fails

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

When an index writeback fails, ploop_advance_local_after_bat_wb()
gives the clusters it allocated back to holes_bitmap. It passes
i + off, the virtual cluster number, to ploop_hole_set_bit(). Since
"dm-ploop: make new allocations immediately visible in BAT" it does
so for every mapped entry of the page, including the ones copied from
the local BAT that this writeback did not change.

This marks as free image clusters that are still in use: data
clusters of other virtual clusters, and also the clusters holding the
header and the BAT, since small virtual cluster numbers fall there.
Later allocations hand them out again, so a guest write can land on
live data or on the BAT, while the clusters really allocated for the
failed writeback stay leaked.

Free the image cluster the entry points to, and only for entries this
writeback changed: those differing from the local BAT, and those the
local BAT maps from a lower delta, which a COW replaced.

Fixes: 6ddad5a4d226 ("dm-ploop: Add ploop target driver")
https://virtuozzo.atlassian.net/browse/VSTOR-151979

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

Feature: dm-ploop: ploop target driver
---
 drivers/md/dm-ploop-map.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 48466435dd19..34bf6487107f 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -868,8 +868,9 @@ static void ploop_advance_local_after_bat_wb(struct ploop *ploop,
 		if (success) {
 			WRITE_ONCE(bat_entries[i], READ_ONCE(dst_clu[i]));
 			WRITE_ONCE(md->bat_levels[i], ploop_top_level(ploop));
-		} else {
-			ploop_hole_set_bit(i + off, ploop);
+		} else if (dst_clu[i] != bat_entries[i] ||
+			   md->bat_levels[i] < ploop_top_level(ploop)) {
+			ploop_hole_set_bit(dst_clu[i], ploop);
 		}
 	}
 #ifdef PLOOP_DELAYWB
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 v2 2/4] dm-ploop: renumber BAT levels when the latest snapshot is merged

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

ploop_merge_latest_snapshot() drops the delta below the top one and
moves the top delta down a level, but leaves bat_levels alone. Every
cluster of the top delta then sits one level above the new top.

Checks done with "< top" do not notice, but the flip of the upper
deltas compares levels for equality and leaves these entries alone.
After the flip they are still taken for the top delta, so their I/O
goes to the delta that has just become the top one, at offsets that
belong to the other file. An ALLOC or RELOC writeback resets the
level of every entry it writes back, which hid this for the pages
that happened to get one.

Lower the levels above the dropped delta and clear the slot the top
delta moved out of, as notify_delta_merged() does for a delta merged
from userspace.

Fixes: 6ddad5a4d226 ("dm-ploop: Add ploop target driver")
https://virtuozzo.atlassian.net/browse/VSTOR-151979

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

Feature: dm-ploop: ploop target driver
---
 drivers/md/dm-ploop-cmd.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
index 5fa78e6cff9a..b3d96bb657ed 100644
--- a/drivers/md/dm-ploop-cmd.c
+++ b/drivers/md/dm-ploop-cmd.c
@@ -721,6 +721,27 @@ static int ploop_process_merge_latest_snapshot(struct ploop *ploop)
 }
 ALLOW_ERROR_INJECTION(ploop_process_merge_latest_snapshot, ERRNO);
 
+static void ploop_renumber_levels_above(struct ploop *ploop, u8 level)
+{
+	u32 i, end, *bat_entries;
+	struct md_page *md;
+	struct rb_node *node;
+	unsigned long flags;
+
+	ploop_for_each_md_page(ploop, md, node) {
+		ploop_init_be_iter(ploop, md->id, &i, &end);
+		bat_entries = md->kmpage;
+
+		spin_lock_irqsave(&md->md_lock, flags); /* write */
+		for (; i <= end; i++) {
+			if (READ_ONCE(bat_entries[i]) != BAT_ENTRY_NONE &&
+			    READ_ONCE(md->bat_levels[i]) > level)
+				md->bat_levels[i]--;
+		}
+		spin_unlock_irqrestore(&md->md_lock, flags);
+	}
+}
+
 static int ploop_merge_latest_snapshot(struct ploop *ploop)
 {
 	u8 level;
@@ -747,7 +768,9 @@ static int ploop_merge_latest_snapshot(struct ploop *ploop)
 	ploop_put_delta(ploop, &ploop->deltas[level]);
 
 	ploop->deltas[level] = ploop->deltas[level + 1];
-	ploop->nr_deltas--;
+	memset(&ploop->deltas[--ploop->nr_deltas], 0,
+	       sizeof(struct ploop_delta));
+	ploop_renumber_levels_above(ploop, level);
 
 	ploop_resume_submitting_pios(ploop);
 out:
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 v2 3/4] dm-ploop: drop the dead PLOOP_DELAYWB code

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

"dm-ploop: disable metadata writeback delay" fenced the code that
published new mappings in the local BAT before their index writeback
completed with #ifdef PLOOP_DELAYWB, because the delay hung pios in
the md wait list. Nothing defines the macro, so the code is not built,
and the blocks keep describing a scheme the rest of the driver no
longer follows. Remove them.

https://virtuozzo.atlassian.net/browse/VSTOR-151979

Signed-off-by: Denis V. Lunev <den@openvz.org>

Feature: dm-ploop: ploop target driver
---
 drivers/md/dm-ploop-map.c | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 34bf6487107f..2f8abfa8905b 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -840,10 +840,6 @@ static void ploop_advance_local_after_bat_wb(struct ploop *ploop,
 
 	/* holes bit map requires bat_lock */
 	spin_lock(&md->md_lock);
-#ifdef PLOOP_DELAYWB
-	if (piwb->type == PIWB_TYPE_ALLOC)
-		goto skip_apply;
-#endif
 	for (; i < last; i++) {
 		if (piwb->type == PIWB_TYPE_DISCARD) {
 			/* discard completed */
@@ -873,9 +869,6 @@ static void ploop_advance_local_after_bat_wb(struct ploop *ploop,
 			ploop_hole_set_bit(dst_clu[i], ploop);
 		}
 	}
-#ifdef PLOOP_DELAYWB
-skip_apply:
-#endif
 
 	WARN_ON_ONCE(!test_bit(MD_WRITEBACK, &md->status));
 
@@ -1347,10 +1340,6 @@ static int ploop_alloc_cluster(struct ploop *ploop, struct ploop_index_wb *piwb,
 	map_index_t *to;
 	int ret = 0;
 	u32 tmp_clu;
-#ifdef PLOOP_DELAYWB
-	unsigned long flags;
-	map_index_t *tomd;
-#endif
 
 	/* Cluster index related to the page[page_id] start */
 	clu -= piwb->page_id * PAGE_SIZE / sizeof(map_index_t) - PLOOP_MAP_OFFSET;
@@ -1371,13 +1360,6 @@ static int ploop_alloc_cluster(struct ploop *ploop, struct ploop_index_wb *piwb,
 		goto out;
 
 	WRITE_ONCE(to[clu], *dst_clu);
-#ifdef PLOOP_DELAYWB
-	spin_lock_irqsave(&piwb->md->md_lock, flags);
-	tomd = piwb->md->kmpage;
-	WRITE_ONCE(tomd[clu], *dst_clu);
-	WRITE_ONCE(piwb->md->bat_levels[clu], ploop_top_level(ploop));
-	spin_unlock_irqrestore(&piwb->md->md_lock, flags);
-#endif
 out:
 	kunmap_local(to);
 	return ret;
@@ -1686,15 +1668,6 @@ static void ploop_submit_cow_index_wb(struct ploop_cow *cow)
 	WRITE_ONCE(to[clu], cow->dst_clu);
 	kunmap_local(to);
 
-#ifdef PLOOP_DELAYWB
-	spin_lock_irqsave(&md->md_lock, flags);
-	to = md->kmpage;
-	WRITE_ONCE(to[clu], cow->dst_clu);
-	WRITE_ONCE(md->bat_levels[clu], ploop_top_level(ploop));
-	spin_unlock_irqrestore(&md->md_lock, flags);
-
-	ploop_md_up_prio(ploop, md);
-#endif
 	/* Prevent double clearing of holes_bitmap bit on complete_cow() */
 	cow->dst_clu = BAT_ENTRY_NONE;
 
@@ -1810,9 +1783,7 @@ static bool ploop_locate_new_cluster_and_attach_pio(struct ploop *ploop,
 		goto error;
 	}
 
-#ifndef PLOOP_DELAYWB
 	ploop_attach_end_action(pio, piwb);
-#endif
 	attached = true;
 	if (add_to_wblist)
 		ploop_add_dirty_for_wb(ploop, md);
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 v2 4/4] dm-ploop: fix lost BAT updates when alloc and discard share a writeback

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

Since "dm-ploop: fix and rework md updates" any update of a BAT page
joins the pending index writeback of that page, whatever type it was
prepared with. On completion ploop_advance_local_after_bat_wb() still
applied the written page by type: a DISCARD writeback copied back only
entries that became zero, an ALLOC one only non-zero entries.

A first write to a cluster that joined a pending DISCARD writeback was
acknowledged with its new mapping on disk but not in the local BAT. It
read back as zeroes, the next write to the cluster allocated another
one, and the next writeback of the page stored that, leaking the
cluster with the acknowledged data. A discard that joined an ALLOC
writeback left the local BAT mapped while disk had zero, so data
written to that cluster afterwards was lost on the next open.

Apply the changes of the pios attached to the writeback instead: the
allocating data pios and COW pios set their entries, the discard pios
clear theirs, other entries are left alone. The relocation on grow
changes an entry that no pio stands for, so its writeback records
that entry to be applied the same way. On failure the same rule as
before frees only what this writeback allocated: a data pio may find
its cluster already mapped by an earlier writeback.

Entries nobody changed no longer get their level reset to the top
one; nothing needs that once the merge of the latest snapshot
renumbers the levels itself.

Fixes: ddf38fa44df4 ("dm-ploop: fix and rework md updates")
https://virtuozzo.atlassian.net/browse/VSTOR-151979

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

Feature: dm-ploop: ploop target driver
---
 drivers/md/dm-ploop-map.c | 133 ++++++++++++++++++++------------------
 drivers/md/dm-ploop.h     |   1 +
 2 files changed, 71 insertions(+), 63 deletions(-)

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 2f8abfa8905b..016be97caaa8 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -86,6 +86,7 @@ void ploop_index_wb_init(struct ploop_index_wb *piwb, struct ploop *ploop)
 	piwb->page_id = PAGE_NR_NONE;
 	piwb->type = PIWB_TYPE_ALLOC;
 	piwb->flush_pio = NULL;
+	piwb->reloc_clu = BAT_ENTRY_NONE;
 }
 
 void ploop_init_pio(struct ploop *ploop, unsigned int bi_op, struct pio *pio)
@@ -802,73 +803,85 @@ static void ploop_complete_cow(struct ploop_cow *cow, blk_status_t bi_status)
 	kmem_cache_free(cow_cache, cow);
 }
 
+static void ploop_advance_allocated(struct ploop *ploop, struct md_page *md,
+				    map_index_t *dst_clu, u32 clu, bool success)
+{
+	u32 i = ploop_bat_clu_idx_in_page(clu);
+	u32 *bat_entries = md->kmpage;
+	u32 new_clu = READ_ONCE(dst_clu[i]);
+
+	if (WARN_ON_ONCE(!new_clu))
+		return;
+
+	if (success) {
+		WRITE_ONCE(bat_entries[i], new_clu);
+		WRITE_ONCE(md->bat_levels[i], ploop_top_level(ploop));
+	} else if (new_clu != READ_ONCE(bat_entries[i]) ||
+		   md->bat_levels[i] < ploop_top_level(ploop)) {
+		ploop_hole_set_bit(new_clu, ploop);
+	}
+}
+
+static void ploop_advance_discarded(struct ploop *ploop, struct md_page *md,
+				    map_index_t *dst_clu, u32 clu, bool success)
+{
+	u32 i = ploop_bat_clu_idx_in_page(clu);
+	u32 *bat_entries = md->kmpage;
+	u8 level = md->bat_levels[i];
+	u32 d_clu = READ_ONCE(bat_entries[i]);
+
+	if (!success || dst_clu[i] || d_clu == BAT_ENTRY_NONE ||
+	    level < ploop_top_level(ploop))
+		return;
+
+	WARN_ON_ONCE(ploop->nr_deltas != 1);
+	WRITE_ONCE(bat_entries[i], BAT_ENTRY_NONE);
+	WRITE_ONCE(md->bat_levels[i], 0);
+	ploop_hole_set_bit(d_clu, ploop);
+}
+
 /*
- * Update local BAT copy with written indexes on success.
- * Mark allocate clusters as holes on failure.
+ * Apply to the local BAT copy the changes made by the pios attached
+ * to @piwb, whatever type @piwb was prepared with: allocations and
+ * discards of one page may share a writeback.
  * FIXME: a failure may mean some sectors are written, so
  * we have to reread BAT page to check that.
  */
 static void ploop_advance_local_after_bat_wb(struct ploop *ploop,
 					     struct ploop_index_wb *piwb,
+					     struct llist_node *ll_ready_pios,
+					     struct llist_node *ll_cow_pios,
 					     bool success)
 {
 	struct md_page *md = piwb->md;
-	u32 i, last, *bat_entries;
-	map_index_t *dst_clu, off;
+	map_index_t *dst_clu;
 	unsigned long flags;
 	LIST_HEAD(list);
 	struct llist_node *wait_llist_pending;
+	struct ploop_cow *cow;
 	struct pio *pio;
 	struct llist_node *pos, *t;
 	BUG_ON(!md);
-	bat_entries = md->kmpage;
-
-	/* Absolute number of first index in page (negative for page#0) */
-	off = piwb->page_id * PAGE_SIZE / sizeof(map_index_t);
-	off -= PLOOP_MAP_OFFSET;
 
 	spin_lock_irqsave(&ploop->bat_lock, flags);
-	/* Last and first index in copied page */
-	last = READ_ONCE(ploop->nr_bat_entries) - off;
-	if (last > PAGE_SIZE / sizeof(map_index_t))
-		last = PAGE_SIZE / sizeof(map_index_t);
-	i = 0;
-	if (!piwb->page_id)
-		i = PLOOP_MAP_OFFSET;
-
 	dst_clu = kmap_local_page(piwb->bat_page);
 
 	/* holes bit map requires bat_lock */
 	spin_lock(&md->md_lock);
-	for (; i < last; i++) {
-		if (piwb->type == PIWB_TYPE_DISCARD) {
-			/* discard completed */
-			u8 level = md->bat_levels[i];
-			u32 d_clu = READ_ONCE(bat_entries[i]);
-
-			if (success && !dst_clu[i] &&
-			    (!(d_clu == BAT_ENTRY_NONE ||
-			    level < ploop_top_level(ploop)))) {
-				WARN_ON_ONCE(ploop->nr_deltas != 1);
-				WRITE_ONCE(bat_entries[i], BAT_ENTRY_NONE);
-				WRITE_ONCE(md->bat_levels[i], 0);
-				ploop_hole_set_bit(d_clu, ploop);
-			}
-
-			continue;
-		}
-
-		if (!dst_clu[i])
-			continue;
-
-		if (success) {
-			WRITE_ONCE(bat_entries[i], READ_ONCE(dst_clu[i]));
-			WRITE_ONCE(md->bat_levels[i], ploop_top_level(ploop));
-		} else if (dst_clu[i] != bat_entries[i] ||
-			   md->bat_levels[i] < ploop_top_level(ploop)) {
-			ploop_hole_set_bit(dst_clu[i], ploop);
-		}
+	llist_for_each(pos, ll_ready_pios) {
+		pio = list_entry((struct list_head *)pos, typeof(*pio), list);
+		if (pio->wants_discard_index_cleanup)
+			ploop_advance_discarded(ploop, md, dst_clu, pio->clu, success);
+		else if (pio->is_data_alloc)
+			ploop_advance_allocated(ploop, md, dst_clu, pio->clu, success);
 	}
+	llist_for_each(pos, ll_cow_pios) {
+		pio = list_entry((struct list_head *)pos, typeof(*pio), list);
+		cow = pio->endio_cb_data;
+		ploop_advance_allocated(ploop, md, dst_clu, cow->cow_pio->clu, success);
+	}
+	if (piwb->reloc_clu != BAT_ENTRY_NONE)
+		ploop_advance_allocated(ploop, md, dst_clu, piwb->reloc_clu, success);
 
 	WARN_ON_ONCE(!test_bit(MD_WRITEBACK, &md->status));
 
@@ -941,24 +954,6 @@ static void ploop_bat_write_finish(struct pio *pio, void *piwb_ptr,
 	struct llist_node *ll_cow_pios;
 	struct llist_node *ll_ready_pios;
 
-	if (!bi_status) {
-		/*
-		 * Success: now update local BAT copy. We could do this
-		 * from our delayed work, but we want to publish new
-		 * mapping in the fastest way. This must be done before
-		 * data bios completion, since right after we complete
-		 * a bio, subsequent read wants to see written data
-		 * (ploop_map() wants to see not zero bat_entries[.]).
-		 */
-		ploop_advance_local_after_bat_wb(ploop, piwb, true);
-	} else {
-		/*
-		 * Index wb failed. Mark clusters as unallocated again.
-		 * piwb->count is zero, so all data writers compeleted.
-		 */
-		ploop_advance_local_after_bat_wb(ploop, piwb, false);
-	}
-
 	spin_lock_irqsave(&piwb->lock, flags);
 	ll_ready_pios = llist_del_all(&piwb->llready_data_pios);
 	if (bi_status)
@@ -966,6 +961,17 @@ static void ploop_bat_write_finish(struct pio *pio, void *piwb_ptr,
 	spin_unlock_irqrestore(&piwb->lock, flags);
 	ll_cow_pios = llist_reverse_order(llist_del_all(&piwb->cow_llist));
 
+	/*
+	 * On success publish the new mapping before the data bios
+	 * complete, since right after we complete a bio, subsequent
+	 * read wants to see written data (ploop_map() wants to see
+	 * not zero bat_entries[.]). On failure mark the clusters
+	 * allocated for this writeback as holes again: piwb->count
+	 * is zero, so all data writers completed.
+	 */
+	ploop_advance_local_after_bat_wb(ploop, piwb, ll_ready_pios,
+					 ll_cow_pios, !bi_status);
+
 	/*
 	 * End pending data bios.
 	 */
@@ -2708,6 +2714,7 @@ int ploop_prepare_reloc_index_wb(struct ploop *ploop,
 		err = ploop_alloc_cluster(ploop, piwb, clu, dst_clu, file);
 		if (err)
 			goto out_reset;
+		piwb->reloc_clu = clu;
 	}
 
 	*ret_md = md;
diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
index 23662943d56a..ce834df6df55 100644
--- a/drivers/md/dm-ploop.h
+++ b/drivers/md/dm-ploop.h
@@ -108,6 +108,7 @@ struct ploop_index_wb {
 	blk_status_t bi_status;
 	u32 page_id;
 	struct bio_vec aux_bvec;
+	u32 reloc_clu;
 	struct pio *flush_pio;
 };
 
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 2/4] dm-ploop: renumber BAT levels when the latest snapshot is merged

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> ploop_merge_latest_snapshot() drops the delta below the top one and
> moves the top delta down a level, but leaves bat_levels alone. Every
> cluster of the top delta then sits one level above the new top.
> 
> Checks done with "< top" do not notice, but the flip of the upper
> deltas compares levels for equality and leaves these entries alone.
> After the flip they are still taken for the top delta, so their I/O
> goes to the delta that has just become the top one, at offsets that
> belong to the other file. An ALLOC or RELOC writeback resets the
> level of every entry it writes back, which hid this for the pages
> that happened to get one.
> 
> Lower the levels above the dropped delta and clear the slot the top
> delta moved out of, as notify_delta_merged() does for a delta merged
> from userspace.
> 
> Fixes: 6ddad5a4d226 ("dm-ploop: Add ploop target driver")
> https://virtuozzo.atlassian.net/browse/VSTOR-151979
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028143647.2972030-1-den@openvz.org?part=2


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 1/4] dm-ploop: fix freeing the wrong clusters when a BAT writeback fails

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> When an index writeback fails, ploop_advance_local_after_bat_wb()
> gives the clusters it allocated back to holes_bitmap. It passes
> i + off, the virtual cluster number, to ploop_hole_set_bit(). Since
> "dm-ploop: make new allocations immediately visible in BAT" it does
> so for every mapped entry of the page, including the ones copied from
> the local BAT that this writeback did not change.
> 
> This marks as free image clusters that are still in use: data
> clusters of other virtual clusters, and also the clusters holding the
> header and the BAT, since small virtual cluster numbers fall there.
> Later allocations hand them out again, so a guest write can land on
> live data or on the BAT, while the clusters really allocated for the
> failed writeback stay leaked.
> 
> Free the image cluster the entry points to, and only for entries this
> writeback changed: those differing from the local BAT, and those the
> local BAT maps from a lower delta, which a COW replaced.
> 
> Fixes: 6ddad5a4d226 ("dm-ploop: Add ploop target driver")
> https://virtuozzo.atlassian.net/browse/VSTOR-151979
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028143647.2972030-1-den@openvz.org?part=1


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 3/4] dm-ploop: drop the dead PLOOP_DELAYWB code

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> "dm-ploop: disable metadata writeback delay" fenced the code that
> published new mappings in the local BAT before their index writeback
> completed with #ifdef PLOOP_DELAYWB, because the delay hung pios in
> the md wait list. Nothing defines the macro, so the code is not built,
> and the blocks keep describing a scheme the rest of the driver no
> longer follows. Remove them.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-151979
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028143647.2972030-1-den@openvz.org?part=3


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 4/4] dm-ploop: fix lost BAT updates when alloc and discard share a writeback

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> Since "dm-ploop: fix and rework md updates" any update of a BAT page
> joins the pending index writeback of that page, whatever type it was
> prepared with. On completion ploop_advance_local_after_bat_wb() still
> applied the written page by type: a DISCARD writeback copied back only
> entries that became zero, an ALLOC one only non-zero entries.
> 
> A first write to a cluster that joined a pending DISCARD writeback was
> acknowledged with its new mapping on disk but not in the local BAT. It
> read back as zeroes, the next write to the cluster allocated another
> one, and the next writeback of the page stored that, leaking the
> cluster with the acknowledged data. A discard that joined an ALLOC
> writeback left the local BAT mapped while disk had zero, so data
> written to that cluster afterwards was lost on the next open.
> [ ... ]
> 
> Fixes: ddf38fa44df4 ("dm-ploop: fix and rework md updates")
> https://virtuozzo.atlassian.net/browse/VSTOR-151979
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028143647.2972030-1-den@openvz.org?part=4


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 0/4] dm-ploop: fix acknowledged writes lost when discard and allocation share a BAT writeback

[Andrey Zhadchenko]

Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>


On 10/28/26 15:36, Denis V. Lunev wrote:
> The hci-ploop fstrim test caught dm-ploop losing an acknowledged write
> (VSTOR-151979). Since "dm-ploop: fix and rework md updates", an
> allocation and a discard on the same BAT page can share one pending
> index writeback, and its completion applies the written page by the
> writeback's type only. A first write that rides in a DISCARD writeback
> is on disk but never reaches the local BAT: it reads back as zeroes,
> the next write allocates the cluster again and the data is gone. A
> discard that rides in an ALLOC writeback leaves the local BAT mapped
> while disk has zero, so writes after it are lost on the next open.
> fstrim or guest TRIM racing ordinary writes is enough.
> 
> Patch 4 is the fix: the completion applies the changes of the pios
> attached to the writeback, and of the relocation done on grow, instead
> of filtering the page by type. Patches 1 and 2 fix older bugs on the
> same path that the old completion partly hid: a failed BAT writeback
> freed the wrong clusters, and the online merge of the latest snapshot
> left BAT levels above the top one. Patch 3 drops the dead PLOOP_DELAYWB
> code.
> 
> Tested on dev-cl04, 6.12.0-211.52.1.22.9.vz10 with the module built
> out of tree:
> - a reproducer racing cluster discards with first writes on one BAT
>    page, 512 clusters a round: without the series 3 of 15360 first
>    writes read back as zeroes right after the write returned and stayed
>    lost after remount; with a 200 us sleep before bat_lock in the
>    allocation path 1801 of 2560; the reverse order, a sleep in the
>    discard path, left data written after a discard unmapped on disk.
>    With the series nothing is lost in either order;
> - the ploop pytest suite, ploop and qcow2 formats, passes;
> - nothing exercises a failing BAT writeback, that path is checked by
>    reading only.
> 
> Changes in v2, after Andrey's review:
> - 2/4: also clear the deltas[] slot the top delta moved out of, as
>    notify_delta_merged() does. The md_lock in the renumbering stays: it
>    is per page, and notify_delta_merged() takes it the same way;
> - 3/4: new, drops every PLOOP_DELAYWB block instead of only the one in
>    the completion;
> - 4/4: the PLOOP_DELAYWB removal moved to 3/4, otherwise unchanged;
> - Reviewed-by added to 1/4, 2/4 and 4/4.
> The ploop pytest suite and the reproducer were rerun on v2.
> 
> v1: http://lore.virtuozzo.com/kernel/20261027163300.2166639-1-den@openvz.org/
> 
> Denis V. Lunev (4):
>    dm-ploop: fix freeing the wrong clusters when a BAT writeback fails
>    dm-ploop: renumber BAT levels when the latest snapshot is merged
>    dm-ploop: drop the dead PLOOP_DELAYWB code
>    dm-ploop: fix lost BAT updates when alloc and discard share a
>      writeback
> 
>   drivers/md/dm-ploop-cmd.c |  25 +++++-
>   drivers/md/dm-ploop-map.c | 161 +++++++++++++++++---------------------
>   drivers/md/dm-ploop.h     |   1 +
>   3 files changed, 95 insertions(+), 92 deletions(-)
> 
> 
> base-commit: b499efa17aa9fb8f20512d1831d1cb61b22e60d3

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 0/4] dm-ploop: fix acknowledged writes lost when discard and allocation share a BAT writeback

[Konstantin Khorenko]

committing without review

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 10/28/26 15:36, Denis V. Lunev wrote:
> The hci-ploop fstrim test caught dm-ploop losing an acknowledged write
> (VSTOR-151979). Since "dm-ploop: fix and rework md updates", an
> allocation and a discard on the same BAT page can share one pending
> index writeback, and its completion applies the written page by the
> writeback's type only. A first write that rides in a DISCARD writeback
> is on disk but never reaches the local BAT: it reads back as zeroes,
> the next write allocates the cluster again and the data is gone. A
> discard that rides in an ALLOC writeback leaves the local BAT mapped
> while disk has zero, so writes after it are lost on the next open.
> fstrim or guest TRIM racing ordinary writes is enough.
> 
> Patch 4 is the fix: the completion applies the changes of the pios
> attached to the writeback, and of the relocation done on grow, instead
> of filtering the page by type. Patches 1 and 2 fix older bugs on the
> same path that the old completion partly hid: a failed BAT writeback
> freed the wrong clusters, and the online merge of the latest snapshot
> left BAT levels above the top one. Patch 3 drops the dead PLOOP_DELAYWB
> code.
> 
> Tested on dev-cl04, 6.12.0-211.52.1.22.9.vz10 with the module built
> out of tree:
> - a reproducer racing cluster discards with first writes on one BAT
>   page, 512 clusters a round: without the series 3 of 15360 first
>   writes read back as zeroes right after the write returned and stayed
>   lost after remount; with a 200 us sleep before bat_lock in the
>   allocation path 1801 of 2560; the reverse order, a sleep in the
>   discard path, left data written after a discard unmapped on disk.
>   With the series nothing is lost in either order;
> - the ploop pytest suite, ploop and qcow2 formats, passes;
> - nothing exercises a failing BAT writeback, that path is checked by
>   reading only.
> 
> Changes in v2, after Andrey's review:
> - 2/4: also clear the deltas[] slot the top delta moved out of, as
>   notify_delta_merged() does. The md_lock in the renumbering stays: it
>   is per page, and notify_delta_merged() takes it the same way;
> - 3/4: new, drops every PLOOP_DELAYWB block instead of only the one in
>   the completion;
> - 4/4: the PLOOP_DELAYWB removal moved to 3/4, otherwise unchanged;
> - Reviewed-by added to 1/4, 2/4 and 4/4.
> The ploop pytest suite and the reproducer were rerun on v2.
> 
> v1: http://lore.virtuozzo.com/kernel/20261027163300.2166639-1-den@openvz.org/
> 
> Denis V. Lunev (4):
>   dm-ploop: fix freeing the wrong clusters when a BAT writeback fails
>   dm-ploop: renumber BAT levels when the latest snapshot is merged
>   dm-ploop: drop the dead PLOOP_DELAYWB code
>   dm-ploop: fix lost BAT updates when alloc and discard share a
>     writeback
> 
>  drivers/md/dm-ploop-cmd.c |  25 +++++-
>  drivers/md/dm-ploop-map.c | 161 +++++++++++++++++---------------------
>  drivers/md/dm-ploop.h     |   1 +
>  3 files changed, 95 insertions(+), 92 deletions(-)
> 
> 
> base-commit: b499efa17aa9fb8f20512d1831d1cb61b22e60d3

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 0/4] dm-ploop: fix acknowledged writes lost when discard and allocation share a BAT writeback

[Konstantin Khorenko]

On Wed, 28 Oct 2026 15:36:43 +0100, Denis V. Lunev wrote:
> dm-ploop: fix acknowledged writes lost when discard and allocation share a BAT writeback
> 
> The hci-ploop fstrim test caught dm-ploop losing an acknowledged write
> (VSTOR-151979). Since "dm-ploop: fix and rework md updates", an
> allocation and a discard on the same BAT page can share one pending
> index writeback, and its completion applies the written page by the
> writeback's type only. A first write that rides in a DISCARD writeback
> is on disk but never reaches the local BAT: it reads back as zeroes,
> the next write allocates the cluster again and the data is gone. A
> discard that rides in an ALLOC writeback leaves the local BAT mapped
> while disk has zero, so writes after it are lost on the next open.
> fstrim or guest TRIM racing ordinary writes is enough.
> 
> [...]

Applied to branch-rh10-6.12.0-211.52.1.22.x.vz10-ovz, will be released in rh10-6.12.0-211.52.1.22.15.vz10, thanks!

[1/4] dm-ploop: fix freeing the wrong clusters when a BAT writeback fails
      https://bitbucket.org/virtuozzocore/vzkernel.vzs/commits/e03772371b21
[2/4] dm-ploop: renumber BAT levels when the latest snapshot is merged
      https://bitbucket.org/virtuozzocore/vzkernel.vzs/commits/7c773193cb6b
[3/4] dm-ploop: drop the dead PLOOP_DELAYWB code
      https://bitbucket.org/virtuozzocore/vzkernel.vzs/commits/c69290e4b1f9
[4/4] dm-ploop: fix lost BAT updates when alloc and discard share a writeback
      https://bitbucket.org/virtuozzocore/vzkernel.vzs/commits/5b2cff1af51d

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 0/4] dm-ploop: fix acknowledged writes lost when discard and allocation share a BAT writeback

[Konstantin Khorenko]

Hi Den,

the patches look good to me, no changes needed.

While reviewing them I found a few problems nearby, all older than the
series:

- A failed cluster copy on grow leaves MD_UPDATING and MD_DIRTY set on
  the BAT page. After the resume every allocating write, discard or COW
  on that page waits in wait_llist forever.
  https://virtuozzo.atlassian.net/browse/VSTOR-152981

- ploop_suspend_submitting_pios() does not wait for the allocator
  thread, which can read a deltas[] slot being vacated by a merge and
  oops on a NULL file or truncate the top image. 2/4 adds the same
  memset as notify_delta_merged() to merge_latest, so the window now
  exists there too.
  https://virtuozzo.atlassian.net/browse/VSTOR-152984

- ploop_submit_embedded_pio() checks stop_submitting_pios without
  deferred_lock, so a pio can slip past the suspend of submission or
  get stuck in llsuspended_pios after the resume.
  https://virtuozzo.atlassian.net/browse/VSTOR-152985

--
Best regards,
Konstantin
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel