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

[Alexander Ivanov]

>>   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.
The leak is intentional. The failed write may have partially reached
the disk, so L1/L2 on disk may point to the allocated cluster. Freeing
it lets the allocator hand it to another offset, and two guest offsets
then share it, which corrupts the image. A leaked cluster is harmless,
and check reclaims it.

The retry concern is mostly theoretical. Refcounts reach the disk
before L1/L2 does, so on a persistently failing device the R2
writeback is expected to fail first. That sets MD_WRITEBACK_ERROR and
the image needs check anyway. A leak needs the R2 and data writes to
succeed while the L2 write fails, e.g. a transient ENOSPC on an L2
overwrite. Such a write is retried no more often than the ENOSPC
timeout. The refcount table is not exhausted either: it is relocated
and the file grows.

A message is not added: the image stays consistent, and space is the
only thing lost.

The stale comment above complete_wbd() is fixed, thanks.
>
>> @@ -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?
A leak needs the data write to succeed while the L2 overwrite fails,
e.g. with a transient ENOSPC. With the disk full, the data write fails
first and the cluster is freed as before. The image stays consistent;
only space is lost, so no message is added.
> 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?
Rewrote the comment.