Re: [Devel] [PATCH hci-8.1 v9 08/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 2 potential issue(s) to consider:
- [Medium] dm-qcow2: relocate_refcount_table() late failure leaves target live on half-converted metadata
- [Low] dm-qcow2: relocate_refcount_table() logs the same message for two different failures
--

commit 6376ef1952337d69d9e96fb5403c763cf0015979
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

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

This patch makes relocate_refcount_table() set md_writeback_error and log
a message when it fails after the header write that points to the new
reftable. With the flag set, wants_check reports the image and resume is
refused until the image is checked.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171

> 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,8 @@ 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 relocate refcount table");
> +		qcow2->tgt->md_writeback_error = true;
>  		goto err_free_r2_pages;
>  	}

[Severity: Low]
This isn't a bug, but the message logged here after a failed
qcow2_rw_page_sync() of the header is identical to the one logged at
err_hdr_written.

Can an administrator tell these two failures apart from the log?

In the header write failure case the cached header is restored and a clean
failure is possible, while at err_hdr_written the header has already been
written and the in-memory state is partly converted. Would distinct
messages make the two cases distinguishable?

Setting md_writeback_error here is conservative, since a short write may
still have reached disk, so that choice looks acceptable.

[ ... ]

> @@ -2405,12 +2407,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))
> -			return -EIO;
> +			goto err_hdr_written;
>  		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
>  	}
>  
>  	return 1;
>  
> +err_hdr_written:
> +	QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
> +	qcow2->tgt->md_writeback_error = true;
> +	return -EIO;
>  err_free_r2_pages:

[Severity: Medium]
Does the target stay live on half-converted metadata after this path
returns -EIO?

By the time err_hdr_written is reached, hdr.refcount_table_offset and
hdr.refcount_table_clusters already point at the new reftable. Only some of
the R1 pages may have been renumbered, the new R2 may be only partly
connected, and the used/unused marking may be incomplete.

The only containment here is the md_writeback_error flag, and it is
consulted only in a few places:

dm-qcow2-target.c:qcow2_preresume()
    if (qcow2_wants_check(tgt)) {
        QC_ERR(ti, "image check and target reload are required");
        return -EIO;

Besides preresume, the flag is read only in the status dump, the
dirty-bit decision and the suspend-time truncate logic. dm-qcow2-map.c
only sets it and never reads it.

The allocation path does not check it either:

allocate_cluster()
    if (unlikely(pos == -ENOENT)) {
        ret = relocate_refcount_table(qcow2, &qio);
        if (ret <= 0)
            return ret;
        goto again;
    }

Can a later allocation that finds no free cluster re-enter
relocate_refcount_table() against the already modified hdr and cached
parameters? Can the target keep servicing writes and cause further
refcount inconsistency until suspend?

Would it make sense to make the target read-only, or to have
allocate_cluster() and the relocation entry check md_writeback_error? The
/* TODO: switch to RO */ comment is still present in the err_free_r2_pages
path of this function.

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

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