It is true but so far this is more like a premature optimization of low
likelihood scenario that increases md memory footprint. In the worst
case it is the guest that actively slows itself with parallel writes and
discards. So no for now. Maybe we can consider this later when we add
per-md locking
On 11/12/26 12:26, [email protected] wrote:
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?
[ ... ]
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
