[Devel] [PATCH 0/7] drivers/md/dm-qcow2: per-qio fast path

[Devel] [PATCH 0/7] drivers/md/dm-qcow2: per-qio fast path

[Andrey Zhadchenko]

dm-qcow2 processes every request consequentially in a single kwork.
Unfortunately this introduces latency and cpu-bound limits. This
patchset addresses the problem by laying per-qio tasks foundation:
qios can be executed in parallel on the same workqueue. For now cover
only requests with metadata fully allocated.

https://virtuozzo.atlassian.net/browse/VSTOR-95494

Andrey Zhadchenko (7):
  drivers/md/dm-qcow2: repeat md page lookup if it was inserted in
    parallel
  drivers/md/dm-qcow2: check MD_DIRTY and md->wbd under lock in
    perform_rw_mapped()
  drivers/md/dm-qcow2: prepare process_one_qio() for fastpath
  drivers/md/dm-qcow2: add per-qio work infrastructure
  drivers/md/dm-qcow2: implement per-qio fast processing
  drivers/md/dm-qcow2: declare DM_TARGET_NOWAIT
  drivers/md/dm-qcow2: process fast path qios in per-cpu batches on a
    workqueue

 drivers/md/dm-qcow2-map.c    | 214 +++++++++++++++++++++++++++++------
 drivers/md/dm-qcow2-target.c |  12 +-
 drivers/md/dm-qcow2.h        |  10 ++
 3 files changed, 197 insertions(+), 39 deletions(-)

-- 
2.43.5

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

[Devel] [PATCH hci-8.1 1/7] drivers/md/dm-qcow2: repeat md page lookup if it was inserted in parallel

[Andrey Zhadchenko]

teach __handle_md_page() to work around -EEXIST by retrying.

https://virtuozzo.atlassian.net/browse/VSTOR-95494
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-qcow2-map.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 78ee7cf5e8817..fed6ce75e98ad 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -1596,20 +1596,23 @@ static int submit_read_md_page(struct qcow2 *qcow2, struct qio **qio,
 /*
  * This may be called with @qio == NULL, in case of we are
  * interesting in searching cached in memory md only.
- * This is aimed to be called not only from main kwork
- * for L1/L2 pages, so all callers looking for L1/L2
- * must care about submit_read_md_page() may return EEXIST.
  */
 static int __handle_md_page(struct qcow2 *qcow2, u64 page_id,
 			    struct qio **qio, struct md_page **ret_md)
 {
 	struct md_page *md;
+	int ret;
 
 	lockdep_assert_held(&qcow2->md_pages_lock);
+again:
 	md = qcow2_md_page_find_or_postpone(qcow2, page_id, qio);
 	if (!md) {
-		if (qio && *qio)
-			return submit_read_md_page(qcow2, qio, page_id);
+		if (qio && *qio) {
+			ret = submit_read_md_page(qcow2, qio, page_id);
+			if (unlikely(ret == -EEXIST))
+				goto again;
+			return ret;
+		}
 		return 0;
 	}
 
-- 
2.43.5

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

[Devel] [PATCH hci-8.1 2/7] drivers/md/dm-qcow2: check MD_DIRTY and md->wbd under lock in perform_rw_mapped()

[Andrey Zhadchenko]

Access to md fields was relying on dm-qcow2 sequential nature. Expand
md_pages_lock to also cover it so it can run in parallel.

https://virtuozzo.atlassian.net/browse/VSTOR-95494
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-qcow2-map.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index fed6ce75e98ad..1023224484f5b 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2908,14 +2908,18 @@ static void perform_rw_mapped(struct qcow2_map *map, struct qio *qio)
 	 * wait for md writeback completion.
 	 *
 	 * L1/L2 pages become set and unset dirty from main work only,
-	 * so lock is not needed for MD_DIRTY/changed_indexes visibility.
+	 * but this may be called from per-qio fast path in parallel
+	 * with main work submitting the writeback and with its completion
+	 * dropping md->wbd.
 	 */
 	index_in_page = map->l2.index_in_page + !!(qcow2->ext_l2);
-	if (rw == WRITE && (md->status & MD_DIRTY) &&
-	    test_bit(index_in_page, md->wbd->changed_indexes)) {
+	if (rw == WRITE) {
 		spin_lock_irqsave(&qcow2->md_pages_lock, flags);
-		md->wbd->nr_submitted++;
-		qio->data = md->wbd;
+		if ((md->status & MD_DIRTY) &&
+		    test_bit(index_in_page, md->wbd->changed_indexes)) {
+			md->wbd->nr_submitted++;
+			qio->data = md->wbd;
+		}
 		spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
 	}
 
-- 
2.43.5

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

[Devel] [PATCH hci-8.1 3/7] drivers/md/dm-qcow2: prepare process_one_qio() for fastpath

[Andrey Zhadchenko]

dm-qcow2 heavily relies on main kwork do_qcow2_work() being
sequential. All metadata change happens there and cannot
run in parallel. Unfortunately that caps dm-qcow2 performance.
But if the request does not require metadata changes we should
be able to immediately dispatch it.
process_one_qio() and handle_metadata() already know what to
do: just add avoid_metadata_changes() flag and return codes.
While at it, just return in handle_metadata() instead of
setting ret to 0 and jumping to if (ret < 0) label.

Also exclude compressed clusters as there are some complications
with wpc_readers.

https://virtuozzo.atlassian.net/browse/VSTOR-95494
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-qcow2-map.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 1023224484f5b..c5d68b6b21f12 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -3534,7 +3534,7 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
 }
 
 static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
-			   struct qcow2_map *map)
+			   struct qcow2_map *map, bool avoid_metadata_changes)
 {
 	bool write = op_is_write((*qio)->bi_op);
 	int ret;
@@ -3543,15 +3543,20 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
 	if (ret < 0 || !*qio) /* Error or postponed */
 		goto check_err;
 
+	if (avoid_metadata_changes && map->compressed)
+		return 2;
+
 	ret = 1;
 	if (unlikely(qcow2->backward_merge_in_process)) {
+		if (avoid_metadata_changes)
+			return 2;
 		/* Keep in mind the below may replace *qio */
 		ret = prepare_backward_merge(qcow2, qio, map, write);
 	} else if (unlikely(fake_l1cow_qio(*qio)) &&
 		(!map->clu_is_cow || (map->level & L2_LEVEL))) {
 		/* Nothing to COW or L1 is mapped exactly once */
 		qio_endio(*qio);
-		ret = 0;
+		return 0;
 	} else if (unlikely(op_is_discard((*qio)->bi_op))) {
 		/*
 		 * Discard carries no data and is advisory. If it does not
@@ -3559,13 +3564,17 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
 		 * is not present, end early.
 		 */
 		if (qio_discard_updates_metadata(qcow2, *qio, map)) {
+			if (avoid_metadata_changes)
+				return 2;
 			ret = prepare_cluster_discard(qcow2, qio, map);
 		} else if (!map->data_clu_alloced || map->clu_is_cow) {
 			qio_endio(*qio);
-			ret = 0;
+			return 0;
 		}
 	} else if (write &&
 		   (!qio_is_fully_alloced(qcow2, *qio, map) || map->clu_is_cow)) {
+		if (avoid_metadata_changes)
+			return 2;
 		if (map->clu_is_cow) {
 			/* COW to compressed or shared with snapshot cluster */
 			ret = prepare_l1l2_replace(qcow2, *qio, map);
@@ -3677,13 +3686,17 @@ static void process_read_qio(struct qcow2 *qcow2, struct qio *qio,
 	qio_endio(qio);
 }
 
-static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
+static int process_one_qio(struct qcow2 *qcow2, struct qio *qio, bool avoid_metadata_changes)
 {
 	struct qcow2_map map = { .qcow2 = qcow2, };
 	bool write;
+	int ret;
 
-	if (!handle_metadata(qcow2, &qio, &map))
-		return;
+	ret = handle_metadata(qcow2, &qio, &map, avoid_metadata_changes);
+	if (!ret)
+		return 0;
+	else if (ret == 2)
+		return 1;
 
 	/*
 	 * Merge machinery makes out of order merges for present
@@ -3695,7 +3708,7 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
 	if (unlikely(qcow2->backward_merge_in_process) &&
 	    (fake_merge_qio(qio) || qcow2_file_is_writable(qcow2))) {
 		submit_top_delta_read(&map, qio);
-		return;
+		return 0;
 	}
 
 	write = op_is_write(qio->bi_op);
@@ -3703,13 +3716,13 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
 	/* Process discard early. Don't do COW just to discard */
 	if (unlikely(op_is_discard(qio->bi_op))) {
 		issue_discard(&map, qio);
-		return;
+		return 0;
 	}
 
 	if (unlikely(map.compressed)) {
 		/* Compressed qio never uses sub-clus */
 		submit_read_compressed(&map, qio, write);
-		return;
+		return 0;
 	}
 
 	if (!write) {
@@ -3722,6 +3735,8 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
 		else
 			perform_rw_mapped(&map, qio);
 	}
+
+	return 0;
 }
 
 static struct bio_vec *create_bvec_from_rq(struct request *rq)
@@ -3824,7 +3839,7 @@ static void process_deferred_qios(struct qcow2 *qcow2, struct list_head *qios)
 			continue;
 		}
 
-		process_one_qio(qcow2, qio);
+		process_one_qio(qcow2, qio, false);
 	}
 }
 
-- 
2.43.5

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

[Devel] [PATCH hci-8.1 4/7] drivers/md/dm-qcow2: add per-qio work infrastructure

[Andrey Zhadchenko]

dm-qcow2 was created to run non-concurrently, so qio never had
per work struct field. Let's add one and use it to queue first
dm-qcow2 pipeline step: qio preparation. It requires no metadata,
so everything is simple here.
We would later move more pipeline steps into qio.

https://virtuozzo.atlassian.net/browse/VSTOR-95494
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-qcow2-map.c | 60 ++++++++++++++++++++++++++++-----------
 drivers/md/dm-qcow2.h     |  6 ++++
 2 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index c5d68b6b21f12..7afaba0afec14 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -362,6 +362,8 @@ struct qio *qcow2_alloc_qio(mempool_t *pool, bool zero)
 }
 ALLOW_ERROR_INJECTION(qcow2_alloc_qio, NULL);
 
+static void do_qio_work(struct work_struct *ws);
+
 void qcow2_init_qio(struct qio *qio, unsigned int bi_op, struct qcow2 *qcow2)
 {
 	qio->qcow2 = qcow2;
@@ -372,7 +374,9 @@ void qcow2_init_qio(struct qio *qio, unsigned int bi_op, struct qcow2 *qcow2)
 	qio->bi_io_vec = NULL;
 	qio->flags = 0;
 	qio->ref_index = REF_INDEX_INVALID;
+	qio->state = QIO_STATE_START;
 	atomic_set(&qio->remaining, 1);
+	INIT_WORK(&qio->worker, do_qio_work);
 
 	/*
 	 * Initially set into BLK_STS_OK, while aio complete,
@@ -3770,10 +3774,10 @@ static void qio_init_iter_from_rq(struct qio *qio, struct request *rq)
 	qio->bi_iter.bi_bvec_done = 0;
 }
 
-static void prepare_one_embedded_qio(struct qcow2 *qcow2, struct qio *qio,
-				     struct list_head *deferred_qios)
+static void prepare_one_embedded_qio(struct qio *qio, struct list_head *deferred_qios)
 {
 	struct qcow2_rq *qrq = qio->endio_cb_data;
+	struct qcow2 *qcow2 = qio->qcow2;
 	struct request *rq = qrq->rq;
 	struct bio_vec *bvec = NULL;
 	LIST_HEAD(list);
@@ -3818,15 +3822,6 @@ static void prepare_one_embedded_qio(struct qcow2 *qcow2, struct qio *qio,
 	qio_endio(qio);
 }
 
-static void process_embedded_qios(struct qcow2 *qcow2, struct list_head *qios,
-				  struct list_head *deferred_qios)
-{
-	struct qio *qio;
-
-	while ((qio = qio_list_pop(qios)))
-		prepare_one_embedded_qio(qcow2, qio, deferred_qios);
-}
-
 static void process_deferred_qios(struct qcow2 *qcow2, struct list_head *qios)
 {
 	struct qio *qio;
@@ -4343,7 +4338,6 @@ static void process_seek_qios(struct qcow2 *qcow, struct list_head *qios)
 void do_qcow2_work(struct work_struct *ws)
 {
 	struct qcow2 *qcow2 = container_of(ws, struct qcow2, worker);
-	LIST_HEAD(embedded_qios);
 	LIST_HEAD(deferred_qios);
 	LIST_HEAD(zread_qios);
 	LIST_HEAD(bwrite_qios);
@@ -4356,7 +4350,6 @@ void do_qcow2_work(struct work_struct *ws)
 
 	current->flags |= PF_LOCAL_THROTTLE|PF_MEMALLOC_NOIO;
 	spin_lock_irq(&qcow2->deferred_lock);
-	list_splice_init(&qcow2->qios[QLIST_EMBEDDED], &embedded_qios);
 	list_splice_init(&qcow2->qios[QLIST_DEFERRED], &deferred_qios);
 	list_splice_init(&qcow2->qios[QLIST_ZREAD], &zread_qios);
 	list_splice_init(&qcow2->qios[QLIST_BMERGE_WRITE], &bwrite_qios);
@@ -4367,7 +4360,6 @@ void do_qcow2_work(struct work_struct *ws)
 	list_splice_init(&qcow2->qios[QLIST_SEEK], &seek_qios);
 	spin_unlock_irq(&qcow2->deferred_lock);
 
-	process_embedded_qios(qcow2, &embedded_qios, &deferred_qios);
 	process_deferred_qios(qcow2, &deferred_qios);
 	process_compressed_read(&zread_qios, &cow_data_qios);
 	process_backward_merge_write(qcow2, &bwrite_qios);
@@ -4491,7 +4483,7 @@ static void qcow2_submit_embedded_qio(struct qcow2_target *tgt, struct qio *qio)
 
 	if (blk_rq_bytes(rq)) {
 		queue_list_id = QLIST_EMBEDDED;
-		worker = &qcow2->worker;
+		worker = &qio->worker;
 	} else {
 		WARN_ON_ONCE(qio->bi_op != REQ_OP_FLUSH);
 		queue_list_id = QLIST_FLUSH;
@@ -4507,7 +4499,10 @@ static void qcow2_submit_embedded_qio(struct qcow2_target *tgt, struct qio *qio)
 		qio->qcow2 = qcow2;
 		qio->queue_list_id = queue_list_id;
 		qio->ref_index = ref_index;
-		list_add_tail(&qio->link, &qcow2->qios[qio->queue_list_id]);
+		if (queue_list_id == QLIST_EMBEDDED)
+			qio->state = QIO_STATE_START;
+		else
+			list_add_tail(&qio->link, &qcow2->qios[qio->queue_list_id]);
 	}
 	spin_unlock_irqrestore(&qcow2->deferred_lock, flags);
 
@@ -4599,6 +4594,39 @@ static void handle_cleanup_mask(struct qio *qio)
 	}
 }
 
+static void do_qio_work(struct work_struct *ws)
+{
+	struct qio *qio = container_of(ws, struct qio, worker);
+	struct qcow2 *qcow2 = qio->qcow2;
+	struct qcow2_target *tgt = qcow2->tgt;
+	unsigned int pflags = current->flags;
+
+	current->flags |= PF_LOCAL_THROTTLE|PF_MEMALLOC_NOIO;
+
+	switch (qio->state) {
+	case QIO_STATE_START: {
+		LIST_HEAD(deferred_qios);
+
+		prepare_one_embedded_qio(qio, &deferred_qios);
+		if (list_empty(&deferred_qios))
+			break;
+
+		spin_lock_irq(&qcow2->deferred_lock);
+		list_splice_tail(&deferred_qios, &qcow2->qios[QLIST_DEFERRED]);
+		spin_unlock_irq(&qcow2->deferred_lock);
+		queue_work(tgt->wq, &qcow2->worker);
+		break;
+	}
+
+	default:
+		qio->bi_status = BLK_STS_IOERR;
+		qio_endio(qio);
+		break;
+	}
+
+	current_restore_flags(pflags, PF_LOCAL_THROTTLE|PF_MEMALLOC_NOIO);
+}
+
 struct qio_data_llseek_hole {
 	struct completion compl;
 	struct qio *higher;
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index d9b8c38e093f5..003e01a4d3ec9 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -236,6 +236,10 @@ enum {
 	QLIST_INVALID = QLIST_COUNT,
 };
 
+enum {
+	QIO_STATE_START = 0,
+};
+
 struct qcow2 {
 	struct qcow2_target *tgt;
 	struct file *file;
@@ -369,6 +373,8 @@ struct qio {
 	struct qcow2 *qcow2;
 	qcow2_endio_t endio_cb;
 	void *endio_cb_data;
+	struct work_struct worker;
+	int state;
 };
 
 #define CLU_OFF(qcow2, pos) (pos & (qcow2->clu_size - 1))
-- 
2.43.5

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

[Devel] [PATCH hci-8.1 5/7] drivers/md/dm-qcow2: implement per-qio fast processing

[Andrey Zhadchenko]

Each qio is handled by it's own work and process_one_qio() can now
work with qios not requiring metadata. Try to process qios right
after splitting. Insert all leftovers into respective lists for
do_qcow2_work().

https://virtuozzo.atlassian.net/browse/VSTOR-95494
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-qcow2-map.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 7afaba0afec14..e55186147fdf7 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -4594,9 +4594,18 @@ static void handle_cleanup_mask(struct qio *qio)
 	}
 }
 
+static void qio_try_fast(struct qio *qio, struct list_head *deferred_qios)
+{
+	list_del(&qio->link);
+
+	if (process_one_qio(qio->qcow2, qio, true))
+		list_add(&qio->link, deferred_qios);
+}
+
 static void do_qio_work(struct work_struct *ws)
 {
 	struct qio *qio = container_of(ws, struct qio, worker);
+	struct qio *i, *next;
 	struct qcow2 *qcow2 = qio->qcow2;
 	struct qcow2_target *tgt = qcow2->tgt;
 	unsigned int pflags = current->flags;
@@ -4606,8 +4615,12 @@ static void do_qio_work(struct work_struct *ws)
 	switch (qio->state) {
 	case QIO_STATE_START: {
 		LIST_HEAD(deferred_qios);
+		LIST_HEAD(split_qios);
+
+		prepare_one_embedded_qio(qio, &split_qios);
+		list_for_each_entry_safe(i, next, &split_qios, link)
+			qio_try_fast(i, &deferred_qios);
 
-		prepare_one_embedded_qio(qio, &deferred_qios);
 		if (list_empty(&deferred_qios))
 			break;
 
-- 
2.43.5

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

[Devel] [PATCH hci-8.1 6/7] drivers/md/dm-qcow2: declare DM_TARGET_NOWAIT

[Andrey Zhadchenko]

dm-qcow2 is a request-based target: REQ_NOWAIT handling is done by blk-mq
(tag allocation) before a request reaches us, and clone_and_map_rq()
never blocks. Still, the target does not declare DM_TARGET_NOWAIT, so
dm_table_set_restrictions() clears BLK_FEAT_NOWAIT on the mapped device
and the block device file gets no FMODE_NOWAIT.

As a result io_uring is unable to issue I/O in nowait mode and punts
*every* request to io-wq worker threads: on 4k randwrite (io_uring,
qd=64, 4 jobs) io-wq threads burn ~24% of all cpus doing nothing but a
thread handoff.

                     before            after
  qd=64 nj=4 IOPS    1034K (11% idle)  892K (30% idle)
  qd=1  nj=1 lat     8us               6us

The lower nj=4 number is because io-wq threads were providing "free"
parallelism for the single-cpu submitters; the next patch gets it back
without burning the cpus.

https://virtuozzo.atlassian.net/browse/VSTOR-95494
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-qcow2-target.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index be09615eb8e2e..4b9ae86ac9443 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -1159,7 +1159,7 @@ static void qcow2_resume(struct dm_target *ti)
 static struct target_type qcow2_target = {
 	.name = "qcow2",
 	.version = {1, 0, 0},
-	.features = DM_TARGET_SINGLETON,
+	.features = DM_TARGET_SINGLETON | DM_TARGET_NOWAIT,
 	.module = THIS_MODULE,
 	.ctr = qcow2_ctr,
 	.dtr = qcow2_dtr,
-- 
2.43.5

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

[Devel] [PATCH hci-8.1 7/7] drivers/md/dm-qcow2: process fast path qios in per-cpu batches on a workqueue

[Andrey Zhadchenko]

queue_work() is pretty costly under high load. It takes pool->lock;
unbound pools do not track running workers (WORKER_UNBOUND is part
of WORKER_NOT_RUNNING), so an idle kworker is woken (idle exit + IPI +
context switch) even if a running worker would pick the work up right
away. PENDING is cleared when execution starts, so a qio re-queued
while its work runs wakes yet another worker just to find the collision
and go back to sleep. On a 12 cpu box with 4 submitters this shows up
as ~3 context switches per I/O, 5% of all cycles spinning on
pool->lock and 6% in the scheduler.

Batch requests with percpu llists. Additionally track if the worker
is busy and queue_work() only if it is not. Drain the whole llist in
the worker.

https://virtuozzo.atlassian.net/browse/VSTOR-95494
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-qcow2-map.c    | 99 ++++++++++++++++++++++++++++++++----
 drivers/md/dm-qcow2-target.c | 10 +++-
 drivers/md/dm-qcow2.h        |  6 ++-
 3 files changed, 102 insertions(+), 13 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index e55186147fdf7..4fb60520b955d 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -87,6 +87,7 @@ static void process_read_qio(struct qcow2 *qcow2, struct qio *qio,
 			     struct qcow2_map *map);
 static void init_qrq_and_embedded_qio(struct qcow2_target *tgt, struct request *rq,
 				      struct qcow2_rq *qrq, struct qio *qio);
+static void qcow2_fast_submit(struct qcow2_target *tgt, struct qio *qio);
 
 static loff_t bytes_off_in_cluster(struct qcow2 *qcow2, struct qio *qio)
 {
@@ -362,8 +363,6 @@ struct qio *qcow2_alloc_qio(mempool_t *pool, bool zero)
 }
 ALLOW_ERROR_INJECTION(qcow2_alloc_qio, NULL);
 
-static void do_qio_work(struct work_struct *ws);
-
 void qcow2_init_qio(struct qio *qio, unsigned int bi_op, struct qcow2 *qcow2)
 {
 	qio->qcow2 = qcow2;
@@ -376,7 +375,6 @@ void qcow2_init_qio(struct qio *qio, unsigned int bi_op, struct qcow2 *qcow2)
 	qio->ref_index = REF_INDEX_INVALID;
 	qio->state = QIO_STATE_START;
 	atomic_set(&qio->remaining, 1);
-	INIT_WORK(&qio->worker, do_qio_work);
 
 	/*
 	 * Initially set into BLK_STS_OK, while aio complete,
@@ -4483,7 +4481,7 @@ static void qcow2_submit_embedded_qio(struct qcow2_target *tgt, struct qio *qio)
 
 	if (blk_rq_bytes(rq)) {
 		queue_list_id = QLIST_EMBEDDED;
-		worker = &qio->worker;
+		worker = NULL;
 	} else {
 		WARN_ON_ONCE(qio->bi_op != REQ_OP_FLUSH);
 		queue_list_id = QLIST_FLUSH;
@@ -4506,8 +4504,13 @@ static void qcow2_submit_embedded_qio(struct qcow2_target *tgt, struct qio *qio)
 	}
 	spin_unlock_irqrestore(&qcow2->deferred_lock, flags);
 
-	if (queue)
+	if (!queue)
+		return;
+
+	if (worker)
 		queue_work(tgt->wq, worker);
+	else
+		qcow2_fast_submit(tgt, qio);
 }
 
 void qcow2_submit_embedded_qios(struct qcow2_target *tgt, struct list_head *list)
@@ -4602,15 +4605,11 @@ static void qio_try_fast(struct qio *qio, struct list_head *deferred_qios)
 		list_add(&qio->link, deferred_qios);
 }
 
-static void do_qio_work(struct work_struct *ws)
+static void do_qio_work(struct qio *qio)
 {
-	struct qio *qio = container_of(ws, struct qio, worker);
-	struct qio *i, *next;
 	struct qcow2 *qcow2 = qio->qcow2;
 	struct qcow2_target *tgt = qcow2->tgt;
-	unsigned int pflags = current->flags;
-
-	current->flags |= PF_LOCAL_THROTTLE|PF_MEMALLOC_NOIO;
+	struct qio *i, *next;
 
 	switch (qio->state) {
 	case QIO_STATE_START: {
@@ -4636,10 +4635,88 @@ static void do_qio_work(struct work_struct *ws)
 		qio_endio(qio);
 		break;
 	}
+}
+
+struct qcow2_fast_cpu {
+	struct llist_head qios;
+	struct work_struct work;
+	atomic_t busy;
+};
+
+static void qcow2_fast_submit(struct qcow2_target *tgt, struct qio *qio)
+{
+	struct qcow2_fast_cpu *fc = get_cpu_ptr(tgt->fast);
+
+	llist_add(&qio->fast_link, &fc->qios);
+	/* Pairs with smp_mb() in qcow2_fast_work() */
+	smp_mb();
+	if (!atomic_read(&fc->busy) && atomic_cmpxchg(&fc->busy, 0, 1) == 0)
+		queue_work(tgt->wq, &fc->work);
+	put_cpu_ptr(tgt->fast);
+}
+
+static void qcow2_fast_work(struct work_struct *work)
+{
+	struct qcow2_fast_cpu *fc = container_of(work, struct qcow2_fast_cpu, work);
+	unsigned int pflags = current->flags;
+	struct llist_node *node;
+	struct qio *qio, *next;
+
+	current->flags |= PF_LOCAL_THROTTLE|PF_MEMALLOC_NOIO;
+
+	for (;;) {
+		node = llist_del_all(&fc->qios);
+		if (!node) {
+			atomic_set(&fc->busy, 0);
+			/* just to be sure we look at the list after busy is posted */
+			smp_mb();
+			if (llist_empty(&fc->qios))
+				break;
+			if (atomic_cmpxchg(&fc->busy, 0, 1) != 0)
+				break;
+			continue;
+		}
+		node = llist_reverse_order(node);
+		llist_for_each_entry_safe(qio, next, node, fast_link)
+			do_qio_work(qio);
+	}
 
 	current_restore_flags(pflags, PF_LOCAL_THROTTLE|PF_MEMALLOC_NOIO);
 }
 
+void qcow2_fast_exit(struct qcow2_target *tgt)
+{
+	struct qcow2_fast_cpu *fc;
+	int cpu;
+
+	if (!tgt->fast)
+		return;
+	for_each_possible_cpu(cpu) {
+		fc = per_cpu_ptr(tgt->fast, cpu);
+		WARN_ON_ONCE(!llist_empty(&fc->qios));
+	}
+	free_percpu(tgt->fast);
+	tgt->fast = NULL;
+}
+
+int qcow2_fast_init(struct qcow2_target *tgt)
+{
+	struct qcow2_fast_cpu *fc;
+	int cpu;
+
+	tgt->fast = alloc_percpu(struct qcow2_fast_cpu);
+	if (!tgt->fast)
+		return -ENOMEM;
+	for_each_possible_cpu(cpu) {
+		fc = per_cpu_ptr(tgt->fast, cpu);
+		init_llist_head(&fc->qios);
+		INIT_WORK(&fc->work, qcow2_fast_work);
+		atomic_set(&fc->busy, 0);
+	}
+
+	return 0;
+}
+
 struct qio_data_llseek_hole {
 	struct completion compl;
 	struct qio *higher;
diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index 4b9ae86ac9443..290409db008b8 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -253,6 +253,7 @@ static void qcow2_tgt_destroy(struct qcow2_target *tgt)
 		/* Now kill the queue */
 		destroy_workqueue(tgt->wq);
 	}
+	qcow2_fast_exit(tgt);
 	qcow2_merge_backward_cancel(tgt);
 
 	mempool_destroy(tgt->qio_pool);
@@ -477,6 +478,11 @@ static struct qcow2_target *alloc_qcow2_target(struct dm_target *ti)
 		goto out_pool;
 	}
 
+	if (qcow2_fast_init(tgt)) {
+		ti->error = "Can't alloc fast path slots";
+		goto out_wq;
+	}
+
 	for (i = 0; i < 2; i++) {
 		release = i ? inflight_ref_exit1 : inflight_ref_exit0;
 		if (percpu_ref_init(&tgt->inflight_ref[i], release,
@@ -484,7 +490,7 @@ static struct qcow2_target *alloc_qcow2_target(struct dm_target *ti)
 			if (i)
 				percpu_ref_exit(&tgt->inflight_ref[0]);
 			ti->error = "could not alloc percpu_ref";
-			goto out_wq;
+			goto out_fast;
 		}
 	}
 
@@ -503,6 +509,8 @@ static struct qcow2_target *alloc_qcow2_target(struct dm_target *ti)
 	qcow2_set_service_operations(ti, false);
 
 	return tgt;
+out_fast:
+	qcow2_fast_exit(tgt);
 out_wq:
 	destroy_workqueue(tgt->wq);
 out_pool:
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index 003e01a4d3ec9..6e2ac8d9979fd 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -4,6 +4,7 @@
 
 #include <linux/percpu-refcount.h>
 #include <linux/device-mapper.h>
+#include <linux/llist.h>
 #include <linux/fs.h>
 #include <linux/eventfd.h>
 
@@ -192,6 +193,7 @@ struct qcow2_target {
 	 */
 	struct qcow2 *top;
 	struct workqueue_struct *wq;
+	struct qcow2_fast_cpu __percpu *fast;
 
 	struct completion inflight_ref_comp;
 	struct percpu_ref inflight_ref[2];
@@ -373,7 +375,7 @@ struct qio {
 	struct qcow2 *qcow2;
 	qcow2_endio_t endio_cb;
 	void *endio_cb_data;
-	struct work_struct worker;
+	struct llist_node fast_link;
 	int state;
 };
 
@@ -390,6 +392,8 @@ loff_t qcow2_llseek_hole(struct dm_target *ti, loff_t offset, int whence);
 
 void do_qcow2_work(struct work_struct *ws);
 void do_qcow2_fsync_work(struct work_struct *ws);
+int qcow2_fast_init(struct qcow2_target *tgt);
+void qcow2_fast_exit(struct qcow2_target *tgt);
 int qcow2_alloc_and_insert_md_page(struct qcow2 *qcow2, u64 index,
 					struct md_page **md);
 struct md_page *qcow2_md_page_find_or_postpone(struct qcow2 *qcow2,
-- 
2.43.5

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

Re: [Devel] [PATCH hci-8.1 1/7] drivers/md/dm-qcow2: repeat md page lookup if it was inserted in parallel

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-qcow2: __handle_md_page() retries on -EEXIST with no bound or progress check
--

commit 3961a8a0138999bca60a809cb24928991567bdd8
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

drivers/md/dm-qcow2: repeat md page lookup if it was inserted in parallel

This patch makes __handle_md_page() retry the md page lookup when submit_read_md_page() returns -EEXIST. It also removes the comment telling callers to handle -EEXIST themselves.

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

> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index 78ee7cf5e8817..fed6ce75e98ad 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c

[ ... ]

> @@ -1596,20 +1596,23 @@ static int submit_read_md_page(struct qcow2 *qcow2, struct qio **qio,
>  /*
>   * This may be called with @qio == NULL, in case of we are
>   * interesting in searching cached in memory md only.
> - * This is aimed to be called not only from main kwork
> - * for L1/L2 pages, so all callers looking for L1/L2
> - * must care about submit_read_md_page() may return EEXIST.
>   */
>  static int __handle_md_page(struct qcow2 *qcow2, u64 page_id,
>  			    struct qio **qio, struct md_page **ret_md)
>  {
>  	struct md_page *md;
> +	int ret;
>  
>  	lockdep_assert_held(&qcow2->md_pages_lock);
> +again:
>  	md = qcow2_md_page_find_or_postpone(qcow2, page_id, qio);
>  	if (!md) {
> -		if (qio && *qio)
> -			return submit_read_md_page(qcow2, qio, page_id);
> +		if (qio && *qio) {
> +			ret = submit_read_md_page(qcow2, qio, page_id);
> +			if (unlikely(ret == -EEXIST))
> +				goto again;
> +			return ret;
> +		}
>  		return 0;
>  	}

[Severity: Low]
Can this retry loop in __handle_md_page() run forever? The goto again
is taken on every -EEXIST from submit_read_md_page(), with no retry limit,
no cond_resched(), and no check that the page has actually become visible
to the lookup.

Organically, -EEXIST comes from md_page_try_insert() under md_pages_lock.
The retry's qcow2_md_page_find_or_postpone() then takes the same lock and
finds the existing node, so the loop should end after one extra pass.

However, two functions in dm-qcow2-target.c are error-injectable:

    ALLOW_ERROR_INJECTION(qcow2_alloc_and_insert_md_page, ERRNO);
    ALLOW_ERROR_INJECTION(qcow2_md_page_find_or_postpone, NULL);

If an injected -EEXIST is returned from qcow2_alloc_and_insert_md_page(),
nothing has been inserted, so each retry misses the lookup and gets
-EEXIST again. If NULL is injected into qcow2_md_page_find_or_postpone(),
an existing page looks missing and md_page_try_insert() fails again on
every pass.

In both cases the calling kwork or submitting thread would spin in
__handle_md_page(). Each pass takes and drops md_pages_lock, does a
kmalloc(GFP_KERNEL) and an alloc_page(GFP_KERNEL), then frees both.

This needs privileged fault injection (fail_function or BPF override), so
there is no organic trigger. Before this patch, an injected -EEXIST was
returned to the caller as an error. After it, the same injection becomes a
hang.

Would a bounded retry, or a check that the page is now present before
looping, be more appropriate here?

The comment removed from above __handle_md_page() also described
-EEXIST handling for callers. Is it still accurate that no caller needs to
handle -EEXIST from this function?

The final state of the series leaves __handle_md_page() and this retry
unchanged, so nothing later in the series addresses this.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029140346.465768-1-andrey.zhadchenko@virtuozzo.com?part=1

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

Re: [Devel] [PATCH hci-8.1 2/7] drivers/md/dm-qcow2: check MD_DIRTY and md->wbd under lock in perform_rw_mapped()

[no-reply]

> Access to md fields was relying on dm-qcow2 sequential nature. Expand
> md_pages_lock to also cover it so it can run in parallel.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-95494
> Feature: dm-qcow2: block device over QCOW2 files driver
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029140346.465768-1-andrey.zhadchenko@virtuozzo.com?part=2


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

Re: [Devel] [PATCH hci-8.1 3/7] drivers/md/dm-qcow2: prepare process_one_qio() for fastpath

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-qcow2: undocumented magic return codes 0/1/2 in handle_metadata()
--

commit c3720b83541dd3fa0f6258c37f4dfb59dfae2146
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

drivers/md/dm-qcow2: prepare process_one_qio() for fastpath

This patch adds an avoid_metadata_changes flag to handle_metadata() and
process_one_qio(). When the flag is set and the qio would need a
metadata change or touches a compressed cluster, the functions return a
new code (2 from handle_metadata(), 1 from process_one_qio()) so that
the caller can fall back to the main kwork. process_deferred_qios()
passes false, so its behaviour is unchanged.

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

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

[ ... ]

> @@ -3543,15 +3543,20 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
>  	ret = parse_metadata(qcow2, qio, map);
>  	if (ret < 0 || !*qio) /* Error or postponed */
>  		goto check_err;
>  
> +	if (avoid_metadata_changes && map->compressed)
> +		return 2;
> +

[Severity: Low]
This isn't a functional bug, but handle_metadata() now returns the bare
literal 2 here and at several other sites. Its existing return value 1
means "continue processing", and the new 2 means "needs the slow path".

Could these be given named constants, or could the return contract be
documented in a comment above handle_metadata()?

[ ... ]

> -static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
> +static int process_one_qio(struct qcow2 *qcow2, struct qio *qio, bool avoid_metadata_changes)
>  {
>  	struct qcow2_map map = { .qcow2 = qcow2, };
>  	bool write;
> +	int ret;
>  
> -	if (!handle_metadata(qcow2, &qio, &map))
> -		return;
> +	ret = handle_metadata(qcow2, &qio, &map, avoid_metadata_changes);
> +	if (!ret)
> +		return 0;
> +	else if (ret == 2)
> +		return 1;

[Severity: Low]
Here the value 2 from handle_metadata() is turned into a return value of
1 from process_one_qio(), which means "not handled, requeue". The same
number 1 therefore means "continue" in handle_metadata() and "requeue" in
process_one_qio().

Would named constants or a comment describing the return values of
process_one_qio() make this clearer?

This isn't a bug, but the new process_one_qio() signature is also longer
than 80 columns:

static int process_one_qio(struct qcow2 *qcow2, struct qio *qio, bool avoid_metadata_changes)

Could it be wrapped?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029140346.465768-1-andrey.zhadchenko@virtuozzo.com?part=3

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

Re: [Devel] [PATCH hci-8.1 4/7] drivers/md/dm-qcow2: add per-qio work infrastructure

[no-reply]

> dm-qcow2 was created to run non-concurrently, so qio never had
> per work struct field. Let's add one and use it to queue first
> dm-qcow2 pipeline step: qio preparation. It requires no metadata,
> so everything is simple here.
> We would later move more pipeline steps into qio.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-95494
> Feature: dm-qcow2: block device over QCOW2 files driver
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029140346.465768-1-andrey.zhadchenko@virtuozzo.com?part=4


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

Re: [Devel] [PATCH hci-8.1 6/7] drivers/md/dm-qcow2: declare DM_TARGET_NOWAIT

[no-reply]

> dm-qcow2 is a request-based target: REQ_NOWAIT handling is done by blk-mq
> (tag allocation) before a request reaches us, and clone_and_map_rq()
> never blocks. Still, the target does not declare DM_TARGET_NOWAIT, so
> dm_table_set_restrictions() clears BLK_FEAT_NOWAIT on the mapped device
> and the block device file gets no FMODE_NOWAIT.
> 
> As a result io_uring is unable to issue I/O in nowait mode and punts
> *every* request to io-wq worker threads: on 4k randwrite (io_uring,
> qd=64, 4 jobs) io-wq threads burn ~24% of all cpus doing nothing but a
> thread handoff.
> 
>                      before            after
>   qd=64 nj=4 IOPS    1034K (11% idle)  892K (30% idle)
>   qd=1  nj=1 lat     8us               6us
> 
> The lower nj=4 number is because io-wq threads were providing "free"
> parallelism for the single-cpu submitters; the next patch gets it back
> without burning the cpus.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-95494
> Feature: dm-qcow2: block device over QCOW2 files driver
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029140346.465768-1-andrey.zhadchenko@virtuozzo.com?part=6


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

Re: [Devel] [PATCH hci-8.1 5/7] drivers/md/dm-qcow2: implement per-qio fast processing

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-qcow2: qio_try_fast() uses list_add() and reverses the order of deferred sibling splits
--

commit 3954eb13b0c2923af1fc503136a833ede7213254
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

drivers/md/dm-qcow2: implement per-qio fast processing

This commit makes do_qio_work() split a qio and then try to process each
piece right away with process_one_qio(..., true). Pieces that still need
metadata work are put on a local list and spliced onto
qcow2->qios[QLIST_DEFERRED] for do_qcow2_work().

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

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

[ ... ]

> +static void qio_try_fast(struct qio *qio, struct list_head *deferred_qios)
> +{
> +	list_del(&qio->link);
> +
> +	if (process_one_qio(qio->qcow2, qio, true))
> +		list_add(&qio->link, deferred_qios);

[Severity: Low]
Can this list_add() reverse the order of the deferred pieces?

qcow2_split_qio_to_list() builds the split list in ascending order, with
the parent last:

    list_splice_tail(&list, ret_list);
    list_add_tail(&qio->link, ret_list);

do_qio_work() walks that list front to back and calls qio_try_fast() on
each entry. Since list_add() inserts at the head of deferred_qios, each
piece that returns 1 from process_one_qio() lands in front of the earlier
ones. deferred_qios therefore ends up in descending order.

That list is then spliced to qcow2->qios[QLIST_DEFERRED] with
list_splice_tail(), so the slow-path pieces of one request are queued in
reverse order. Before this patch, prepare_one_embedded_qio() filled the
deferred list directly, and the order was preserved.

I did not find a correctness problem. Each piece covers a different
cluster, and process_deferred_qios() does not depend on order. The visible
effect is that cluster and metadata allocation and submission happen in
reverse order for large writes that need allocation.

Would list_add_tail() be more appropriate here to keep the original order?

> +}
> +
>  static void do_qio_work(struct work_struct *ws)
>  {

[ ... ]

> +		prepare_one_embedded_qio(qio, &split_qios);
> +		list_for_each_entry_safe(i, next, &split_qios, link)
> +			qio_try_fast(i, &deferred_qios);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029140346.465768-1-andrey.zhadchenko@virtuozzo.com?part=5

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

Re: [Devel] [PATCH hci-8.1 7/7] drivers/md/dm-qcow2: process fast path qios in per-cpu batches on a workqueue

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-qcow2: per-CPU fast-path worker serializes blocking qios, losing per-qio parallelism
--

commit 899ab5e8861b55a2a4717a14d3e51773e55d3c87
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

drivers/md/dm-qcow2: process fast path qios in per-cpu batches on a workqueue

This patch replaces the per-qio work_struct on the fast path with a
per-CPU llist and one work item per CPU. A per-CPU busy flag limits
queue_work() calls, and the worker drains the whole llist each time it
runs.

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

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

[ ... ]

> +static void qcow2_fast_submit(struct qcow2_target *tgt, struct qio *qio)
> +{
> +	struct qcow2_fast_cpu *fc = get_cpu_ptr(tgt->fast);
> +
> +	llist_add(&qio->fast_link, &fc->qios);
> +	/* Pairs with smp_mb() in qcow2_fast_work() */
> +	smp_mb();
> +	if (!atomic_read(&fc->busy) && atomic_cmpxchg(&fc->busy, 0, 1) == 0)
> +		queue_work(tgt->wq, &fc->work);
> +	put_cpu_ptr(tgt->fast);
> +}

[Severity: Medium]
Does the busy flag reduce fast path concurrency compared to the old
per-qio work items?

Before this patch each qio had its own work_struct on an unbound
WQ_HIGHPRI workqueue, so a qio that slept delayed only itself. Now at
most one work item per submitting CPU is queued or running.

qcow2_fast_work() then runs every qio on that CPU's llist serially:

qcow2_fast_work()
  llist_for_each_entry_safe(qio, next, node, fast_link)
      do_qio_work(qio);

do_qio_work() can sleep. For QIO_STATE_START it goes through
prepare_one_embedded_qio() and then qio_try_fast():

do_qio_work()
  prepare_one_embedded_qio()
    create_bvec_from_rq()
      kvmalloc_array(nr_bvec, sizeof(struct bio_vec), GFP_NOIO)
  qio_try_fast()
    process_one_qio(qio->qcow2, qio, true)
      perform_rw_mapped()
        map_and_submit_rw()
          qcow2_call_rw_iter()
            file->f_op->write_iter() / read_iter()

qcow2_call_rw_iter() uses a sync kiocb (init_sync_kiocb). On a
buffered backing file, page cache, filesystem locking and writeback
throttling can block there. process_one_qio() with the fast flag set
only defers qios that need metadata changes, so plain mapped reads and
writes are issued inline.

If one qio sleeps in that path, every later qio on the same CPU's llist
waits behind it, and no other kworker is queued for that CPU. Could a
workload with a single submitting CPU and blocking backing-file I/O
become effectively single-threaded?

The commit message gives savings in context switches and pool->lock
time for 4 submitters on 12 CPUs. It gives no throughput or latency
numbers for the blocking case. Could such numbers be provided?

> +static void qcow2_fast_work(struct work_struct *work)
> +{

[ ... ]

> +	for (;;) {
> +		node = llist_del_all(&fc->qios);
> +		if (!node) {
> +			atomic_set(&fc->busy, 0);
> +			/* just to be sure we look at the list after busy is posted */
> +			smp_mb();
> +			if (llist_empty(&fc->qios))
> +				break;
> +			if (atomic_cmpxchg(&fc->busy, 0, 1) != 0)
> +				break;
> +			continue;
> +		}
> +		node = llist_reverse_order(node);
> +		llist_for_each_entry_safe(qio, next, node, fast_link)
> +			do_qio_work(qio);
> +	}

[Severity: Medium]
This is a minor point. The loop has no batch limit and no cond_resched()
between iterations. Under sustained submission on one CPU, can this keep
one WQ_HIGHPRI kworker busy and delay qcow2->worker and
qcow2->fsync_worker, which are queued on the same workqueue?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029140346.465768-1-andrey.zhadchenko@virtuozzo.com?part=7

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

Re: [Devel] [PATCH hci-8.1 7/7] drivers/md/dm-qcow2: process fast path qios in per-cpu batches on a workqueue

[Andrey Zhadchenko]

On 10/29/26 15:22, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] dm-qcow2: per-CPU fast-path worker serializes blocking qios, losing per-qio parallelism
> --
> 
> commit 899ab5e8861b55a2a4717a14d3e51773e55d3c87
> Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
> 
> drivers/md/dm-qcow2: process fast path qios in per-cpu batches on a workqueue
> 
> This patch replaces the per-qio work_struct on the fast path with a
> per-CPU llist and one work item per CPU. A per-CPU busy flag limits
> queue_work() calls, and the worker drains the whole llist each time it
> runs.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-95494
> 
>> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
>> --- a/drivers/md/dm-qcow2-map.c
>> +++ b/drivers/md/dm-qcow2-map.c
> 
> [ ... ]
> 
>> +static void qcow2_fast_submit(struct qcow2_target *tgt, struct qio *qio)
>> +{
>> +	struct qcow2_fast_cpu *fc = get_cpu_ptr(tgt->fast);
>> +
>> +	llist_add(&qio->fast_link, &fc->qios);
>> +	/* Pairs with smp_mb() in qcow2_fast_work() */
>> +	smp_mb();
>> +	if (!atomic_read(&fc->busy) && atomic_cmpxchg(&fc->busy, 0, 1) == 0)
>> +		queue_work(tgt->wq, &fc->work);
>> +	put_cpu_ptr(tgt->fast);
>> +}
> 
> [Severity: Medium]
> Does the busy flag reduce fast path concurrency compared to the old
> per-qio work items?
> 
> Before this patch each qio had its own work_struct on an unbound
> WQ_HIGHPRI workqueue, so a qio that slept delayed only itself. Now at
> most one work item per submitting CPU is queued or running.
> 
> qcow2_fast_work() then runs every qio on that CPU's llist serially:
> 
> qcow2_fast_work()
>    llist_for_each_entry_safe(qio, next, node, fast_link)
>        do_qio_work(qio);
> 
> do_qio_work() can sleep. For QIO_STATE_START it goes through
> prepare_one_embedded_qio() and then qio_try_fast():
> 
> do_qio_work()
>    prepare_one_embedded_qio()
>      create_bvec_from_rq()
>        kvmalloc_array(nr_bvec, sizeof(struct bio_vec), GFP_NOIO)
>    qio_try_fast()
>      process_one_qio(qio->qcow2, qio, true)
>        perform_rw_mapped()
>          map_and_submit_rw()
>            qcow2_call_rw_iter()
>              file->f_op->write_iter() / read_iter()
> 
> qcow2_call_rw_iter() uses a sync kiocb (init_sync_kiocb). On a
> buffered backing file, page cache, filesystem locking and writeback
> throttling can block there. process_one_qio() with the fast flag set
> only defers qios that need metadata changes, so plain mapped reads and
> writes are issued inline.

First of all, the backing file is not buffered. We explicitly require 
O_DIRECT.
Secondly we only submit IO here and this should be quite fast.
Also it is an still upgrade over single kwork.
In general this problem indeed exists but not in this configuration. If 
we add more stages like compressed read which can really sleep (for 
buffer allocation) this may become a problem.

> 
> If one qio sleeps in that path, every later qio on the same CPU's llist
> waits behind it, and no other kworker is queued for that CPU. Could a
> workload with a single submitting CPU and blocking backing-file I/O
> become effectively single-threaded?
> 
> The commit message gives savings in context switches and pool->lock
> time for 4 submitters on 12 CPUs. It gives no throughput or latency
> numbers for the blocking case. Could such numbers be provided?
> 
>> +static void qcow2_fast_work(struct work_struct *work)
>> +{
> 
> [ ... ]
> 
>> +	for (;;) {
>> +		node = llist_del_all(&fc->qios);
>> +		if (!node) {
>> +			atomic_set(&fc->busy, 0);
>> +			/* just to be sure we look at the list after busy is posted */
>> +			smp_mb();
>> +			if (llist_empty(&fc->qios))
>> +				break;
>> +			if (atomic_cmpxchg(&fc->busy, 0, 1) != 0)
>> +				break;
>> +			continue;
>> +		}
>> +		node = llist_reverse_order(node);
>> +		llist_for_each_entry_safe(qio, next, node, fast_link)
>> +			do_qio_work(qio);
>> +	}
> 
> [Severity: Medium]
> This is a minor point. The loop has no batch limit and no cond_resched()
> between iterations. Under sustained submission on one CPU, can this keep
> one WQ_HIGHPRI kworker busy and delay qcow2->worker and
> qcow2->fsync_worker, which are queued on the same workqueue?
> 

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

Re: [Devel] [PATCH hci-8.1 3/7] drivers/md/dm-qcow2: prepare process_one_qio() for fastpath

[Alexander Ivanov]

On 10/29/26 15:03, Andrey Zhadchenko wrote:
> dm-qcow2 heavily relies on main kwork do_qcow2_work() being
> sequential. All metadata change happens there and cannot
> run in parallel. Unfortunately that caps dm-qcow2 performance.
> But if the request does not require metadata changes we should
> be able to immediately dispatch it.
> process_one_qio() and handle_metadata() already know what to
> do: just add avoid_metadata_changes() flag and return codes.

Looks lake a typo, avoid_metadata_changes is not a function.

> While at it, just return in handle_metadata() instead of
> setting ret to 0 and jumping to if (ret < 0) label.
>
> Also exclude compressed clusters as there are some complications
> with wpc_readers.
>
> https://virtuozzo.atlassian.net/browse/VSTOR-95494
> Feature: dm-qcow2: block device over QCOW2 files driver
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
> ---
>   drivers/md/dm-qcow2-map.c | 35 +++++++++++++++++++++++++----------
>   1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index 1023224484f5b..c5d68b6b21f12 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -3534,7 +3534,7 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>   }
>   
>   static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
> -			   struct qcow2_map *map)
> +			   struct qcow2_map *map, bool avoid_metadata_changes)
>   {
>   	bool write = op_is_write((*qio)->bi_op);
>   	int ret;
> @@ -3543,15 +3543,20 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
>   	if (ret < 0 || !*qio) /* Error or postponed */
>   		goto check_err;
>   
> +	if (avoid_metadata_changes && map->compressed)
> +		return 2;
Looks like a magic constant. Maybe enum/define?
> +
>   	ret = 1;
>   	if (unlikely(qcow2->backward_merge_in_process)) {
> +		if (avoid_metadata_changes)
> +			return 2;
>   		/* Keep in mind the below may replace *qio */
>   		ret = prepare_backward_merge(qcow2, qio, map, write);
>   	} else if (unlikely(fake_l1cow_qio(*qio)) &&
>   		(!map->clu_is_cow || (map->level & L2_LEVEL))) {
>   		/* Nothing to COW or L1 is mapped exactly once */
>   		qio_endio(*qio);
> -		ret = 0;
> +		return 0;
>   	} else if (unlikely(op_is_discard((*qio)->bi_op))) {
>   		/*
>   		 * Discard carries no data and is advisory. If it does not
> @@ -3559,13 +3564,17 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
>   		 * is not present, end early.
>   		 */
>   		if (qio_discard_updates_metadata(qcow2, *qio, map)) {
> +			if (avoid_metadata_changes)
> +				return 2;
>   			ret = prepare_cluster_discard(qcow2, qio, map);
>   		} else if (!map->data_clu_alloced || map->clu_is_cow) {
>   			qio_endio(*qio);
> -			ret = 0;
> +			return 0;
>   		}
>   	} else if (write &&
>   		   (!qio_is_fully_alloced(qcow2, *qio, map) || map->clu_is_cow)) {
> +		if (avoid_metadata_changes)
> +			return 2;
>   		if (map->clu_is_cow) {
>   			/* COW to compressed or shared with snapshot cluster */
>   			ret = prepare_l1l2_replace(qcow2, *qio, map);
> @@ -3677,13 +3686,17 @@ static void process_read_qio(struct qcow2 *qcow2, struct qio *qio,
>   	qio_endio(qio);
>   }
>   
> -static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
> +static int process_one_qio(struct qcow2 *qcow2, struct qio *qio, bool avoid_metadata_changes)
>   {
>   	struct qcow2_map map = { .qcow2 = qcow2, };
>   	bool write;
> +	int ret;
>   
> -	if (!handle_metadata(qcow2, &qio, &map))
> -		return;
> +	ret = handle_metadata(qcow2, &qio, &map, avoid_metadata_changes);
> +	if (!ret)
> +		return 0;
> +	else if (ret == 2)
> +		return 1;
>   
>   	/*
>   	 * Merge machinery makes out of order merges for present
> @@ -3695,7 +3708,7 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
>   	if (unlikely(qcow2->backward_merge_in_process) &&
>   	    (fake_merge_qio(qio) || qcow2_file_is_writable(qcow2))) {
>   		submit_top_delta_read(&map, qio);
> -		return;
> +		return 0;
>   	}
>   
>   	write = op_is_write(qio->bi_op);
> @@ -3703,13 +3716,13 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
>   	/* Process discard early. Don't do COW just to discard */
>   	if (unlikely(op_is_discard(qio->bi_op))) {
>   		issue_discard(&map, qio);
> -		return;
> +		return 0;
>   	}
>   
>   	if (unlikely(map.compressed)) {
>   		/* Compressed qio never uses sub-clus */
>   		submit_read_compressed(&map, qio, write);
> -		return;
> +		return 0;
>   	}
>   
>   	if (!write) {
> @@ -3722,6 +3735,8 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
>   		else
>   			perform_rw_mapped(&map, qio);
>   	}
> +
> +	return 0;
>   }
>   
>   static struct bio_vec *create_bvec_from_rq(struct request *rq)
> @@ -3824,7 +3839,7 @@ static void process_deferred_qios(struct qcow2 *qcow2, struct list_head *qios)
>   			continue;
>   		}
>   
> -		process_one_qio(qcow2, qio);
> +		process_one_qio(qcow2, qio, false);
>   	}
>   }
>   
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 6/7] drivers/md/dm-qcow2: declare DM_TARGET_NOWAIT

[Alexander Ivanov]

On 10/29/26 15:03, Andrey Zhadchenko wrote:
> dm-qcow2 is a request-based target: REQ_NOWAIT handling is done by blk-mq
> (tag allocation) before a request reaches us, and clone_and_map_rq()
> never blocks. Still, the target does not declare DM_TARGET_NOWAIT, so
> dm_table_set_restrictions() clears BLK_FEAT_NOWAIT on the mapped device
> and the block device file gets no FMODE_NOWAIT.
>
> As a result io_uring is unable to issue I/O in nowait mode and punts
> *every* request to io-wq worker threads: on 4k randwrite (io_uring,
> qd=64, 4 jobs) io-wq threads burn ~24% of all cpus doing nothing but a
> thread handoff.
>
>                       before            after
>    qd=64 nj=4 IOPS    1034K (11% idle)  892K (30% idle)
>    qd=1  nj=1 lat     8us               6us
>
> The lower nj=4 number is because io-wq threads were providing "free"
> parallelism for the single-cpu submitters; the next patch gets it back
> without burning the cpus.
This commit drops multi-submitter throughput and the next patch brings
it back, which leaves a regression for bisect to land on. Maybe it has sense
to swap the order, so the batching patch goes first and this one follows?
>
> https://virtuozzo.atlassian.net/browse/VSTOR-95494
> Feature: dm-qcow2: block device over QCOW2 files driver
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
> ---
>   drivers/md/dm-qcow2-target.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> index be09615eb8e2e..4b9ae86ac9443 100644
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c
> @@ -1159,7 +1159,7 @@ static void qcow2_resume(struct dm_target *ti)
>   static struct target_type qcow2_target = {
>   	.name = "qcow2",
>   	.version = {1, 0, 0},
> -	.features = DM_TARGET_SINGLETON,
> +	.features = DM_TARGET_SINGLETON | DM_TARGET_NOWAIT,
>   	.module = THIS_MODULE,
>   	.ctr = qcow2_ctr,
>   	.dtr = qcow2_dtr,
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH 0/7] drivers/md/dm-qcow2: per-qio fast path

[Alexander Ivanov]

LGTM with minor notes.

Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

On 10/29/26 15:03, Andrey Zhadchenko wrote:
> dm-qcow2 processes every request consequentially in a single kwork.
> Unfortunately this introduces latency and cpu-bound limits. This
> patchset addresses the problem by laying per-qio tasks foundation:
> qios can be executed in parallel on the same workqueue. For now cover
> only requests with metadata fully allocated.
>
> https://virtuozzo.atlassian.net/browse/VSTOR-95494
>
> Andrey Zhadchenko (7):
>    drivers/md/dm-qcow2: repeat md page lookup if it was inserted in
>      parallel
>    drivers/md/dm-qcow2: check MD_DIRTY and md->wbd under lock in
>      perform_rw_mapped()
>    drivers/md/dm-qcow2: prepare process_one_qio() for fastpath
>    drivers/md/dm-qcow2: add per-qio work infrastructure
>    drivers/md/dm-qcow2: implement per-qio fast processing
>    drivers/md/dm-qcow2: declare DM_TARGET_NOWAIT
>    drivers/md/dm-qcow2: process fast path qios in per-cpu batches on a
>      workqueue
>
>   drivers/md/dm-qcow2-map.c    | 214 +++++++++++++++++++++++++++++------
>   drivers/md/dm-qcow2-target.c |  12 +-
>   drivers/md/dm-qcow2.h        |  10 ++
>   3 files changed, 197 insertions(+), 39 deletions(-)
>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 1/7] drivers/md/dm-qcow2: repeat md page lookup if it was inserted in parallel

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On Thu, 29 Oct 2026 17:03:40 +0300, Andrey Zhadchenko wrote:
> teach __handle_md_page() to work around -EEXIST by retrying.

Item 3 of the cover letter reply: say in the message what fails
without this patch, and add a Fixes tag for 412d5a4f7bf1b
("dm/dm-qcow2: add llseek_hole").

Why: the race is already there. qcow2_llseek_hole() runs
parse_metadata() in process context, in parallel with the main kwork.
When both miss the same md page, the one that loses the insert gets
-EEXIST: a guest I/O ends with EIO, or lseek returns -EEXIST. So this
is a fix on its own, not only a preparation for the fast path.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 2/7] drivers/md/dm-qcow2: check MD_DIRTY and md->wbd under lock in perform_rw_mapped()

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On Thu, 29 Oct 2026 17:03:41 +0300, Andrey Zhadchenko wrote:
> -	if (rw == WRITE && (md->status & MD_DIRTY) &&
> -	    test_bit(index_in_page, md->wbd->changed_indexes)) {
> +	if (rw == WRITE) {
>  		spin_lock_irqsave(&qcow2->md_pages_lock, flags);
> -		md->wbd->nr_submitted++;
> -		qio->data = md->wbd;
> +		if ((md->status & MD_DIRTY) &&
> +		    test_bit(index_in_page, md->wbd->changed_indexes)) {
> +			md->wbd->nr_submitted++;
> +			qio->data = md->wbd;
> +		}
>  		spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);

Item 1 of the cover letter reply: under this lock, when MD_WRITEBACK
is set and the index is in changed_indexes, park the qio on
md->wait_list, as parse_l2() does. Do not attach it to a wbd under
writeback: after a failed writeback complete_wbd() frees the wbd.

Why: the lock makes this check consistent, but parse_l2() decided
under an earlier lock hold that the write need not wait. With the fast
path of patch 5:

  main kwork                      fast worker
  allocates cluster X,
  L2 page DIRTY
                                  parse_l2(): write to X, page DIRTY,
                                  not delayed, unlock
  submit_metadata_writeback():
  DIRTY -> WRITEBACK
                                  perform_rw_mapped(): no MD_DIRTY,
                                  qio->data = NULL, submit

The write is acked as soon as its data is written. A FLUSH after it
finds QLIST_COMPLETED_WB empty and does only vfs_fsync(), it does not
wait for the L2 write in flight. A crash then loses a write that was
acked and flushed. If the L2 write fails instead, the entry is reverted
and X freed while the write already returned success. The base is safe
only because parse, submit and the DIRTY -> WRITEBACK switch happen in
one do_qcow2_work() pass.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH 0/7] drivers/md/dm-qcow2: per-qio fast path

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On Thu, 29 Oct 2026 17:03:39 +0300, Andrey Zhadchenko wrote:
> dm-qcow2 processes every request consequentially in a single kwork.
> Unfortunately this introduces latency and cpu-bound limits. This
> patchset addresses the problem by laying per-qio tasks foundation:
> qios can be executed in parallel on the same workqueue. For now cover
> only requests with metadata fully allocated.

What to change for v2:

1. Patches 2 and 5, code: a fast-path write can miss the wbd of the
   L2 page that maps it, and then it is acked before that L2 entry is
   on disk. Handle MD_WRITEBACK in perform_rw_mapped() the way
   parse_l2() does.

2. Patch 3, code: stop the fast path before the R1/R2 lookup in
   parse_metadata(), it never needs those pages.

3. Patch 1, commit message: say what fails today and add a Fixes tag
   for 412d5a4f7bf1b ("dm/dm-qcow2: add llseek_hole").

4. Patch 7, code and message: bound the drain loop, fix the sentence
   about re-queued qios, say that the fast path is now serial per
   submitting CPU.

5. Patches 6 and 7, commit messages: move the measured numbers into
   this thread.

6. Patch 4, code: update the QLIST_EMBEDDED comment.

7. Open remarks from the thread, please answer or fix them in v2:
   Sashiko on patch 1 (unbounded retry) and patch 5 (list_add order),
   Alexander on patch 3 (typo, magic 2) and patch 6 (swap 6 and 7).

8. Cover letter of v2:
   - "consequentially" becomes "sequentially";
   - not only fully allocated requests go fast: reads of unallocated
     or lower-delta clusters and partial discards do too, say that;
   - mention DM_TARGET_NOWAIT and the per-cpu batching.

Details on each item are in the replies to the patches. Alexander's
Reviewed-by should not be carried over to v2 as is, item 1 changes
the code he reviewed.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 3/7] drivers/md/dm-qcow2: prepare process_one_qio() for fastpath

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On Thu, 29 Oct 2026 17:03:42 +0300, Andrey Zhadchenko wrote:
>  static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
> -			   struct qcow2_map *map)
> +			   struct qcow2_map *map, bool avoid_metadata_changes)
>  {
>  	bool write = op_is_write((*qio)->bi_op);
>  	int ret;

Item 2 of the cover letter reply: pass the flag into parse_metadata()
and return to the slow path before qcow2_handle_r1r2_maps().

Why: for COW writes and cluster unmapping discards parse_metadata()
looks up the R1 and R2 pages before this function decides to go slow,
so the fast path reads them for nothing. The comment above
__handle_r1r2_maps() says R1 is unstable outside the main kwork. If the
R1 page is not cached, submit_read_md_page() drops the lock and inserts
it under the old page id; when relocate_refcount_table() has renumbered
the cached R1 pages meanwhile, a stale copy of the old R1 cluster stays
in the cache under an id whose cluster relocate then frees.

Alexander's remarks on the typo and on the literal 2 still need an
answer (item 7).

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 4/7] drivers/md/dm-qcow2: add per-qio work infrastructure

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On Thu, 29 Oct 2026 17:03:43 +0300, Andrey Zhadchenko wrote:
> -		list_add_tail(&qio->link, &qcow2->qios[qio->queue_list_id]);
> +		if (queue_list_id == QLIST_EMBEDDED)
> +			qio->state = QIO_STATE_START;
> +		else
> +			list_add_tail(&qio->link, &qcow2->qios[qio->queue_list_id]);

Item 6 of the cover letter reply: update the comment on QLIST_EMBEDDED
in dm-qcow2.h.

Why: embedded qios are no longer put on that list, it only serves as a
queue_list_id now, while the comment still calls it the list for the
initial setup of embedded qios.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 5/7] drivers/md/dm-qcow2: implement per-qio fast processing

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On Thu, 29 Oct 2026 17:03:44 +0300, Andrey Zhadchenko wrote:
> +static void qio_try_fast(struct qio *qio, struct list_head *deferred_qios)
> +{
> +	list_del(&qio->link);
> +
> +	if (process_one_qio(qio->qcow2, qio, true))
> +		list_add(&qio->link, deferred_qios);
> +}

Items 1 and 7 of the cover letter reply.

This patch is where the race of item 1 becomes live: from here on
process_one_qio() runs in parallel with do_qcow2_work(), and a write
can reach perform_rw_mapped() after the main kwork switched its L2 page
to writeback. The details are in the reply to patch 2.

Sashiko's list_add() remark holds, please use list_add_tail() or say
why the reversed order is fine.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 7/7] drivers/md/dm-qcow2: process fast path qios in per-cpu batches on a workqueue

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On Thu, 29 Oct 2026 17:03:46 +0300, Andrey Zhadchenko wrote:
> context switch) even if a running worker would pick the work up right
> away. PENDING is cleared when execution starts, so a qio re-queued
> while its work runs wakes yet another worker just to find the collision
> and go back to sleep. On a 12 cpu box with 4 submitters this shows up
> as ~3 context switches per I/O, 5% of all cycles spinning on
> pool->lock and 6% in the scheduler.

Items 4 and 5 of the cover letter reply:
- fix the re-queue sentence;
- move the measurements into this thread;
- add a paragraph that the fast path is now serial per submitting
  CPU, and why that is acceptable.

Why: after patch 4 every qio work is queued exactly once, so no qio is
re-queued while it runs. The work that is re-queued while running is
qcow2->worker, and this patch does not touch it.

On your answer to Sashiko: O_DIRECT is enforced for every delta, that
holds. "We only submit IO" does not hold for all fast-path qios: a
partial discard runs qcow2_punch_hole() right in this worker, which
takes the inode lock and waits for all direct I/O in flight on the
image. Everything queued on that CPU waits behind it. Either send
partial discards to the slow path or say it in the message.

> +	for (;;) {
> +		node = llist_del_all(&fc->qios);
> +		if (!node) {

Item 4 of the cover letter reply: bound the loop, or add
cond_resched() between batches.

Why: while one CPU keeps refilling its llist this work item never
returns. flush_workqueue() in qcow2_flush_deferred_activity(), which
the backward merge completion calls with I/O still running, waits for
it.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 6/7] drivers/md/dm-qcow2: declare DM_TARGET_NOWAIT

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On Thu, 29 Oct 2026 17:03:45 +0300, Andrey Zhadchenko wrote:
>                      before            after
>   qd=64 nj=4 IOPS    1034K (11% idle)  892K (30% idle)
>   qd=1  nj=1 lat     8us               6us

Item 5 of the cover letter reply: drop the table and the "~24% of all
cpus" from the message and post them in this thread. Say instead what
gets faster: io_uring no longer punts every request to io-wq.

Why: the numbers hold only on the box and the job they were measured
with, a later reader of the commit cannot act on them.

The code is correct. Alexander's request to put this patch after the
batching one still needs an answer (item 7).

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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