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

[Konstantin Khorenko <khorenko@virtuozzo.com>]

> drivers/md/dm-qcow2: truncate unused tail of image on suspend
...
> Truncate the file to min(file_preallocated_area_start, used end).
> Skip the scan if md writeback failed, as refcounts on disk are stale
> then, and fall back to the old behaviour on scan error.

There is one more case of stale refcounts on disk: an image with
INCOMPATIBLE_FEATURES_DIRTY_BIT set. qcow2_check_convert_hdr() accepts it
(the check is commented out), and with qemu lazy refcounts that bit means
refcounts were not written for the last allocations, so they are too low.

Today such an image loses data only when the allocator hands out a
cluster whose refcount is zero but which L2 still references. After this
patch the first suspend truncates live clusters at the tail without a
single write to the device. The image is already invalid per spec, so
this is not a regression in the strict sense, but either the commit
message should say that a dirty image is not supported, or the dirty-bit
check in qcow2_check_convert_hdr() should come back.

> +static int get_md_page_sync(struct qcow2 *qcow2, u64 id, struct md_page **md)
> +{
> +     int ret;
> +
> +     spin_lock_irq(&qcow2->md_pages_lock);
> +     *md = qcow2_md_page_find_or_postpone(qcow2, id, NULL);
> +     spin_unlock_irq(&qcow2->md_pages_lock);
> +     if (*md)
> +             return 0;
> +
> +     ret = qcow2_alloc_and_insert_md_page(qcow2, id, md);
> +     if (ret)
> +             return ret;
> +
> +     ret = qcow2_rw_page_sync(READ, qcow2, id, (*md)->page);

qcow2_attach_file() does the same alloc + qcow2_rw_page_sync(READ) +
MD_UPTODATE sequence by hand for page 0, minus the erase on failure. If
this helper lives in dm-qcow2-target.c next to the other md page helpers
and is exported through dm-qcow2.h, that call site can use it as well.

> +     pos = round_down(qcow2->file_size - 1, clu_size);
> +     while (pos >= 0) {

Everything at or above file_preallocated_area_start is unused by
construction: allocate_cluster() moves it past every cluster it hands
out, and relocate_refcount_table() goes through truncate_prealloc_safe(),
which sets it to the requested length. Starting from file_size scans the
R2 entries covering the preallocated area (up to PREALLOC_SIZE) on every
suspend for nothing. Start from file_preallocated_area_start - 1 instead.

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