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

[Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>]

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