[Devel] [PATCH hci-8.1 v11 10/10] dm-qcow2: truncate unused tail of image on suspend

[Alexander Ivanov]

>> +		ret = qcow2_md_page_read_sync(qcow2, r2.page_id, &r2.md);
>> +		if (ret)
>> +			return ret;
>> +
>> +		for (i = r2.index_in_page; i >= 0; i--, pos -= clu_size) {
>> +			if (get_r2_entry(qcow2, r2.md, i))
>> +				return min_t(loff_t, pos + clu_size,
>> +					     qcow2->file_preallocated_area_start);
>> +		}
> [Severity: Medium]
> Can this scan allocate memory in a way that deadlocks under reclaim? qcow2_calc_used_end() is called from the postsuspend hook:
>
> qcow2_postsuspend()
>    qcow2_truncate_unused_tail()
>      qcow2_calc_used_end()
>        qcow2_md_page_read_sync()
>          qcow2_alloc_and_insert_md_page()
>            kmalloc(sizeof(**md), GFP_KERNEL)
>            alloc_page(GFP_KERNEL)
>
> Every R1 or R2 page that is not already in the md cache is read this way, and then read from the image file. Neither the dm core nor the driver sets a memalloc_noio or memalloc_nofs scope around postsuspend.
>
> Direct reclaim from these GFP_KERNEL allocations could issue writeback to a filesystem or swap on this same suspended device, or on a device stacked on it. That I/O cannot complete until resume, and resume waits for postsuspend.
>
> Before this patch, postsuspend only used the already cached page 0 through qcow2_set_image_file_features(). The other allocations in the I/O and metadata paths use GFP_NOIO or run under PF_MEMALLOC_NOIO.
>
> Should the scan run under a memalloc_noio_save()/memalloc_noio_restore() scope, or use GFP_NOIO allocations?
Fixed.
>> +	if (tgt->md_writeback_error) {
>> +		QC_INFO(ti, "Skip shrinking to used end after md writeback error");
>> +	} else if (kernel_sets_dirty_bit && qcow2->opened_dirty) {
>> +		QC_INFO(ti, "Skip shrinking to used end of dirty image");
>> +	} else {
>> +		used_end = qcow2_calc_used_end(qcow2);
> [Severity: Medium]
> Can this truncation cut off live guest data? The used end is derived from refcounts alone, with no cross-check against L2. If a cluster has refcount zero but is still referenced by L2, the cut point moves below live data and qcow2_truncate_safe() discards it permanently.
>
> The commit message says refcounts may be below real usage after a crash, since both QEMU with lazy_refcounts and this driver write L2 before refcounts. The only guards here are md_writeback_error and kernel_sets_dirty_bit && opened_dirty.
>
> A v2 image has no dirty bit. qcow2_check_convert_hdr() returns early for version 2, so opened_dirty stays false.
>
> In the default mode kernel_sets_dirty_bit is false, so the opened_dirty guard is skipped.
>
> The ctr-time check in qcow2_check_convert_hdr() is commented out:
>
> //	if (!is_ro && kernel_sets_dirty_bit !=
> //	    !(hdr->incompatible_features & INCOMPATIBLE_FEATURES_DIRTY_BIT))
> //		return kernel_sets_dirty_bit ? -EUCLEAN : -ENOLCK;
>
> qcow2_wants_check() covers only md_writeback_error and truncate_error, which are errors from the current session. Nothing in the driver detects an on-disk inconsistency left by an earlier crash.
>
> The commit message says the scan is safe "only because userspace checks the image before an RW open". Can the driver verify that assumption?
>
> Would it be possible to refuse the truncation when the cut range is still referenced by L2, or to make the shrink opt-in?
As I wrtote, the driver already relies on refcounts being consistent: 
find_unused_cluster()
reuses any cluster with zero refcount, so an image with refcounts below 
L2 usage gets live
data overwritten by the first allocation that reaches it, with or 
without this patch.