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

[Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>]

Patch should be split.
l1/l2 revert tracking and relocation error are pre-existing problems and 
should be separated.

On 10/26/26 15:42, Alexander Ivanov wrote:
> Discard punches holes in place of freed clusters, so they hold no
> physical extents, but it doesn't change the file size. On postsuspend
> we truncate the image file only down to file_preallocated_area_start,
> i.e. we drop preallocations only. So if clusters at the end of the
> file are freed (refcount dropped to zero), the file size stays the
> same and the image never shrinks.
> 
> Add qcow2_calc_used_end(), which scans refcount blocks backward from
> file_preallocated_area_start and returns the end of the last cluster
> with non-zero refcount. Refcount ranges with no allocated refblock
> are skipped at once. Metadata pages are taken from md cache or read
> synchronously. This is called on suspended device after md writeback,
> so R1 and R2 can't change under us. A failed relocation of the
> reftable leaves file_preallocated_area_start beyond the space covered
> by the reftable, so the scan starts at most from its end. If the
> relocation fails after the new header is written, refcounts no longer
> match the header, so set md_writeback_error there.
> 
> Truncate the file to the used end. Fall back to the old behaviour on
> scan error and after any failed md writeback. A failed R1/R2 write
> leaves refcounts on disk stale. A failed L1/L2 write reverts the
> refcount of a newly allocated cluster to zero, while L1/L2 on disk may
> still point to it. Mark such an L1/L2 page with MD_LX_REVERTED and
> clear the mark on its next successful writeback, which rewrites the
> whole page, e.g. when a request delayed on ENOSPC is resubmitted. On
> md writeback flush a still marked page sets a new lx_writeback_error
> flag, which is reported in get_errors and logged when the scan is
> skipped. The flag is lost on target reload, after which the scan would
> trust the reverted refcount, so include it in wants_check like
> md_writeback_error: resume is refused until the image is checked and
> the target reloaded.
> 
> Refcounts may also be below real usage after a crash, since both QEMU
> with lazy_refcounts and this driver write L2 before refcounts. The scan
> would then cut off clusters L2 still points to, even in a session
> without guest writes. With kernel_sets_dirty_bit such an image has the
> dirty bit set at open, and the disabled dirty bit check lets it be
> opened RW, so skip the scan for it. Otherwise userspace sets the dirty
> bit on every RW open, so the bit doesn't mean the image is
> inconsistent. Userspace checks the image before opening it RW, which
> find_unused_cluster() already relies on when it reuses clusters with
> zero refcount.
> 
> Close and reload to RO go through postsuspend too. On remove,
> __dm_destroy() calls the presuspend and postsuspend hooks if the live
> table isn't suspended yet. It neither stops the queue nor waits for
> I/O, but the device is idle there, because dm_lock_for_deletion()
> refuses to remove an open device. On table swap, do_resume() suspends
> the old (RW) table before dm_swap_table(). So the file is shrunk in
> these cases as well.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-qcow2: block device over QCOW2 files driver
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   drivers/md/dm-qcow2-cmd.c    |  4 +-
>   drivers/md/dm-qcow2-map.c    | 96 +++++++++++++++++++++++++++++++++---
>   drivers/md/dm-qcow2-target.c | 38 +++++++++++---
>   drivers/md/dm-qcow2.h        |  6 ++-
>   4 files changed, 127 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
> index 2865940cff30..1520733c962c 100644
> --- a/drivers/md/dm-qcow2-cmd.c
> +++ b/drivers/md/dm-qcow2-cmd.c
> @@ -20,9 +20,9 @@ static int qcow2_get_errors(struct qcow2_target *tgt, char *result,
>   	int ret;
>   
>   	ret = DMEMIT("wants_check=%d\nmd_writeback_error=%d\ntruncate_error=%d\n"
> -		     "merge_backward_error=%d\n",
> +		     "merge_backward_error=%d\nlx_writeback_error=%d\n",
>   		      wants_check, tgt->md_writeback_error, tgt->truncate_error,
> -		      tgt->backward_merge.error);
> +		      tgt->backward_merge.error, tgt->lx_writeback_error);
>   
>   	return ret ? 1 : 0;
>   }
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index fa63bd6acfb8..c0669dc630b8 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -1090,6 +1090,11 @@ static int calc_refcounters_map(struct qcow2 *qcow2, loff_t pos,
>   	return -EIO;
>   }
>   
> +static bool r1_entry_is_valid(struct qcow2 *qcow2, u64 entry)
> +{
> +	return !(entry & R1_RESERVED_ZERO_MASK) && CLU_OFF(qcow2, entry) == 0;
> +}
> +
>   static int calc_r2_page_id(struct qcow2 *qcow2, struct qcow2_map_item *r1,
>   						struct qcow2_map_item *r2)
>   {
> @@ -1097,8 +1102,7 @@ static int calc_r2_page_id(struct qcow2 *qcow2, struct qcow2_map_item *r1,
>   	u32 bits = qcow2->refblock_bits;
>   	loff_t pos;
>   
> -	if (WARN_ON_ONCE((entry & R1_RESERVED_ZERO_MASK) ||
> -			 (CLU_OFF(qcow2, entry) != 0)))
> +	if (WARN_ON_ONCE(!r1_entry_is_valid(qcow2, entry)))
>   		return -EIO;
>   
>   	/* The corresponding refcount block has not yet been allocated */
> @@ -1362,6 +1366,8 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>   		if (cleared)
>   			continue; /* no cluster was allocated */
>   
> +		/* L1/L2 on disk may point to the cluster */
> +		wbd->md->status |= MD_LX_REVERTED;
>   		spin_unlock(&qcow2->md_pages_lock);
>   		pos &= ~LX_REFCOUNT_EXACTLY_ONE;
>   
> @@ -1385,6 +1391,8 @@ static void clear_writeback_status(struct qcow2 *qcow2, struct md_page *md,
>   	lockdep_assert_held(&qcow2->md_pages_lock);
>   
>   	md->status &= ~(MD_WRITEBACK|MD_WRITEBACK_ERROR);
> +	if (!ret)
> +		md->status &= ~MD_LX_REVERTED;
>   	list_splice_init(&md->wait_list, wait_list);
>   	if (ret && !md->wbd) {
>   		/*
> @@ -2201,6 +2209,74 @@ int qcow2_truncate_safe(struct file *file, loff_t new_len)
>   	return do_ftruncate(file, new_len, 0);
>   }
>   
> +/*
> + * Returns the end of the last cluster, which is used according
> + * to refcount table, clamped to file_preallocated_area_start since
> + * the last cluster may be partial. Clusters freed by discard may
> + * leave the tail of file unused. Everything at or above
> + * file_preallocated_area_start is unused by construction, so the scan
> + * starts below it. Failed relocate_refcount_table() leaves it beyond
> + * reftable_max_file_size, so clamp the start to the reftable coverage.
> + * This must be called on suspended device after md writeback, so
> + * nobody changes R1 and R2.
> + */
> +loff_t qcow2_calc_used_end(struct qcow2 *qcow2)
> +{
> +	u32 clu_size = qcow2->clu_size;
> +	struct qcow2_map_item r1, r2;
> +	struct md_page *r1_md = NULL;
> +	loff_t pos;
> +	s32 i;
> +	int ret;
> +
> +	pos = min(qcow2->file_preallocated_area_start,
> +		  qcow2->reftable_max_file_size);
> +	pos = round_down(pos - 1, clu_size);
> +	while (pos >= 0) {
> +		if (calc_refcounters_map(qcow2, pos, &r1, &r2) < 0)
> +			return -EIO;
> +
> +		if (!r1_md || r1_md->id != r1.page_id) {
> +			ret = qcow2_md_page_read_sync(qcow2, r1.page_id, &r1_md);
> +			if (ret)
> +				return ret;
> +		}
> +		r1.md = r1_md;
> +
> +		/*
> +		 * The scan walks R1 entries normal I/O may never touch,
> +		 * so don't let calc_r2_page_id() WARN on a corrupted one.
> +		 */
> +		if (!r1_entry_is_valid(qcow2, get_u64_from_be_page(r1_md->page,
> +							r1.index_in_page)))
> +			return -EIO;
> +
> +		ret = calc_r2_page_id(qcow2, &r1, &r2);
> +		if (ret == -ENOENT) {
> +			/* No R2 cluster, so all clusters it covers are unused */
> +			pos = (loff_t)r1.index * qcow2->refblock_entries * clu_size;
> +			pos -= clu_size;
> +			continue;
> +		}
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		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) {
> +			/* Last cluster is partial if size isn't cluster-aligned */
> +			if (get_r2_entry(qcow2, r2.md, i))
> +				return min_t(loff_t, pos + clu_size,
> +					     qcow2->file_preallocated_area_start);
> +		}
> +	}
> +
> +	return -EIO;
> +}
> +
>   static int truncate_prealloc_safe(struct qcow2 *qcow2, loff_t len, const char *func)
>   {
>   	loff_t prealloc_len, max_prealloc_len = qcow2->reftable_max_file_size;
> @@ -2372,21 +2448,21 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>   		if (!WARN_ON_ONCE(!md))
>   			qcow2_md_make_dirty(qcow2, md, true);
>   		if (!md)
> -			break; /* goto err_free_r2_pages */
> +			break; /* goto err_hdr_written */
>   	}
>   	spin_unlock_irq(&qcow2->md_pages_lock);
>   	if (i != nr_pages)
> -		goto err_free_r2_pages;
> +		goto err_hdr_written;
>   
>   	/* Connect new R2 to new R1 */
>   	for (i = end; i < r2_end; i += clu_size) {
>   		if (calc_refcounters_map(qcow2, i, &r1, &r2) < 0) {
>   			WARN_ON_ONCE(1);
> -			goto err_free_r2_pages;
> +			goto err_hdr_written;
>   		}
>   		ret = handle_md_page(qcow2, r1.page_id, NULL, &md);
>   		if (WARN_ON_ONCE(ret <= 0))
> -			goto err_free_r2_pages;
> +			goto err_hdr_written;
>   		spin_lock_irq(&qcow2->md_pages_lock);
>   		set_u64_to_be_page(md->page, r1.index_in_page, i);
>   		qcow2_md_make_dirty(qcow2, md, true);
> @@ -2397,7 +2473,7 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>   	for (i = pos; i < r2_end; i += clu_size) {
>   		ret = qcow2_handle_r1r2_maps(qcow2, i, NULL, &r1, &r2, false);
>   		if (WARN_ON_ONCE(ret <= 0))
> -			goto err_free_r2_pages;
> +			goto err_hdr_written;
>   		mark_cluster_used(qcow2, r2.md, r2.index_in_page);
>   	}
>   
> @@ -2405,12 +2481,16 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>   	for (i = old_pos; i < old_end; i += clu_size) {
>   		ret = qcow2_handle_r1r2_maps(qcow2, i, NULL, &r1, &r2, false);
>   		if (WARN_ON_ONCE(ret <= 0))
> -			goto err_free_r2_pages;
> +			goto err_hdr_written;
>   		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
>   	}
>   
>   	return 1;
>   
> +err_hdr_written:
> +	/* Header on disk points to new R1, refcounts may be inconsistent */
> +	QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
> +	qcow2->tgt->md_writeback_error = true;
>   err_free_r2_pages:
>   	for (i = end; i < r2_end; i += clu_size) {
>   		ret = handle_md_page(qcow2, i >> PAGE_SHIFT, NULL, &md);
> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> index 2d69e39a3530..7895fea531ae 100644
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c
> @@ -170,6 +170,7 @@ static void qcow2_free_md_pages_tree(struct rb_root *root)
>   void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2)
>   {
>   	struct rb_node *node;
> +	bool wb_error = false;
>   	struct md_page *md;
>   	int i;
>   
> @@ -191,13 +192,17 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
>   	     node; node = rb_next(node)) {
>   		md = rb_entry(node, struct md_page, node);
>   		/* FIXME: call md_make_dirty() and try once again? */
> -		if (md->status & MD_WRITEBACK_ERROR) {
> -			QC_ERR(tgt->ti, "Failed to write dirty pages");
> -			tgt->md_writeback_error = true;
> -			break;
> -		}
> +		if (md->status & MD_WRITEBACK_ERROR)
> +			wb_error = true;
> +		if (md->status & MD_LX_REVERTED)
> +			tgt->lx_writeback_error = true;
>   	}
>   	spin_unlock_irq(&qcow2->md_pages_lock);
> +
> +	if (wb_error) {
> +		QC_ERR(tgt->ti, "Failed to write dirty pages");
> +		tgt->md_writeback_error = true;
> +	}
>   }
>   
>   static void qcow2_flush_deferred_activity_all(struct qcow2_target *tgt)
> @@ -1058,11 +1063,32 @@ static void qcow2_truncate_preallocations(struct dm_target *ti)

qcow2_truncate_preallocations should be renamed. It does much more now.

>   {
>   	struct qcow2_target *tgt = to_qcow2_target(ti);
>   	struct qcow2 *qcow2 = top_qcow2_protected(ti);
> -	loff_t end = qcow2->file_preallocated_area_start;
> +	loff_t used_end, end = qcow2->file_preallocated_area_start;
>   	int ret;
>   
>   	if (!(dm_table_get_mode(ti->table) & FMODE_WRITE))
>   		return;
> +
> +	/*
> +	 * Refcounts don't match L1/L2 on disk after failed md writeback.
> +	 * They may also be below real usage in an image opened dirty,
> +	 * e.g. after a crash. If userspace sets the dirty bit, it is set
> +	 * on every RW open and userspace checks the image before that.
> +	 */

Useless comment that just repeats what code does. Why?

> +	if (tgt->md_writeback_error) {
> +		QC_INFO(ti, "Skip shrinking to used end after md writeback error");
> +	} else if (tgt->lx_writeback_error) {
> +		QC_INFO(ti, "Skip shrinking to used end after reverted L1/L2 writeback");
> +	} 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);
> +		if (used_end < 0)
> +			QC_ERR(ti, "Can't find used end: %pe", ERR_PTR(used_end));
> +		else
> +			end = used_end;
> +	}
> +
>   	if (end == qcow2->file_size)
>   		return;
>   
> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
> index a2d951ee5541..7db2d9301cad 100644
> --- a/drivers/md/dm-qcow2.h
> +++ b/drivers/md/dm-qcow2.h
> @@ -134,6 +134,7 @@ struct md_page {
>   #define MD_DIRTY	(1U << 1) /* Page contains changes and wants writeback */
>   #define MD_WRITEBACK	(1U << 2) /* Writeback was submitted */
>   #define MD_WRITEBACK_ERROR (1U << 3) /* Last writeback failed with error */
> +#define MD_LX_REVERTED	(1U << 4) /* L1/L2 on disk may point to freed cluster */
>   	unsigned int status;
>   	struct page *page;
>   	struct list_head wait_list;
> @@ -200,6 +201,7 @@ struct qcow2_target {
>   	bool service_operations_allowed;
>   	bool wants_suspend;
>   	bool md_writeback_error;
> +	bool lx_writeback_error;
>   	bool truncate_error;
>   	bool event_enospc;
>   
> @@ -413,6 +415,7 @@ void qcow2_ref_dec(struct qcow2_target *tgt, u8 ref_index);
>   int qcow2_inflight_ref_switch(struct qcow2_target *tgt);
>   void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2);
>   int qcow2_truncate_safe(struct file *file, loff_t new_len);
> +loff_t qcow2_calc_used_end(struct qcow2 *qcow2);
>   
>   void qcow2_merge_backward_work(struct work_struct *work);
>   void qcow2_merge_backward_cancel(struct qcow2_target *tgt);
> @@ -461,7 +464,8 @@ static inline bool fake_service_qio(struct qio *qio)
>   static inline bool qcow2_wants_check(struct qcow2_target *tgt)
>   {
>   
> -	return !!(tgt->md_writeback_error|tgt->truncate_error);
> +	return !!(tgt->md_writeback_error|tgt->lx_writeback_error|
> +		  tgt->truncate_error);
>   }
>   
>   static inline bool qcow2_file_is_writable(struct qcow2 *qcow2)

I would really advise againt commenting every action. The following 
example is the worst of the worst.

/* If we have an error skip shrinking */
If (error)
	skip_shrinking()


Please read Documentation/process/coding-style.rst section 8 (or have 
your agent read it). I am not planning to go on each comment in this 
patch, please do the pruning yourself.

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel