Re: [Devel] [PATCH hci-8.1 1/7] drivers/md/dm-qcow2: repeat md page lookup if it was inserted in parallel

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-qcow2: __handle_md_page() retries on -EEXIST with no bound or progress check
--

commit 3961a8a0138999bca60a809cb24928991567bdd8
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

drivers/md/dm-qcow2: repeat md page lookup if it was inserted in parallel

This patch makes __handle_md_page() retry the md page lookup when submit_read_md_page() returns -EEXIST. It also removes the comment telling callers to handle -EEXIST themselves.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-95494

> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index 78ee7cf5e8817..fed6ce75e98ad 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c

[ ... ]

> @@ -1596,20 +1596,23 @@ static int submit_read_md_page(struct qcow2 *qcow2, struct qio **qio,
>  /*
>   * This may be called with @qio == NULL, in case of we are
>   * interesting in searching cached in memory md only.
> - * This is aimed to be called not only from main kwork
> - * for L1/L2 pages, so all callers looking for L1/L2
> - * must care about submit_read_md_page() may return EEXIST.
>   */
>  static int __handle_md_page(struct qcow2 *qcow2, u64 page_id,
>  			    struct qio **qio, struct md_page **ret_md)
>  {
>  	struct md_page *md;
> +	int ret;
>  
>  	lockdep_assert_held(&qcow2->md_pages_lock);
> +again:
>  	md = qcow2_md_page_find_or_postpone(qcow2, page_id, qio);
>  	if (!md) {
> -		if (qio && *qio)
> -			return submit_read_md_page(qcow2, qio, page_id);
> +		if (qio && *qio) {
> +			ret = submit_read_md_page(qcow2, qio, page_id);
> +			if (unlikely(ret == -EEXIST))
> +				goto again;
> +			return ret;
> +		}
>  		return 0;
>  	}

[Severity: Low]
Can this retry loop in __handle_md_page() run forever? The goto again
is taken on every -EEXIST from submit_read_md_page(), with no retry limit,
no cond_resched(), and no check that the page has actually become visible
to the lookup.

Organically, -EEXIST comes from md_page_try_insert() under md_pages_lock.
The retry's qcow2_md_page_find_or_postpone() then takes the same lock and
finds the existing node, so the loop should end after one extra pass.

However, two functions in dm-qcow2-target.c are error-injectable:

    ALLOW_ERROR_INJECTION(qcow2_alloc_and_insert_md_page, ERRNO);
    ALLOW_ERROR_INJECTION(qcow2_md_page_find_or_postpone, NULL);

If an injected -EEXIST is returned from qcow2_alloc_and_insert_md_page(),
nothing has been inserted, so each retry misses the lookup and gets
-EEXIST again. If NULL is injected into qcow2_md_page_find_or_postpone(),
an existing page looks missing and md_page_try_insert() fails again on
every pass.

In both cases the calling kwork or submitting thread would spin in
__handle_md_page(). Each pass takes and drops md_pages_lock, does a
kmalloc(GFP_KERNEL) and an alloc_page(GFP_KERNEL), then frees both.

This needs privileged fault injection (fail_function or BPF override), so
there is no organic trigger. Before this patch, an injected -EEXIST was
returned to the caller as an error. After it, the same injection becomes a
hang.

Would a bounded retry, or a check that the page is now present before
looping, be more appropriate here?

The comment removed from above __handle_md_page() also described
-EEXIST handling for callers. Is it still accurate that no caller needs to
handle -EEXIST from this function?

The final state of the series leaves __handle_md_page() and this retry
unchanged, so nothing later in the series addresses this.

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

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