[Devel] [PATCH hci-8.1 v11 08/10] dm-qcow2: leak clusters allocated for a failed L1/L2 writeback

[Alexander Ivanov]

>> @@ -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.
Added "fail the L2 table allocation request on L1 writeback error" patch.