Re: [Devel] [PATCH VZ10 v3 09/11] drivers/md/dm-qcow2: do discards during backward merge only for writable image

[Konstantin Khorenko <khorenko@virtuozzo.com>]

as committed, a read-only backward merge of an image with internal snapshots
completes "successfully" while silently skipping every cluster under a shared L1 entry
(parse_l1() still refuses write-mode parsing of shared entries, so the merge qio ends with "nothing to merge") -

data loss once the source is dropped from the chain.
Patch 10 fixes exactly this and its own commit message confirms the data loss.
Suggest squashing/reordering, or rejecting RO sources with nb_snapshots != 0 in patch 9.

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 8/25/26 14:24, Andrey Zhadchenko wrote:
> Backward merge discards every merged cluster from the image we merge
> from: the L2 entry is zeroed and refcounts are decremented, which
> dirties the image metadata. This requires the image file to be opened
> for write, while merge-in-the-middle images usually opened read-only.
> 
> Skip the discard step when the merged image is read-only:
>  - complete the merge qio right after its data is written to the lower
> delta, without the L1/L2 entry update;
>  - process READs on such image in the regular way. Previously reads would
> cause out-of-order merge for present cluster and then requeue. Without
> discard it will loop.
>  - do not break COW at L1. Note that due to how merge machinery works,
> we can't merge without unuse and internal snaphots (to be addressed
> in the next patches).
>  - do not clear the dirty bit on merge completion: the image was never
> modified.
> 
> Just in case add warning and end qio if we somehow encounter non-service
> write qio for read-only images during the merge.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-138288
> Feature: dm-qcow2: block device over QCOW2 files driver
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
> ---
>  drivers/md/dm-qcow2-cmd.c | 21 +++++++++++++--------
>  drivers/md/dm-qcow2-map.c | 27 ++++++++++++++++++++++++++-
>  drivers/md/dm-qcow2.h     |  5 +++++
>  3 files changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
> index c15c46a0fe8b7..fa3762c58372b 100644
> --- a/drivers/md/dm-qcow2-cmd.c
> +++ b/drivers/md/dm-qcow2-cmd.c
> @@ -264,7 +264,7 @@ static int qcow2_merge_backward_start(struct qcow2_target *tgt, int efd, u32 dep
>  	lower = qcow2->lower;
>  	if (!lower)
>  		return -ENOENT;
> -	if (!(lower->file->f_mode & FMODE_WRITE))
> +	if (!qcow2_file_is_writable(lower))
>  		return -EACCES;
>  	if (qcow2->clu_size != lower->clu_size)
>  		return -EOPNOTSUPP;
> @@ -315,11 +315,14 @@ void qcow2_merge_backward_work(struct work_struct *work)
>  	 * there would be problems with unusing them:
>  	 * we'd have to freeze IO going to all data clusters
>  	 * under every L1 entry related to several snapshots.
> +	 * Readonly images skip this stage.
>  	 */
> -	ret = qcow2_break_l1cow(tgt, qcow2);
> -	if (ret) {
> -		QC_ERR(tgt->ti, "Can't break L1 COW");
> -		goto out_err;
> +	if (qcow2_file_is_writable(qcow2)) {
> +		ret = qcow2_break_l1cow(tgt, qcow2);
> +		if (ret) {
> +			QC_ERR(tgt->ti, "Can't break L1 COW");
> +			goto out_err;
> +		}
>  	}
>  
>  	backward_merge_update_stage(tgt, BACKWARD_MERGE_STAGE_SET_DIRTY);
> @@ -388,9 +391,11 @@ static int qcow2_merge_backward_complete(struct qcow2_target *tgt)
>  	qcow2_flush_deferred_activity(tgt, qcow2); /* Delayed md pages */
>  	qcow2->lower = NULL;
>  
> -	ret = qcow2_set_image_file_features(qcow2, false);
> -	if (ret < 0)
> -		QC_ERR(tgt->ti, "Can't unuse merged img (%d)", ret);
> +	if (qcow2_file_is_writable(qcow2)) {
> +		ret = qcow2_set_image_file_features(qcow2, false);
> +		if (ret < 0)
> +			QC_ERR(tgt->ti, "Can't unuse merged img (%d)", ret);
> +	}
>  	qcow2_destroy(qcow2);
>  
>  	tgt->backward_merge.state = BACKWARD_MERGE_STOPPED;
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index aa11956ac1d8c..c790b2ad3787e 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -2688,6 +2688,12 @@ static void backward_merge_write_complete(struct qcow2_target *tgt, struct qio *
>  		return;
>  	}
>  
> +	/* Skip discard for read-only source images */
> +	if (!qcow2_file_is_writable(qcow2)) {
> +		qio_endio(qio);
> +		return;
> +	}
> +
>  	WARN_ON_ONCE(qio->flags & QIO_IS_DISCARD_FL);
>  	qio->flags |= QIO_IS_DISCARD_FL;
>  
> @@ -2731,6 +2737,17 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
>  	struct qio *aux_qio;
>  	int ret;
>  
> +	/* Readonly image mappings remain stable, so reads just go through */
> +	if (!qcow2_file_is_writable(qcow2)) {
> +		if (!op_is_write((*qio)->bi_op))
> +			return 1;
> +		if (WARN_ON_ONCE(!fake_merge_qio(*qio))) {
> +			(*qio)->bi_status = BLK_STS_IOERR;
> +			qio_endio(*qio);
> +			return 0;
> +		}
> +	}
> +
>  	if (!map->data_clu_alloced) {
>  		WARN_ON_ONCE(map->clu_is_cow); /* Strange COW at L1 */
>  		if (fake_merge_qio(*qio)) {
> @@ -3640,7 +3657,15 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
>  	if (!handle_metadata(qcow2, &qio, &map))
>  		return;
>  
> -	if (unlikely(qcow2->backward_merge_in_process)) {
> +	/*
> +	 * Merge machinery makes out of order merges for present
> +	 * clusters when it sees the reads. But if the merge does
> +	 * not discard the cluser mapping, it will spin endlessly.
> +	 * So process only actual merge qios or reads from writable
> +	 * images.
> +	 */
> +	if (unlikely(qcow2->backward_merge_in_process) &&
> +	    (fake_merge_qio(qio) || qcow2_file_is_writable(qcow2))) {
>  		submit_top_delta_read(&map, qio);
>  		return;
>  	}
> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
> index 0f006f1ae48cc..d9b8c38e093f5 100644
> --- a/drivers/md/dm-qcow2.h
> +++ b/drivers/md/dm-qcow2.h
> @@ -460,6 +460,11 @@ static inline bool qcow2_wants_check(struct qcow2_target *tgt)
>  	return !!(tgt->md_writeback_error|tgt->truncate_error);
>  }
>  
> +static inline bool qcow2_file_is_writable(struct qcow2 *qcow2)
> +{
> +	return qcow2->file->f_mode & FMODE_WRITE;
> +}
> +
>  static inline void remap_to_clu(struct qcow2 *qcow2, struct qio *qio, loff_t clu_pos)
>  {
>  	qio->bi_iter.bi_sector &= (to_sector(qcow2->clu_size) - 1);