+ 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?
A leaked cluster does not corrupt the image: nothing on disk points to a
free
cluster, and no two offsets share a cluster. It is the same harmless
leak that
qemu-img check reclaims during any check. Setting wants_check would bring
back the resume refusal that this commit removes. The message already says
that each failed request leaks one cluster. A separate log message adds
nothing,
since the guest already gets the write error.
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?
The loop itself explains why it runs twice.
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
