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

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