[Devel] [PATCH hci-8.1 v12 09/12] dm-qcow2: leak clusters allocated for a failed L1/L2 writeback

[no-reply at sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dm-qcow2: clusters leaked silently after failed L1/L2 writeback, no log or check flag
- [Low] dm-qcow2: qcow2_flush_deferred_activity() two-pass loop lost its rationale
--

commit 9ad1bd4fdc16fdd64c7c406c8964d581b12af819
Author: Alexander Ivanov <alexander.ivanov at virtuozzo.com>

dm-qcow2: leak clusters allocated for a failed L1/L2 writeback

This commit stops revert_l_entries_update() from dropping the refcount of
clusters allocated for a failed L1/L2 writeback. It also clears
FREE_ALLOCATED_CLU for COW qios that end with the writeback error. The
clusters are leaked instead of being freed, so an on-disk L1/L2 entry that
points to one stays valid.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171

> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c

[ ... ]

>  static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>  {
> -	struct qcow2_map_item r1, r2;
[ ... ]
> +	struct page *pe_page = wbd->pe_page;
> +	u64 old;
> +	int i;
>  
>  	lockdep_assert_held(&qcow2->md_pages_lock);
>  	for_each_set_bit(i, wbd->changed_indexes, LX_INDEXES_PER_PAGE) {
[ ... ]
> +		old = pe_page ? get_u64_from_be_page(pe_page, i) : 0;
> +		set_u64_to_be_page(wbd->md->page, i, old);
>  	}
>  }

[ ... ]

> @@ -1408,6 +1373,7 @@ static void complete_wbd(struct qcow2 *qcow2, struct wb_desc *wbd)
[ ... ]
> +		list_for_each_entry(qio, &end_list, link) {
> +			if (qio->ext)
> +				qio->ext->cleanup_mask &= ~FREE_ALLOCATED_CLU;
> +		}
>  		qcow2_dispatch_qios(qcow2, NULL, &wait_list);
>  		end_qios(&end_list, errno_to_blk_status(wbd->ret));

[Severity: Medium]
The commit message says "Check frees them, so the image needs no check and
resume needs no refusal." Is that accurate?

Once revert_l_entries_update() no longer calls mark_cluster_unused(), the
L2-table cluster (for an L1 write) or the data cluster (for an L2 write)
allocated in prepare_l_entry_update() keeps its refcount. The same applies
to COW clusters, because complete_wbd() clears FREE_ALLOCATED_CLU, which
bypasses handle_cleanup_mask()->mark_cluster_unused().

Nothing seems to report this leak. clear_writeback_status() sets
MD_WRITEBACK_ERROR only when !md->wbd, which is not the case on this path.
There is also no QC_ERR message and no counter, so wants_check and the
status output do not tell the operator that a check is needed to reclaim
the space.

With persistent write failures and guest retries, each failed request
leaks one more cluster. Leaked clusters also stay counted as used, so the
used-end tail truncation added later in the series cannot shrink past
them.

Would it make sense to log the leak, or to flag the image so that a check
is requested, and to reword the commit message accordingly?

> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c
> @@ -173,10 +173,6 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
>  	struct md_page *md;
>  	int i;
>  
> -	/*
> -	 * We need second iteration, since revert_l_entries_update() may start timer again after failed wb.
> -	 */
>  	for (i = 0; i < 2; i++) {

[Severity: Low]
This isn't a bug, but the comment explaining why the loop runs twice is
deleted while the loop itself stays.

mark_cluster_unused() is still reached from handle_cleanup_mask() with
FREE_ALLOCATED_CLU, and process_indexes_end() still dirties R2 pages. Both
can re-arm slow_wb_timer after the first pass.

Could the comment be rewritten to state the remaining reason for the second
iteration, rather than removed?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261110075140.1475607-1-alexander.ivanov@virtuozzo.com?part=9