[Devel] [PATCH vz10 24/24] dm-ploop: fail BAT-writeback waiters instead of retrying forever on I/O error

[Andrey Zhadchenko]

NACK.
Same reasoning as for the [Devel] [PATCH vz10 23/24] dm-qcow2: fail 
waiters instead of retrying forever after metadata I/O error

On 7/6/26 13:00, Konstantin Khorenko wrote:
> ploop_advance_local_after_bat_wb() unconditionally re-dispatched
> md->wait_llist (pios postponed by ploop_delay_if_md_busy() while this BAT
> page was already dirty/under writeback) for retry, regardless of whether the
> writeback that just completed succeeded or failed - the exact same class of
> bug just fixed for dm-qcow2 in commit db5b5d2d30d8 ("dm-qcow2: fail waiters
> instead of retrying forever after metadata I/O error"), found by auditing
> dm-ploop for the analogous pattern after that live repro.
> 
> On a persistently failing backing device, a retried pio re-triggers the same
> BAT-page write, fails the same way, and lands back on md->wait_llist again -
> an unbounded retry storm that leaves the pio's originating I/O blocked in an
> uninterruptible wait forever (unkillable D state), wedging `ploop
> umount`/`dmsetup remove` and eventually `reboot` on the stuck task.
> 
> A failed BAT writeback means the page's on-disk state was not persisted, so
> retrying it as if nothing happened is not safe. Thread the real bi_status
> through (instead of a plain success/failure bool) and fail md->wait_llist
> waiters with it via ploop_pio_endio() on failure, exactly like
> piwb->llready_data_pios (the data bios belonging to this same BAT update) are
> already correctly handled a few lines below. The success path is unchanged.
> 
> Fixes: a9cd5e6dc646 ("dm-ploop: Add ploop target driver")
> Feature: dm-ploop: ploop target driver
> https://virtuozzo.atlassian.net/browse/VSTOR-137234
> Signed-off-by: Konstantin Khorenko <khorenko at virtuozzo.com>
> ---
>   drivers/md/dm-ploop-map.c | 28 ++++++++++++++++++++++++----
>   1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
> index 48466435dd19..af6b95241564 100644
> --- a/drivers/md/dm-ploop-map.c
> +++ b/drivers/md/dm-ploop-map.c
> @@ -810,9 +810,10 @@ static void ploop_complete_cow(struct ploop_cow *cow, blk_status_t bi_status)
>    */
>   static void ploop_advance_local_after_bat_wb(struct ploop *ploop,
>   					     struct ploop_index_wb *piwb,
> -					     bool success)
> +					     blk_status_t bi_status)
>   {
>   	struct md_page *md = piwb->md;
> +	bool success = !bi_status;
>   	u32 i, last, *bat_entries;
>   	map_index_t *dst_clu, off;
>   	unsigned long flags;
> @@ -890,7 +891,26 @@ static void ploop_advance_local_after_bat_wb(struct ploop *ploop,
>   		llist_for_each_safe(pos, t, wait_llist_pending) {
>   			pio = list_entry((struct list_head *)pos, typeof(*pio), list);
>   			INIT_LIST_HEAD(&pio->list);
> -			list_add(&pio->list, &list);
> +			/*
> +			 * These pios arrived while this BAT page's writeback
> +			 * was already in flight and were postponed
> +			 * (ploop_delay_if_md_busy()) to retry their own BAT
> +			 * update once it finished. If it failed, this page's
> +			 * on-disk state was not persisted, so retrying is not
> +			 * safe: on a persistently failing backing device the
> +			 * retried pio would immediately hit the same
> +			 * disk-level failure and land back on this list
> +			 * again - an unbounded retry storm that leaves the
> +			 * pio's (uninterruptible) originator blocked forever.
> +			 * Fail them, mirroring how piwb->llready_data_pios
> +			 * is already handled above.
> +			 */
> +			if (success) {
> +				list_add(&pio->list, &list);
> +			} else {
> +				pio->bi_status = bi_status;
> +				ploop_pio_endio(pio);
> +			}
>   		}
>   	}
>   
> @@ -956,13 +976,13 @@ static void ploop_bat_write_finish(struct pio *pio, void *piwb_ptr,
>   		 * 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);
> +		ploop_advance_local_after_bat_wb(ploop, piwb, bi_status);
>   	} 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);
> +		ploop_advance_local_after_bat_wb(ploop, piwb, bi_status);
>   	}
>   
>   	spin_lock_irqsave(&piwb->lock, flags);