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@sashiko.virtuozzo.com]

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