[Devel] [PATCH VZ10 08/10] drivers/md/dm-qcow2: do discards during backward merge only for writable image

[Andrey Zhadchenko]

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 at virtuozzo.com>
---
 drivers/md/dm-qcow2-cmd.c | 23 +++++++++++++++--------
 drivers/md/dm-qcow2-map.c | 27 ++++++++++++++++++++++++++-
 drivers/md/dm-qcow2.h     |  5 +++++
 3 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
index c15c46a0fe8b7..f2c8f51e17af6 100644
--- a/drivers/md/dm-qcow2-cmd.c
+++ b/drivers/md/dm-qcow2-cmd.c
@@ -264,7 +264,9 @@ 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_file_is_writable(qcow2) && qcow2->hdr.nb_snapshots)
 		return -EACCES;
 	if (qcow2->clu_size != lower->clu_size)
 		return -EOPNOTSUPP;
@@ -315,11 +317,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 +393,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 f80504927eaae..aa30d12f8ee04 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)) {
@@ -3630,7 +3647,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);
-- 
2.43.5