Re: [Devel] [PATCH hci-8.1 v10 08/10] dm-qcow2: leak clusters allocated for a failed L1/L2 writeback

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dm-qcow2: clusters of a failed L1/L2 writeback leak at runtime and are not logged
- [Low] dm-qcow2: stale comment in qcow2_flush_deferred_activity() after revert change
--

commit b704be783dffd54a5e66f07bd3558ffa2d0a7655
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

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

This patch stops revert_l_entries_update() from dropping the refcount of
clusters allocated for a failed L1/L2 writeback. It also clears
FREE_ALLOCATED_CLU on COW qios that end with the error, so those clusters
keep their refcount too. The commit message says a later check repairs any
clusters that end up leaked.

> 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 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) {
>  		/* Here we restore prealloced and compressed clu mappings */
> +		old = pe_page ? get_u64_from_be_page(pe_page, i) : 0;
> +		set_u64_to_be_page(wbd->md->page, i, old);
>  	}
>  }

[Severity: Medium]
Does this leave the newly allocated clusters permanently allocated for the
lifetime of the running instance?

The data cluster from LU_WANTS_ALLOC and the new L2-table cluster from an
L1 update are allocated in prepare_l_entry_update(), and
mark_cluster_used() sets their refcount to 1.

If the md page writeback then fails, complete_wbd() calls
revert_l_entries_update(), which now only restores the in-memory entry
from pe_page, or to 0. Nothing calls mark_cluster_unused() for these
clusters any more, and no other path frees them.

The guest can retry the failed write, and that retry allocates a new
cluster because the old one is still marked used. With a persistently
failing backing device, would each retry consume one more cluster until
the image or the refcount table is exhausted?

Nothing counts or reclaims these clusters, and no QC_ERR message is
logged, so the operator is not told that a check is needed. Would it make
sense to log when a cluster is intentionally leaked here?

The comment above complete_wbd() that says "clusters allocations
reverted" also looks stale now.

[ ... ]

> @@ -1415,6 +1381,8 @@ static void complete_wbd(struct qcow2 *qcow2, struct wb_desc *wbd)
>  				       &wait_list, &end_list);
>  		spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
>  
> +		list_for_each_entry(qio, &end_list, link)
> +			qio->ext->cleanup_mask &= ~FREE_ALLOCATED_CLU;
>  		qcow2_dispatch_qios(qcow2, NULL, &wait_list);
>  		end_qios(&end_list, errno_to_blk_status(wbd->ret));

Clearing FREE_ALLOCATED_CLU here makes handle_cleanup_mask() skip its
mark_cluster_unused() call for COW qios on the failed end_list.

That removes the only remaining path that would free the COW-allocated
cluster. Is this loop always safe, given that qio->ext is dereferenced
for every qio on end_list? Can any qio on dependent_list have a NULL ext?

The commit message also says:

  "Keep the refcount of these clusters instead. If L1/L2 on disk points to
  such a cluster, the image stays consistent; otherwise the cluster is
  leaked, which check repairs by freeing it. So the image needs no check
  and resume needs no refusal."

Is this accurate? The message itself says a leaked cluster is repaired by
check, so the statement that the image needs no check appears to
contradict it. Could the message be reworded to say that the image stays
consistent but may need a check to reclaim leaked clusters?

diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c

[ ... ]

> 	/*
> 	 * 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 is this comment in qcow2_flush_deferred_activity()
now stale?

It says revert_l_entries_update() may start the slow-writeback timer again
after a failed writeback. That relied on revert_l_entries_update() calling
mark_cluster_unused(), which dirtied R2 pages and re-armed slow_wb_timer.
This patch removes that call, so revert_l_entries_update() only rewrites
L1/L2 entries in the page.

Timer restarts can still come from other end_qios()/qio_endio() cleanup
paths. Could the comment be rewritten to describe the current reason for
the second iteration?

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

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