Re: [Devel] [PATCH hci-8.1 v5 3/3] dm-qcow2: truncate unused tail of image on suspend

[Konstantin Khorenko <khorenko@virtuozzo.com>]

I'd rather not truncate the image in the kernel at all. My comments
below are for the case we keep it.

The scan trusts the refcounts and never checks them against L1/L2. A
wrong refcount or a wrong scan start then costs data on the first
suspend: stop, snapshot or compact.

Without this patch such an image can still be repaired:
- ploop runs "qemu-img check -r all" on umount, compact and resize;
- our qemu-img (qcow2_check_extra_preallocation(), not in mainline)
  rebuilds the refcounts from L1/L2 and truncates the tail only if the
  image is clean;
- qcow2_preresume() rereads the metadata, so this works on a suspended
  device too.

This review alone found two ways the scan cuts live data, see below. I
don't think they are the last ones, so I suggest we keep the truncation
in userspace.

> + * leave the tail of file unused. Everything at or above
> + * file_preallocated_area_start is unused by construction, so the scan
> + * starts below it. This must be called on suspended device after md

This is not true, and the idea came from my v1 review.

place_r2() links a new refblock into R1, but moves neither
file_preallocated_area_start nor file_size. The refblock stays at or
above the watermark if:
- truncate_prealloc_safe() in allocate_cluster() fails after
  place_r2();
- a parallel mark_cluster_unused() lowers free_cluster_search_pos
  before the second pass of find_unused_cluster().

The scan then starts below the refblock and may truncate the file under
it. R1 on disk points past EOF, and the next session gets -EIO on
allocations in that range. In the first case the old code did nothing,
so this is a regression.

Please make place_r2() extend the file and raise the watermark to
r2_pos + clu_size before it links the refblock into R1. The old code
can cut such a refblock too, so this is a separate patch before this
one.

> +	pos = round_down(qcow2->file_preallocated_area_start - 1, clu_size);
> +	while (pos >= 0) {
> +		if (calc_refcounters_map(qcow2, pos, &r1, &r2) < 0)
> +			return -EIO;

After a failed relocate_refcount_table(), file_preallocated_area_start
stays at r2_end, beyond reftable_max_file_size. The first pos is then
outside the reftable, and calc_refcounters_map() hits WARN_ONCE(), which
panics with panic_on_warn.

Please start from min(file_preallocated_area_start,
reftable_max_file_size).

> +	/* Refcounts on disk are stale after failed md writeback */
> +	if (!tgt->md_writeback_error) {

md_writeback_error is set for R1/R2 pages only. If an L2 write fails
but the page did reach the disk, revert_l_entries_update() drops the
refcount of the new cluster to zero, while L2 on disk still points to
it. If the cluster is at the tail, the scan cuts it.

Please skip the scan after any failed md writeback, L1/L2 included.

> on remove, __dm_destroy() suspends the live table if it isn't
> suspended yet

__dm_destroy() only calls the presuspend and postsuspend hooks: it does
not stop the queue or wait for I/O. The device is idle there only
because dm_lock_for_deletion() refuses to remove an open device. Please
say that in the commit message.

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel