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 <[email protected]> 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/[email protected]?part=8 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
