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