Re: [Devel] [PATCH hci-8.1 5/7] drivers/md/dm-qcow2: implement per-qio fast processing

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-qcow2: qio_try_fast() uses list_add() and reverses the order of deferred sibling splits
--

commit 3954eb13b0c2923af1fc503136a833ede7213254
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

drivers/md/dm-qcow2: implement per-qio fast processing

This commit makes do_qio_work() split a qio and then try to process each
piece right away with process_one_qio(..., true). Pieces that still need
metadata work are put on a local list and spliced onto
qcow2->qios[QLIST_DEFERRED] for do_qcow2_work().

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 qio_try_fast(struct qio *qio, struct list_head *deferred_qios)
> +{
> +	list_del(&qio->link);
> +
> +	if (process_one_qio(qio->qcow2, qio, true))
> +		list_add(&qio->link, deferred_qios);

[Severity: Low]
Can this list_add() reverse the order of the deferred pieces?

qcow2_split_qio_to_list() builds the split list in ascending order, with
the parent last:

    list_splice_tail(&list, ret_list);
    list_add_tail(&qio->link, ret_list);

do_qio_work() walks that list front to back and calls qio_try_fast() on
each entry. Since list_add() inserts at the head of deferred_qios, each
piece that returns 1 from process_one_qio() lands in front of the earlier
ones. deferred_qios therefore ends up in descending order.

That list is then spliced to qcow2->qios[QLIST_DEFERRED] with
list_splice_tail(), so the slow-path pieces of one request are queued in
reverse order. Before this patch, prepare_one_embedded_qio() filled the
deferred list directly, and the order was preserved.

I did not find a correctness problem. Each piece covers a different
cluster, and process_deferred_qios() does not depend on order. The visible
effect is that cluster and metadata allocation and submission happen in
reverse order for large writes that need allocation.

Would list_add_tail() be more appropriate here to keep the original order?

> +}
> +
>  static void do_qio_work(struct work_struct *ws)
>  {

[ ... ]

> +		prepare_one_embedded_qio(qio, &split_qios);
> +		list_for_each_entry_safe(i, next, &split_qios, link)
> +			qio_try_fast(i, &deferred_qios);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029140346.465768-1-andrey.zhadchenko@virtuozzo.com?part=5

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel