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