[Devel] [PATCH hci-8.1 v12 09/12] dm-qcow2: leak clusters allocated for a failed L1/L2 writeback

[Alexander Ivanov]

>> +		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.