Re: [Devel] [PATCH hci-8.1 v6 5/6] dm-qcow2: keep the dirty bit of an image opened dirty

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] dm-qcow2: hdr.incompatible_features can't tell the driver's own dirty bit from a crash-left one
- [Low] dm-qcow2: stale hdr dirty bit makes the new clear-skip leave a clean lower image dirty
--

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

dm-qcow2: keep the dirty bit of an image opened dirty

This patch changes qcow2_set_image_file_features() so that, with
kernel_sets_dirty_bit, the on-disk dirty bit is cleared only if it is not
set in the parsed header (qcow2->hdr.incompatible_features). The aim is to
keep the dirty marker on an image that was already dirty when opened.

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

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

[ ... ]

> @@ -676,7 +676,8 @@ int qcow2_set_image_file_features(struct qcow2 *qcow2, bool dirty)
>  	if (kernel_sets_dirty_bit) {
>  		if (dirty)
>  			raw_hdr->incompatible_features |= dirty_mask;
> -		else
> +		else if (!(qcow2->hdr.incompatible_features &
> +			   INCOMPATIBLE_FEATURES_DIRTY_BIT))
>  			raw_hdr->incompatible_features &= ~dirty_mask;
>  	}

[Severity: Low]
Can qcow2->hdr.incompatible_features tell the driver's own dirty bit apart
from one left by a crash? The header is re-parsed from disk on every
preresume:

qcow2_preresume()
    qcow2_free_md_pages_all(tgt);
    ...
    qcow2_parse_metadata(ti, tgt);
    ...
    qcow2_set_image_file_features(tgt->top, true);

If the kernel's bit is already on disk at parse time, the parsed header
reports it as dirty-at-open, and every later call with dirty=false skips
the clear.

One way this happens is a failed clear in postsuspend:

qcow2_postsuspend()
    ret = qcow2_set_image_file_features(qcow2, false);
    if (ret)
        QC_ERR(ti, "Can't set features");

If the qcow2_rw_page_sync() write fails, the error is only logged and the
bit stays on disk. The next qcow2_preresume() then reads it back as dirty
at open, and the driver never clears it afterwards. A retried preresume
without an intervening postsuspend could do the same.

The new "Skip shrinking to used end of dirty image" branch in
qcow2_truncate_preallocations() from patch 6 tests the same parsed-header
bit. A falsely dirty image would also lose its tail truncation.

The commit message does not mention this behaviour. Could it be described,
or could the driver track separately whether the bit was set at open?

[Severity: Low]
Does the new condition also leave a clean lower image marked dirty after a
cancelled backward merge? In dm-qcow2-cmd.c, qcow2_merge_backward_work()
calls:

ret = qcow2_set_image_file_features(lower, true);

This sets the bit on lower's raw header page and on disk only. lower->hdr
is not updated.

If a cancel arrives while the merge is in the WAIT_COMPLETION state,
qcow2_merge_backward_cancel() does only this:

} else if (tgt->backward_merge.state == BACKWARD_MERGE_WAIT_COMPLETION) {
    set_backward_merge_in_process(tgt, *tgt->backward_merge.pqcow2, false);
    tgt->backward_merge.state = BACKWARD_MERGE_STOPPED;

It never clears lower's on-disk dirty bit. On the next resume,
qcow2_preresume() re-parses every image in the chain, so
lower->hdr.incompatible_features now contains the kernel's own bit.

A later merge_backward start can then go one of two ways:

- If it fails, the clear in qcow2_merge_backward_work() is skipped:
  ret2 = qcow2_set_image_file_features(lower, false);

- If it completes, lower becomes the top image. The clear in
  qcow2_postsuspend() is skipped for the same reason:
  ret = qcow2_set_image_file_features(qcow2, false);

Before this patch the second case cleared the bit. With this patch the
image stays marked dirty on disk even though the kernel set the bit. The
stale on-disk bit after cancel already existed, but it used to be cleared
at the next postsuspend and is now permanent.

Userspace may then run an unnecessary check, or refuse to open the image.
The exposure needs kernel_sets_dirty_bit=1, a cancel in WAIT_COMPLETION, and
a later merge or suspend cycle.

Could the cancel path clear the bit, or could the driver record the open-time
dirty state separately from the re-parsed header?

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

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