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

[Devel] [PATCH hci-8.1 0/3] 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 3 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.

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.

Denis V. Lunev (3):
  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: fix lost BAT updates when alloc and discard share a
    writeback

 drivers/md/dm-ploop-cmd.c |  22 ++++++
 drivers/md/dm-ploop-map.c | 139 +++++++++++++++++++-------------------
 drivers/md/dm-ploop.h     |   1 +
 3 files changed, 93 insertions(+), 69 deletions(-)


base-commit: 02401c5e378f282fbf11bdc65996253ff65dad9b
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 1/3] 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>

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 2/3] 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, 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>

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

diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
index 5fa78e6cff9a..cfbba9b3e234 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;
@@ -748,6 +769,7 @@ static int ploop_merge_latest_snapshot(struct ploop *ploop)
 
 	ploop->deltas[level] = ploop->deltas[level + 1];
 	ploop->nr_deltas--;
+	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

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

[Andrey Zhadchenko]

On 10/27/26 17:32, Denis V. Lunev wrote:
> 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, 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>
> 
> Feature: dm-ploop: ploop target driver
> ---
>   drivers/md/dm-ploop-cmd.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
> index 5fa78e6cff9a..cfbba9b3e234 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 */

Not blocking, but I don't think we need spinlock here. What actually 
protects us is that pios are suspended. There are no other users if pios 
are suspended and we are in the command already.
If we weren't suspended we would have another problem with half pages 
with correct level and half with the wrong.
So either delete spinlock or take it outside the cycle.

> +		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;
> @@ -748,6 +769,7 @@ static int ploop_merge_latest_snapshot(struct ploop *ploop)
>   
>   	ploop->deltas[level] = ploop->deltas[level + 1];

We can also zero ploop->deltas[level + 1] just in case so we do not have 
any stale info afterwards.

>   	ploop->nr_deltas--;
> +	ploop_renumber_levels_above(ploop, level);
>   
>   	ploop_resume_submitting_pios(ploop);
>   out:

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

[Devel] [PATCH hci-8.1 3/3] 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. The PLOOP_DELAYWB branch goes with the
old loop; nothing defines that macro.

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>

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

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 34bf6487107f..a637df932495 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,80 +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);
-#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 */
-			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);
 	}
-#ifdef PLOOP_DELAYWB
-skip_apply:
-#endif
+	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));
 
@@ -948,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)
@@ -973,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.
 	 */
@@ -2737,6 +2736,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 3/3] dm-ploop: fix lost BAT updates when alloc and discard share a writeback

[Andrey Zhadchenko]

On 10/27/26 17:32, Denis V. Lunev wrote:
> 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. The PLOOP_DELAYWB branch goes with the
> old loop; nothing defines that macro.
> 
> 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>
> 
> Feature: dm-ploop: ploop target driver
> ---
>   drivers/md/dm-ploop-map.c | 140 +++++++++++++++++++-------------------
>   drivers/md/dm-ploop.h     |   1 +
>   2 files changed, 71 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
> index 34bf6487107f..a637df932495 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,80 +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);
> -#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 */
> -			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);
>   	}
> -#ifdef PLOOP_DELAYWB
> -skip_apply:
> -#endif

Maybe even drop PLOOP_DELAYWB as a separate patch and in other places 
too? Looks like it is dead anyway. And this remove makes it even more 
inconsistent.

> +	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));
>   
> @@ -948,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)
> @@ -973,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.
>   	 */
> @@ -2737,6 +2736,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;
> +	c;
>   	struct pio *flush_pio;
>   };
>   

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

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

[Andrey Zhadchenko]

I have left a few suggestions, but the patches can be merges as-is anyway.

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

On 10/27/26 17:32, 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 3 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.
> 
> 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.
> 
> Denis V. Lunev (3):
>    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: fix lost BAT updates when alloc and discard share a
>      writeback
> 
>   drivers/md/dm-ploop-cmd.c |  22 ++++++
>   drivers/md/dm-ploop-map.c | 139 +++++++++++++++++++-------------------
>   drivers/md/dm-ploop.h     |   1 +
>   3 files changed, 93 insertions(+), 69 deletions(-)
> 
> 
> base-commit: 02401c5e378f282fbf11bdc65996253ff65dad9b

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