Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] dm-qcow2: concurrent qio preparers can deadlock on qio_pool under memory pressure --
commit 1b2b6ed6f3d2b43e71cc7424d0502aa9998c0d28 Author: Andrey Zhadchenko <[email protected]> dm-qcow2: add per-qio work infrastructure This commit adds a work_struct and a state field to struct qio. It uses them to queue the first pipeline step, qio preparation, as one work item per data request on tgt->wq. Previously this step ran serialised inside the single qcow2->worker. 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 [ ... ] > @@ -4519,7 +4511,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 { [ ... ] > spin_unlock_irqrestore(&qcow2->deferred_lock, flags); > > if (queue) > queue_work(tgt->wq, worker); [Severity: Medium] Can concurrent preparers deadlock on qio_pool under memory pressure? Before this patch, preparation ran serialised inside qcow2->worker, so only one preparer held a partial split list at a time. Now one work item is queued per data request on the unbound tgt->wq, which has the default max_active limit of 256. Each work item runs: do_qio_work() prepare_one_embedded_qio() qcow2_split_qio_to_list() split_and_chain_qio() qcow2_alloc_qio(qcow2->tgt->qio_pool, true) mempool_alloc(GFP_NOIO) The split qios stay on a local list and are only handed to QLIST_DEFERRED in do_qio_work() after the whole request has been split: > + prepare_one_embedded_qio(qio, &deferred_qios); > + if (list_empty(&deferred_qios)) > + break; The pool is created in alloc_qcow2_target() as: tgt->qio_pool = mempool_create_kmalloc_pool(MIN_QIOS, sizeof(struct qio)); with 512 elements, shared by all preparers and the rest of the pipeline. Up to 512 requests may be in flight (qrq_pool), and each request needs up to request_size/clu_size splits (about 20 for a 64K cluster and a 1280K request, and more for smaller clusters). Total demand can therefore exceed 512. If kmalloc keeps failing under memory pressure, can several preparers each hold part of the reserve while all of them sleep in mempool_alloc() waiting for more? The only wake-up source is a free of a qio from a completed request, and none of the blocked preparers' own splits can complete until splitting finishes. The later patches in the series do not appear to remove this. The final state (d9084b95) drops the per-qio work_struct and runs do_qio_work() from per-CPU qcow2_fast_work items on the same tgt->wq. That lowers the number of preparers from up to 256 to one per CPU, but each preparer still builds its full split list with blocking mempool_alloc() before releasing any of it, so the hold-and-wait remains possible, mainly on many-core machines or with small clusters. Would it make sense to throttle or reserve pool space for preparers? -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=4 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
