Thank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [Low] dm-qcow2: message and comment misdescribe -EIO on second lookup miss --
commit ac0961cf11bb91c58bc95e6d03ca39499fd6547e Author: Andrey Zhadchenko <[email protected]> dm-qcow2: repeat md page lookup if it was inserted in parallel This teaches __handle_md_page() to repeat the md page lookup once when submit_read_md_page() returns -EEXIST. -EEXIST means qcow2_llseek_hole() and the main kwork raced to insert the same md page. > diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c > index 32373114580e..58b95d7a955c 100644 > --- a/drivers/md/dm-qcow2-map.c > +++ b/drivers/md/dm-qcow2-map.c > @@ -1676,20 +1676,33 @@ 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. > + * > + * submit_read_md_page() drops md_pages_lock, so the page may be > + * inserted in parallel. Then it is looked up once more. It is not > + * found again only if its read failed in the meantime, and then > + * @qio fails too. > */ > static int __handle_md_page(struct qcow2 *qcow2, u64 page_id, > struct qio **qio, struct md_page **ret_md) > { > + bool retried = false; > 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)) { > + if (retried) > + return -EIO; > + retried = true; > + goto again; > + } > + return ret; > + } > return 0; > } > [Severity: Low] This isn't a bug, but does the code match what the comment and commit message say about a second lookup miss? They say the second lookup misses only if the read failed and erased the page, and that the qio then fails with -EIO. The code does something else. After the retry lookup misses with *qio still set, it calls submit_read_md_page() again. That call allocates and inserts a fresh md page, queues the qio on it and issues a new read. So the qio is retried, not failed. -EIO is returned only when the second submit_read_md_page() also returns -EEXIST, which means two insert races in a row. Could the comment and commit message describe that instead? Alternatively, the retry path could fail the qio directly when the second lookup misses. -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=1 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
