Thank you for your contribution! Sashiko AI review found 2 potential issues to consider: - [Medium] dm-qcow2: concurrent qio preparers hold partial splits while sleeping in mempool_alloc() - [Low] dm-qcow2: redundant qio->state init in qcow2_submit_embedded_qio() --
commit f0e3abee17282a35ee80199fc8894df280f02698 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 qio preparation (prepare_one_embedded_qio()) as a per-request work item. Previously this ran inside the main qcow2->worker. > diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c > index b9daa258285e..496fbcc61348 100644 > --- a/drivers/md/dm-qcow2-map.c > +++ b/drivers/md/dm-qcow2-map.c > @@ -4710,7 +4702,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 { [Severity: Medium] As reported by [email protected] on v2 [1], can concurrent preparers deadlock on qio_pool under memory pressure? Every data request now gets its own work item on tgt->wq, which is WQ_UNBOUND, so many of them can run at once. Each one runs do_qio_work() -> prepare_one_embedded_qio() -> qcow2_split_qio_to_list() -> split_and_chain_qio(), which calls qcow2_alloc_qio() on the shared 512-element qio_pool. That is a blocking mempool_alloc(GFP_NOIO). The splits are kept on a local list and are released only after the whole request has been split. Several preparers can each hold part of the reserve while sleeping in mempool_alloc() for more. None of them can make progress, and the device hangs. Before this patch only one preparer ran at a time, inside qcow2->worker. Would a non-blocking first pass, with a serialized sleeping fallback as dm-crypt does, avoid this? > @@ -4726,7 +4718,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); [Severity: Low] This isn't a bug, but is this qio->state assignment needed? qcow2_init_qio() already sets qio->state = QIO_STATE_START before the qio is submitted, and QIO_STATE_START is the only state. Dropping it would also remove the if/else inside the deferred_lock section, leaving just the list_add_tail() for the non-embedded case. [1] https://lore.kernel.org/r/010701a1c556541f-a3b013d6-70e1-45b6-bfc1-37c018cdf88f-000...@eu-central-1.amazonses.com -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=4 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
