Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] dm-qcow2: fast path uses unpinned L2 mapping between parse_metadata() and data submit --
commit a3dc5e64e9aa8f73ed35df62194a86cf78276045 Author: Andrey Zhadchenko <[email protected]> dm-qcow2: prepare process_one_qio() for fastpath This patch adds an avoid_metadata_changes flag to process_one_qio(), handle_metadata() and parse_metadata(). With the flag set, a qio that needs metadata changes, R1/R2 lookups or a compressed cluster is handed back to the main kwork through the new QIO_NEEDS_MAIN_KWORK return code. All callers in this patch pass false. 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 [ ... ] > @@ -2075,6 +2084,9 @@ static int parse_metadata(struct qcow2 *qcow2, struct > qio **qio, > if (!qio_may_modify_image(qcow2, *qio)) > return 0; > > + if (avoid_metadata_changes) > + return QIO_NEEDS_MAIN_KWORK; > + > /* Now refcounters table/block */ > ret = qcow2_handle_r1r2_maps(qcow2, pos, qio, &map->r1, > &map->r2, map->compressed); [Severity: Medium] In this patch alone the flag is only ever passed as false, so this path is not reachable yet. It becomes live with the later patches that add per-qio fast processing and per-cpu fast workers. Those call process_one_qio(..., true) from outside the main kwork. Is the L2 mapping stable between parse_metadata() and the data submit in that case? parse_metadata() drops md_pages_lock at its unlock label before this point. A qio that continues on the fast path then carries map->data_clu_pos and map->l2.md with no lock, pin or wpc_readers reference: process_one_qio() handle_metadata() parse_metadata() /* md_pages_lock dropped at unlock */ process_read_qio() or perform_rw_mapped() map_and_submit_rw(qcow2, map->data_clu_pos, qio) Meanwhile the main kwork can run the discard path: prepare_cluster_discard() prepare_l_entry_replace() issue_discard() qcow2_punch_hole(qcow2->file, pos, qio->bi_iter.bi_size) issue_discard() punches the hole immediately. It does not wait for fast path qios that already parsed the same L2 entry. Can a fast-path read or write then go to a cluster that a concurrent discard has already unmapped and freed, and which may have been reallocated to another guest offset? That would return stale or foreign data on a read, or write into a reused cluster. As far as I can see, wpc_readers is taken only for compressed and shared clusters in submit_read_whole_cow_clu() and submit_read_compressed(), and this patch excludes compressed clusters from the fast path. The other mechanisms only affect qios that parse later: delay_if_wpc_readers_locked() and md_index_set_locked() do not cover a qio that has already parsed. perform_rw_mapped() only revalidates the DIRTY and WRITEBACK state. Fully allocated, non-COW, non-compressed clusters were already untracked before this patch, so the new exposure is the added parse-to-submit window in the per-cpu worker. That window can stretch if the worker is preempted. The commit message says "if the request does not require metadata changes we should be able to immediately dispatch it." What keeps the L2 entry and data cluster stable for such a qio until its data I/O is submitted? [ ... ] > -static void process_one_qio(struct qcow2 *qcow2, struct qio *qio) > +static int process_one_qio(struct qcow2 *qcow2, struct qio *qio, bool > avoid_metadata_changes) > { > struct qcow2_map map = { .qcow2 = qcow2, }; > bool write; > + int ret; > > - if (!handle_metadata(qcow2, &qio, &map)) > - return; > + ret = handle_metadata(qcow2, &qio, &map, avoid_metadata_changes); > + if (!ret) > + return 0; > + else if (ret == QIO_NEEDS_MAIN_KWORK) > + return 1; [Severity: Medium] This is the other side of the same concern. When handle_metadata() returns 1 with avoid_metadata_changes set, the code below goes on to process_read_qio() or perform_rw_mapped() without any protection against a concurrent discard on the main kwork, as described above. Is there something that closes this window? -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=3 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
