From 9c7f2d788fd2acf354d3c7559c1d90e503fbf542 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 31 Dec 2025 12:43:48 -0600 Subject: [PATCH] Fixed weakened wear-leveling (mdir aliasing) due to rev changes This is a common pitfall with mdirs that has only ended up in the codebase ~3(?) times. Naively using a power-of-two recycle counter for relocations ends up aliasing mdir blocks such that only one block is actually wear-leveled. I'm not entirely sure it was intentional, but the previous double-increment during needsrelocation checks made mdirs relocate one recycle early, avoiding this aliasing issue. However, the double-increment had other issues. The most glaring is that it would always trigger two relocations back-to-back due to the mismatch between counter cycles and overflow checks. Sort of defeating the purpose of wear-leveling the two mdir blocks separately... --- What we really want is a counter that's always coprime with 2. Such as our old friend mod (2^n)-1. Unfortunately mod (2^n)-1 counters don't really have any great optimization trick. They show up all the time when code relies on the multiplicative cycle of a 2^n finite-field, but despite this, all of the implementations I've seen rely on a simple branch to handle the one extra state. I'm not sure this is the best implementation, but adding 2 and subtracting 1 on non-overflow seems to minimize resulting code cost. Presumably because the compiler is able to deduplicate this with the needsrelocation check. Though we're only talking about a handful of bytes. Code changes: code stack ctx before: 35152 2136 660 after: 35164 (+0.0%) 2136 (+0.0%) 660 (+0.0%) code stack ctx gbmap before: 38388 2144 776 gbmap after: 38400 (+0.0%) 2144 (+0.0%) 776 (+0.0%) code stack ctx preerase before: 38928 2168 796 preerase after: 38940 (+0.0%) 2168 (+0.0%) 796 (+0.0%) --- We don't have great tests for this, as it's difficult to create rigorous checks for our best-effort dynamic wear-leveling. But, surprisingly enough, this _was_ caught by test_exhaustion_spam_uzd_fuzz's doubling-disk-doubles-lifetime check! Though only with LFS3_YES_GBMAP=1: LFS3_YES_GBMAP=1 \ TESTS=tests/test_exhaustion.toml \ make test-runner -j \ && ./scripts/test.py test_exhaustion_spam_uzd_fuzz -O- -j \ | grep lifetime --- lfs3.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/lfs3.c b/lfs3.c index 8c45addb..54e92566 100644 --- a/lfs3.c +++ b/lfs3.c @@ -7798,7 +7798,10 @@ static inline bool lfs3_rev_needsrelocation(lfs3_t *lfs3, uint32_t rev) { } // does out recycle counter overflow? - uint32_t rev_ = rev + (1 << (28-lfs3_smax(lfs3->recycle_bits, 0))); + uint32_t rev_ = rev + (2 << (28-lfs3_smax(lfs3->recycle_bits, 0))); + // ^ note the +2! this ensures we overflow after + // an odd number of recycles, otherwise we'd + // only ever relocate one block in mdirs return (rev_ >> 28) != (rev >> 28); } #endif @@ -7806,7 +7809,15 @@ static inline bool lfs3_rev_needsrelocation(lfs3_t *lfs3, uint32_t rev) { #ifndef LFS3_RDONLY static inline uint32_t lfs3_rev_inc(lfs3_t *lfs3, uint32_t rev) { // increment recycle counter/revision - return rev + (1 << (28-lfs3_smax(lfs3->recycle_bits, 0))); + // + // this mess increments by 2 unless we _don't_ overflow, in effect + // it implements a mod (2^n)-1 counter, the goal is to avoid + // multiple needsrelocation triggers (see above) + uint32_t rev_ = rev + (2 << (28-lfs3_smax(lfs3->recycle_bits, 0))); + if ((rev_ >> 28) == (rev >> 28)) { + rev_ -= 1 << (28-lfs3_smax(lfs3->recycle_bits, 0)); + } + return rev_; } #endif @@ -15174,18 +15185,17 @@ static int lfs3_init(lfs3_t *lfs3, uint32_t flags, // find the number of bits to use for recycle counters // - // Add 1, to include the initial erase, multiply by 2, since we - // alternate which metadata block we erase each compaction, and limit - // to 28-bits so we always have some bits to determine the most recent - // revision. + // Add 1 for the initial erase, and multiply by 2 since we alternate + // which metadata block we erase each compaction. + // + // We're currently limited to 20-bits to keep space for + // perturb/low-effort debug bits, though this could be relaxed + // in the future (though >28 bits may cause problems since we need + // some bits to tell revisions apart). + // #ifndef LFS3_RDONLY if (lfs3->cfg->block_recycles != -1) { - lfs3->recycle_bits = lfs3_min( - lfs3_nlog2(2*(lfs3->cfg->block_recycles+1)+1)-1, - 28); - // we're currently limited to 20-bits to keep space for - // perturb/low-effort debug bits, though this could be relaxed - // in the future + lfs3->recycle_bits = lfs3_nlog2(2*(lfs3->cfg->block_recycles+1)+1)-1; LFS3_ASSERT(lfs3->recycle_bits <= 20); } else { lfs3->recycle_bits = -1;