Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] dm-qcow2: dec_writers() scans whole qcow2-wide discard_wait_list under irq-off lock --
commit 8c3ab12e988e2b5f6106aa1b1403344b45fba8ab Author: Andrey Zhadchenko <[email protected]> dm-qcow2: wait for in-flight writes before freeing a discarded cluster This patch makes a discard that unmaps a non-shared cluster wait in process_indexes_end() until the data writes counted in its L2 page have completed. The discard switches the writer generation of the page and waits for the previous generation to drain. A per-qcow2 discard_wait_list holds the delayed discards, and dec_writers() wakes them. > 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 discard_waiters_to_list(struct qcow2 *qcow2, struct md_page *md, > + struct list_head *list) > +{ > + struct qio *discard, *tmp; > + > + lockdep_assert_held(&qcow2->md_pages_lock); > + list_for_each_entry_safe(discard, tmp, &qcow2->discard_wait_list, link) > { > + if (discard->ext->lx_md == md) > + list_move_tail(&discard->link, list); > + } > +} > + > static void dec_writers(struct qio *qio) > { > struct md_page *md = qio->writer_md; > + struct qcow2 *qcow2 = qio->qcow2; > + u8 gen = qio->writer_gen; > + LIST_HEAD(wait_list); > + unsigned long flags; > > qio->writer_md = NULL; > - atomic_dec(&md->active_writers[qio->writer_gen]); > + /* > + * Fully ordered RMW pairs with smp_mb() in delay_discard_if_writers(): > + * either we see switched generation, or the discard sees our put. > + * The generation can't be switched back to ours while a discard > + * waits for it, so the same generation means nobody is to wake. > + */ > + if (atomic_dec_return(&md->active_writers[gen]) || > + READ_ONCE(md->writer_gen) == gen) > + return; > + > + spin_lock_irqsave(&qcow2->md_pages_lock, flags); > + discard_waiters_to_list(qcow2, md, &wait_list); > + spin_unlock_irqrestore(&qcow2->md_pages_lock, flags); > + qcow2_dispatch_qios(qcow2, NULL, &wait_list); > +} [Severity: Low] Is it intended that dec_writers() scans the whole per-qcow2 discard_wait_list with irqs disabled? When a discard switches md->writer_gen in delay_discard_if_writers() and the last writer of the old generation completes, dec_writers() takes the slow path: dec_writers() spin_lock_irqsave(&qcow2->md_pages_lock, flags); discard_waiters_to_list(qcow2, md, &wait_list); discard_waiters_to_list() walks the single qcow2-wide list and compares discard->ext->lx_md with md, so the cost is proportional to all pending discards on the qcow2 rather than to those on this md page. It also moves every discard for that md, including ones that were only delayed (no QIO_DISCARD_WAIT_FL set). Those re-run delay_discard_if_writers() and may be delayed again, each costing an extra qcow2_dispatch_qios() and worker pass. This looks like a minor inefficiency rather than a correctness problem, and the list is bounded by the number of in-flight discards. As far as I can tell the later patch in the series ("dm-qcow2: let discards of one L2 page wait for in-flight writes together") leaves dec_writers() unchanged, so the wake is not narrowed there either. Would a per-md list, or waking only the discard that holds QIO_DISCARD_WAIT_FL, be worth considering? [ ... ] -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=2 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
