Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] dm-qcow2: unbounded goto again in __handle_md_page() on injected -EEXIST --
commit 949be3603b3c812058ed084d1aae429a3ec4e585 Author: Andrey Zhadchenko <[email protected]> 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. The -EEXIST means another context, such as qcow2_llseek_hole() running in parallel with the kwork, inserted the same md page first. The retry lets the qio be served from, or postponed on, that page. > 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 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() spin forever? The goto again assumes that -EEXIST from submit_read_md_page() always means a page with this id is now in the rb-tree, so the next qcow2_md_page_find_or_postpone() will find it. That holds for the genuine source of -EEXIST, md_page_try_insert() in dm-qcow2-target.c, which returns it only when the id is already in the tree. However, qcow2_alloc_and_insert_md_page() is annotated: ALLOW_ERROR_INJECTION(qcow2_alloc_and_insert_md_page, ERRNO); so fail_function can make it return -EEXIST while no page exists. In that case: __handle_md_page() qcow2_md_page_find_or_postpone() -> NULL submit_read_md_page() -> -EEXIST (injected) goto again repeats with no retry cap and no cond_resched(). Before this patch, the injected -EEXIST was returned as an ordinary error. All callers of __handle_md_page() that pass a qio would be affected, so the kwork or the llseek caller in process context could spin or trigger a soft lockup. Would a bounded retry, or a cond_resched() in the loop, be appropriate? > > *ret_md = md; > return 1; -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=1 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
