From 50d012b2adf2e097a22561a06b48076e11cace4b Mon Sep 17 00:00:00 2001 From: Rob Norris Date: Fri, 26 Jun 2026 04:42:56 +1000 Subject: [PATCH] zbookmark_compare: handle "marker" bookmarks with negative levels "Marker" bookmarks (those with zb_level == ZB_ROOT_LEVEL, ZB_ZIL_LEVEL or ZB_DNODE_LEVEL) represent valid blocks, but are associated with a dataset directly rather than with a specific object within it. They end up on bookmark lists during scan prefetch, and so need to be sorted ahead of any "true" object blocks. The problem is that for negative levels, BP_SPANB produces a negative shift, which is not legal C. Fortunately the results are used only for comparison, so the worst possible behaviour in a forgiving compilation environment is a mis-sort, which for the scan/traverse cases, means that we haven't prefetched certain metadata before we actually need it. But there _is_ UB in there, and UBSAN does rightly complain. Here we fix all this by handling these bookmarks directly - sorting them ahead of "true" object blocks, which is usually what scan/traverse will prefer. And we don't do any interesting math on these bookmarks, so we sidestep the whole UB thing. Sponsored-by: TrueNAS Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Rob Norris Closes #14777 Closes #18652 --- module/zfs/zio.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/module/zfs/zio.c b/module/zfs/zio.c index 4b7c13dd1e94..a45221d6544e 100644 --- a/module/zfs/zio.c +++ b/module/zfs/zio.c @@ -6002,6 +6002,67 @@ zbookmark_compare(uint16_t dbss1, uint8_t ibs1, uint16_t dbss2, uint8_t ibs2, zb1->zb_blkid == zb2->zb_blkid) return (0); + if (zb1->zb_level < 0 || zb2->zb_level < 0) { + /* + * "Negative" levels are ZB_ROOT_LEVEL, ZB_ZIL_LEVEL or + * ZB_DNODE_LEVEL, and represent some sort of auxiliary dataset + * block or object. In this case, we're usually being called + * from dsl_scan or dmu_traverse. + * + * These "levels" are more like a "type" signal, not directly + * comparable, but we have to do something. So we order them in + * the order we would see them during a typical scan or + * traverse: + * + * - ZB_ROOT_LEVEL: the "top" block carrying the dataset head + * - ZB_ZIL_LEVEL: the head ZIL block attached to the dataset + * - ZB_DNODE_LEVEL: "virtual" position representing an + * entire object. Sorts ahead of the true + * data blocks for the object. + * - level >= 0: data blocks + * + * We work through these cases from top to bottom, with + * appropriate tiebreaks for each kind. + */ + + /* + * Root level wins. It shouldn't be possible for both to be the + * root level in this per-dataset tree, and there's no obvious + * tiebreaker, but we handle it as a defensive measure. + */ + if (zb1->zb_level == ZB_ROOT_LEVEL && + zb2->zb_level == ZB_ROOT_LEVEL) + return (TREE_PCMP(zb1, zb2)); + if (zb1->zb_level == ZB_ROOT_LEVEL) + return (-1); + if (zb2->zb_level == ZB_ROOT_LEVEL) + return (1); + + /* ZIL bookmarks have valid blkid, so the earlier one wins. */ + if (zb1->zb_level == ZB_ZIL_LEVEL && + zb2->zb_level == ZB_ZIL_LEVEL) + return (TREE_CMP(zb1->zb_blkid, zb2->zb_blkid)); + if (zb1->zb_level == ZB_ZIL_LEVEL) + return (-1); + if (zb2->zb_level == ZB_ZIL_LEVEL) + return (1); + + /* + * If we get this far, then at least one is ZB_DNODE_LEVEL, and + * the other is either ZB_DNODE_LEVEL or a data block. + * Regardless, the one with the lower-numbered object wins - + * earler ZB_DNODE_LEVEL beats later, but data block on earlier + * objects beats the virtual marker on later objects. + */ + int cmp = TREE_CMP(zb1->zb_object, zb2->zb_object); + if (cmp != 0) + return (cmp); + + if (zb1->zb_level == ZB_DNODE_LEVEL) + return (-1); + return (1); + } + IMPLY(zb1->zb_level > 0, ibs1 >= SPA_MINBLOCKSHIFT); IMPLY(zb2->zb_level > 0, ibs2 >= SPA_MINBLOCKSHIFT);