[Devel] [PATCH hci-8.1 v11 08/10] 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 1 potential issue(s) to consider:
- [Medium] dm-qcow2: clusters leaked on failed L1/L2 writeback; L1 alloc retry repeats leak
--

Subject: Re: [PATCH] dm-qcow2: leak clusters allocated for a failed L1/L2 writeback

commit 8f9e1298a69365ad6d96d693fc232f0ee7761431
Author: Alexander Ivanov <alexander.ivanov at virtuozzo.com>

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

This commit stops dropping the refcount of clusters allocated for a failed
L1/L2 writeback. revert_l_entries_update() now only restores the in-memory
entries, and complete_wbd() clears FREE_ALLOCATED_CLU on the qios ended
with the error.

> 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);
>  	}
>  }

[ ... ]

> @@ -1408,6 +1373,7 @@ static void complete_wbd(struct qcow2 *qcow2, struct wb_desc *wbd)
>  		LIST_HEAD(wait_list);
>  		LIST_HEAD(end_list);
>  		unsigned long flags;
> +		struct qio *qio;
>  
>  		spin_lock_irqsave(&qcow2->md_pages_lock, flags);
>  		revert_l_entries_update(qcow2, wbd);
> @@ -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));

[Severity: Medium]
Can this leak more than one cluster per failed writeback in the L1 case?

In prepare_l1l2_allocation(), the qio that allocated an L2-table cluster is
queued on the L1 page wait list, not on wbd->dependent_list:

    list_add_tail(&qio->link, &map->l1.md->wait_list);

On a failed writeback, complete_wbd() calls clear_writeback_status(), which
does:

    list_splice_init(&md->wait_list, wait_list);
    ...
    list_splice_init(&md->wbd->dependent_list, end_list);

Only dependent_list is ended with the error. The qios on md->wait_list are
re-dispatched through qcow2_dispatch_qios().

revert_l_entries_update() has already reset the L1 entry to 0, so the
re-dispatched qio finds it empty and allocates another L2-table cluster,
dirties L1 and attempts writeback again. The cluster from the earlier attempt
is no longer freed, since mark_cluster_unused() is gone.

With a persistent L1 writeback error, does each retry cycle leak one more
cluster? Before this patch the cluster was freed, so the retry reused the
same one.

The L2 data-cluster and COW cases look bounded, since those qios sit on
dependent_list and end with the error, losing one cluster per failed request.

I did not find anything later in the series that reclaims the leaked
clusters. Nothing appears to roll back free_cluster_search_pos either.

The commit message says:

> So the image needs no check and resume needs no refusal.

Is this accurate? The leaked case is exactly the one that check repairs,
and the message does not mention that a repeated L1 allocation retry can
leak further clusters.

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