From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konstantin Khorenko Date: Tue, 25 Aug 2026 19:12:18 +0200 Subject: Re: [Devel] [PATCH VZ10 v3 09/11] drivers/md/dm-qcow2: do discards during backward merge only for writable image In-Reply-To: <20260825122430.252094-10-andrey.zhadchenko@virtuozzo.com> References: <20260825122430.252094-1-andrey.zhadchenko@virtuozzo.com> <20260825122430.252094-10-andrey.zhadchenko@virtuozzo.com> Message-ID: <6b94991c-a771-4b20-842c-3bd2e48da063@virtuozzo.com> List-Id: 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 > --- > 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);