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