Re: [Devel] [PATCH hci-8.1 v10 07/10] dm-qcow2: flag the image for check if reftable relocation fails late

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dm-qcow2: relocate_refcount_table() late failure leaves half-updated reftable live
- [Low] dm-qcow2: relocate_refcount_table() logs stale ret at err_hdr_written
- [Low] dm-qcow2: relocate_refcount_table() doc comment still promises safe rollback on header write failure
--

commit 4b75bcef16f3e55f38e854c689601b4a1f90f664
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-qcow2: flag the image for check if reftable relocation fails late

This sets md_writeback_error and logs a message when
relocate_refcount_table() fails after the header write has been issued.
With the flag set, wants_check reports the image and resume is refused
until it is checked. The old err_eio label is replaced by a new
err_hdr_written label for the failures that happen after the header write.

> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c

[ ... ]

> @@ -2351,6 +2351,10 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>  	if (ret) {
>  		/* Restore old R1 */
>  		set_reftable_in_raw_hdr(md0->page, old_pos, old_clus);
> +		QC_ERR(qcow2->tgt->ti,
> +		       "failed to write header with relocated refcount table: %d",
> +		       ret);
> +		qcow2->tgt->md_writeback_error = true;
>  		goto err_free_r2_pages;
>  	}

[Severity: Low]
This isn't a bug, but the block comment above relocate_refcount_table()
is now out of date:

 * Then, we try to write new hdr on disk. In case of failure, we
 * restore old hdr in memory and do safe rollback. In case of success,

A failure of the header write now also sets md_writeback_error, so the
image is reported by wants_check and resume is refused. Could the comment
be updated to say so?

The comment at the top of the function also says "We act in the way to
provide safe rollback throughout whole function", which is no longer
accurate.

[ ... ]

> @@ -2372,24 +2376,25 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)

[ ... ]

>  	/* Connect new R2 to new R1: one for each group new R1 and R2 span */
>  	group_size = (u64)qcow2->refblock_entries * clu_size;
>  	for (i = pos, r2_pos = end; i < r2_end; i += group_size, r2_pos += clu_size) {
>  		if (WARN_ON_ONCE(r2_pos >= r2_end))
> -			goto err_eio;
> +			goto err_hdr_written;

[Severity: Low]
Is ret assigned on this path before the jump? In
relocate_refcount_table(), the WARN_ON_ONCE(r2_pos >= r2_end) branch goes
to err_hdr_written without setting ret.

At that point ret holds either 0 (from the header write or from
calc_refcounters_map()) or the positive value left by handle_md_page() in
an earlier iteration.

The label then logs:

	QC_ERR(qcow2->tgt->ti, "failed to fill relocated refcount table: %d",
	       ret ?: -ENOENT);

The ?: operator only replaces 0, so a stale positive ret is printed as if
it were an error code, for example "failed to fill relocated refcount
table: 1". Could this branch set ret to a real errno, as the other
converted err_eio sites do?

[ ... ]

> @@ -2408,12 +2413,18 @@ 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_eio;
> +			goto err_hdr_written;
>  		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
>  	}
>  
>  	return 1;
>  
> +err_hdr_written:
> +	/* 0: md page is not cached */
> +	QC_ERR(qcow2->tgt->ti, "failed to fill relocated refcount table: %d",
> +	       ret ?: -ENOENT);
> +	qcow2->tgt->md_writeback_error = true;
> +	return -EIO;

[Severity: Medium]
Does this path leave a half-updated reftable live while the target keeps
running?

When control reaches err_hdr_written, qcow2->hdr.refcount_table_offset and
refcount_table_clusters already point at the new reftable. Only some of the
R1 pages may have been renumbered. The new R2 clusters may not be linked
into R1. The new R1 and R2 clusters may not be marked used, and the old R1
clusters may or may not be marked unused. Nothing rolls this back.

The caller is allocate_cluster():

allocate_cluster()
  find_unused_cluster() returns -ENOENT
    relocate_refcount_table()
      -> err_hdr_written, returns -EIO
  if (ret <= 0) return ret;

The -EIO only fails the current qio. As far as I can see, md_writeback_error
is read only by the status output, qcow2_truncate_unused_tail() and
wants_check(). No allocator or map path reads it. Later qios can therefore
keep calling find_unused_cluster() and mark_cluster_used() against the
inconsistent refcounts.

Could those qios hand out clusters that belong to the new reftable or R2
area, or call relocate_refcount_table() again on the corrupt state and
overwrite metadata or data?

The commit message only claims that the image is flagged and that resume is
refused, and that claim holds. But the "/* TODO: switch to RO */" comment
now remains only on the err_free_r2_pages path. Should the late-failure
path also stop further allocations, for example by switching the target to
read-only?

>  err_free_r2_pages:
>  	for (i = new_md_start; i < new_md_end; i += PAGE_SIZE) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261109115856.1362416-1-alexander.ivanov@virtuozzo.com?part=7

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