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

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

1. Get rid of useless comments (which is all comments in this patch)
2. kernel_dirty_bit should be kernel_dirty_bit_set because without set 
it makes no sense.
3. Commit message should be reworded to include the following very 
important information:
  - kernel_sets_dirty_bit is a module parameter
  - we do not set it in Virtuozzo
  - ploop sets this from userspace itself (so this is rather a hardening 
patch)

On 10/26/26 15:42, Alexander Ivanov wrote:
> With kernel_sets_dirty_bit, postsuspend clears the dirty bit
> unconditionally. An image that is dirty at open, e.g. after a crash,
> has unrepaired refcounts, which may be below real usage, yet it is
> marked clean on disk after the first suspend. A userspace check before
> the next open then has no reason to repair it.
> 
> Clear the bit only if the image was clean before the kernel set it.
> The parsed header can't tell this: it is reread on every preresume and
> may carry the kernel's own bit, e.g. on the lower image of a backward
> merge cancelled by suspend in waiting_completion or of a merge with
> depth > 0. So track two flags per image: kernel_dirty_bit is set only
> while the last header write was a successful set and the reread header
> still has the bit, opened_dirty is set when the header has the bit and
> kernel_dirty_bit is not set. Clearing the bit of an image opened dirty
> is left to userspace check.
> 
> After a failed write the bit found on disk is taken as opened dirty,
> which costs an extra check, rather than as the kernel's, which could
> clear a bit set by userspace while the device was suspended.
> 
> This only preserves the on-disk marker. The driver still accepts a
> dirty image read-write and allocates from its refcounts, so
> find_unused_cluster() may hand out clusters that L2 still references
> until the image is checked.

Completely irrelevant paragraph

> 
> 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-target.c | 15 +++++++++++++--
>   drivers/md/dm-qcow2.h        |  2 ++
>   2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> index b667adb5472a..2d69e39a3530 100644
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c
> @@ -659,6 +659,7 @@ int qcow2_set_image_file_features(struct qcow2 *qcow2, bool dirty)
>   	struct QCowHeader *raw_hdr;
>   	struct md_page *md;
>   	bool is_ro;
> +	int ret;
>   
>   	if (qcow2->hdr.version ==  2)
>   		return 0;
> @@ -676,12 +677,15 @@ 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->opened_dirty)
>   			raw_hdr->incompatible_features &= ~dirty_mask;
>   	}
>   	kunmap(md->page);
>   
> -	return qcow2_rw_page_sync(WRITE, qcow2, md->id, md->page);
> +	ret = qcow2_rw_page_sync(WRITE, qcow2, md->id, md->page);
> +	if (kernel_sets_dirty_bit)
> +		qcow2->kernel_dirty_bit = dirty && !ret;
> +	return ret;
>   }
>   ALLOW_ERROR_INJECTION(qcow2_set_image_file_features, ERRNO);
>   
> @@ -903,6 +907,13 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
>   		goto out;
>   
>   	qcow2_calc_cached_parameters(qcow2, hdr);
> +	/* The header is reread on resume and may carry our own dirty bit */
> +	if (!(hdr->incompatible_features & INCOMPATIBLE_FEATURES_DIRTY_BIT)) {
> +		qcow2->opened_dirty = false;
> +		qcow2->kernel_dirty_bit = false;
> +	} else if (!qcow2->kernel_dirty_bit) {
> +		qcow2->opened_dirty = true;
> +	}
>   	ret = -EOPNOTSUPP;
>   	if (qcow2->clu_size < PAGE_SIZE ||
>   	    (qcow2->ext_l2 && qcow2->clu_size < PAGE_SIZE * 32))
> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
> index 69d3ae6fca8a..a2d951ee5541 100644
> --- a/drivers/md/dm-qcow2.h
> +++ b/drivers/md/dm-qcow2.h
> @@ -257,6 +257,8 @@ struct qcow2 {
>   	u32 refblock_bits;
>   	u32 refblock_entries;
>   	bool ext_l2;
> +	bool opened_dirty; /* Dirty bit was set before we set it */> +	bool kernel_dirty_bit; /* We set the dirty bit on disk */
>   
>   	bool pause_submitting_qios; /* This is used only on top qcow2 image */
>   	bool backward_merge_in_process;

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