From 5fa85583cdf389ab612823829954546513e47670 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 24 Apr 2024 01:37:41 -0500 Subject: [PATCH] Dropped block-level erased-state checksums for RAM-tracked erased-state Unfortunately block-level erased-state checksums (becksums) don't really work as intended. An invalid becksum _does_ signal that a prog has been attempted, but a valid becksum does _not_ prove that a prog has _not_ been attempted. Rbyd ecksums work, but only thanks to a combination of prioritizing valid commits and the use of perturb bits to force erased-state changes. It _is_ possible to end up with an ecksum collision, but only if you 1. lose power before completing a commit, and 2. end up with a non-trivial crc32c collision. If this does happen, at the very least the resulting commit will likely end up corrupted and thrown away later. Block-level becksums, at least as originally designed, don't have either of these protections. To make matters worse, the blocks these becksums reference contain only raw user data. Write 0xffs into a file and you will likely end up with a becksum collision! This is a problem for a couple of reasons: 1. Progging multiple times to erased-state is likely to result in corrupted data, though this is also likely to get caught with validating writes. Worst case, the resulting data looks valid, but with weakened data retention. 2. Because becksums are stored in the copy-on-write metadata of the file, attempting to open a file twice for writing (or more advanced copy-on-write operations in the future) can lead to a situation where a prog is attempted on _already committed_ data. This is very bad and breaks copy-on-write guarantees. --- So clearly becksums are not fit for purpose and should be dropped. What can we replace them with? The first option, implemented here, is RAM-tracked erased state. Give each lfsr_file_t its own eblock/eoff fields to track the last known good erased-state. And before each prog, clear eblock/eoff so we never accidentally prog to the same erased-state twice. It's interesting to note we don't currently clear eblock/eoff in all file handles, this is ok only because we don't currently share eblock/eoff across file handles. Each eblock/eoff is exclusive to the lfsr_file_t and does not appear anywhere else in the system. The main downside of this approach is that, well, the RAM-tracked erase-state is only tracked in RAM. Block-level erased-state effectively does not persist across reboots. I've considered adding some sort of per-file erased-state tracking to the mdir that would need to be cleared before use, but such a mechanism ends up quite complicated. At the moment, I think the best second option is to put erased-state tracking in the future-planned bmap. This would let you opt-in to on-disk tracking of all erased-state in the system. One nice thing about RAM-tracked erased-state is that it's not on disk, so it's not really a compatibility concern and won't get in the way of additional future erased-state tracking. --- Benchmarking becksums vs RAM-tracking has been quite interesting. While in theory becksums can track much more erased-state, it's quite unlikely anything but the most recent erased-state actually ends up used. The end result is no real measurable performance loss, and actually a minor speedup because we don't need to calculate becksums on every block write. There are some pathological cases, such as multiple write heads, but these are out-of-scope right now (note! multiple explicit file handles currently handle this case beautifully because we don't share eblock/eoff!) Becksums were also relatively complicated, and needed extra scaffolding to pass around/propagate as secondary tags alongside the primary bptr. So trading these for RAM-tracking also gives us a nice bit of code/stack savings, albeit at a 2-word RAM cost in lfsr_file_t: code stack structs before: 33888 2864 1096 after: 33564 (-1.0%) 2816 (-1.7%) 1104 (+0.7%) lfsr_file_t before: 104 lfsr_file_t after: 112 (+7.7%) --- lfs.c | 158 ++++++++++++-------------------------------- lfs.h | 3 + scripts/dbgbmap.py | 1 - scripts/dbgbtree.py | 2 - scripts/dbglfs.py | 2 - scripts/dbgmtree.py | 2 - scripts/dbgrbyd.py | 2 - scripts/dbgtag.py | 2 - 8 files changed, 45 insertions(+), 127 deletions(-) diff --git a/lfs.c b/lfs.c index 7edf3752..5656d6df 100644 --- a/lfs.c +++ b/lfs.c @@ -743,7 +743,6 @@ enum lfsr_tag { LFSR_TAG_BSHRUB = 0x0308, LFSR_TAG_BTREE = 0x030c, LFSR_TAG_DID = 0x0310, - LFSR_TAG_BECKSUM = 0x0314, LFSR_TAG_BRANCH = 0x031c, LFSR_TAG_MROOT = 0x0321, LFSR_TAG_MDIR = 0x0325, @@ -9594,6 +9593,8 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file, file->m.flags = flags; file->cfg = cfg; file->pos = 0; + file->eblock = 0; + file->eoff = -1; // default data state file->bshrub = LFSR_BSHRUB_BNULL(); @@ -9859,7 +9860,7 @@ static lfs_ssize_t lfsr_bshrub_estimate(lfs_t *lfs, const lfsr_file_t *file) { static int lfsr_bshrub_lookupnext(lfs_t *lfs, const lfsr_file_t *file, lfs_off_t pos, lfsr_bid_t *bid_, lfsr_tag_t *tag_, lfsr_bid_t *weight_, - lfsr_bptr_t *bptr_, lfsr_ecksum_t *becksum_) { + lfsr_bptr_t *bptr_) { if (pos >= lfsr_bshrub_size(&file->bshrub)) { return LFS_ERR_NOENT; } @@ -9880,9 +9881,6 @@ static int lfsr_bshrub_lookupnext(lfs_t *lfs, const lfsr_file_t *file, if (bptr_) { bptr_->data = file->bshrub.u.bsprout; } - if (becksum_) { - becksum_->cksize = -1; - } return 0; // block pointer? @@ -9899,9 +9897,6 @@ static int lfsr_bshrub_lookupnext(lfs_t *lfs, const lfsr_file_t *file, if (bptr_) { *bptr_ = file->bshrub.u.bptr; } - if (becksum_) { - becksum_->cksize = -1; - } return 0; // bshrub/btree? @@ -9942,23 +9937,6 @@ static int lfsr_bshrub_lookupnext(lfs_t *lfs, const lfsr_file_t *file, } LFS_ASSERT(lfsr_data_size(bptr_->data) <= weight); } - if (becksum_) { - // need an extra lookup to find becksums - err = lfsr_rbyd_lookup(lfs, &rbyd, rid, LFSR_TAG_BECKSUM, - &data); - if (err && err != LFS_ERR_NOENT) { - return err; - } - - if (err == LFS_ERR_NOENT) { - becksum_->cksize = -1; - } else { - err = lfsr_data_readecksum(lfs, &data, becksum_); - if (err) { - return err; - } - } - } return 0; } else { @@ -10024,7 +10002,7 @@ static lfs_ssize_t lfsr_bshrub_readnext(lfs_t *lfs, const lfsr_file_t *file, lfsr_bid_t weight; lfsr_bptr_t bptr; int err = lfsr_bshrub_lookupnext(lfs, file, pos_, - &bid, &tag, &weight, &bptr, NULL); + &bid, &tag, &weight, &bptr); if (err) { return err; } @@ -10235,7 +10213,7 @@ evict:; static int lfsr_file_carve(lfs_t *lfs, lfsr_file_t *file, lfs_off_t pos, lfs_off_t weight, lfs_soff_t delta, - lfsr_tag_t tag, const lfsr_bptr_t *bptr, const lfsr_ecksum_t *becksum) { + lfsr_tag_t tag, const lfsr_bptr_t *bptr) { // Note! This function has some rather special constraints: // // 1. We must never allow our btree size to overflow, even temporarily. @@ -10255,7 +10233,7 @@ static int lfsr_file_carve(lfs_t *lfs, lfsr_file_t *file, lfsr_bid_t bid = lfsr_bshrub_size(&file->bshrub); lfsr_attr_t attrs[5]; lfs_size_t attr_count = 0; - uint8_t buf[3*LFSR_BPTR_DSIZE+2*LFSR_ECKSUM_DSIZE]; + uint8_t buf[3*LFSR_BPTR_DSIZE]; lfs_size_t buf_size = 0; // always convert to bshrub/btree when this function is called @@ -10316,14 +10294,12 @@ static int lfsr_file_carve(lfs_t *lfs, lfsr_file_t *file, lfsr_tag_t right_tag_ = 0; lfsr_bid_t right_weight_; lfsr_bptr_t right_bptr_; - lfsr_ecksum_t right_becksum_; while (pos < lfsr_bshrub_size(&file->bshrub)) { lfsr_tag_t tag_; lfsr_bid_t weight_; lfsr_bptr_t bptr_; - lfsr_ecksum_t becksum_; int err = lfsr_bshrub_lookupnext(lfs, file, pos, - &bid, &tag_, &weight_, &bptr_, &becksum_); + &bid, &tag_, &weight_, &bptr_); if (err) { LFS_ASSERT(err != LFS_ERR_NOENT); return err; @@ -10474,9 +10450,6 @@ static int lfsr_file_carve(lfs_t *lfs, lfsr_file_t *file, .cksum = bptr_.cksum, }; - // copy over becksum since erase-state is still valid - right_becksum_ = becksum_; - } else { LFS_UNREACHABLE(); } @@ -10515,14 +10488,6 @@ static int lfsr_file_carve(lfs_t *lfs, lfsr_file_t *file, lfsr_data_frombptr(bptr, &buf[buf_size])); buf_size += LFSR_BPTR_DSIZE; - // append becksum? - if (becksum && becksum->cksize != -1) { - attrs[attr_count++] = LFSR_ATTR( - LFSR_TAG_BECKSUM, 0, - lfsr_data_fromecksum(becksum, &buf[buf_size])); - buf_size += LFSR_ECKSUM_DSIZE; - } - } else { LFS_UNREACHABLE(); } @@ -10543,14 +10508,6 @@ static int lfsr_file_carve(lfs_t *lfs, lfsr_file_t *file, lfsr_data_frombptr(&right_bptr_, &buf[buf_size])); buf_size += LFSR_BPTR_DSIZE; - // copy over becksum since erase-state is still valid - if (right_becksum_.cksize != -1) { - attrs[attr_count++] = LFSR_ATTR( - LFSR_TAG_BECKSUM, 0, - lfsr_data_fromecksum(&right_becksum_, &buf[buf_size])); - buf_size += LFSR_ECKSUM_DSIZE; - } - } else { LFS_UNREACHABLE(); } @@ -10602,10 +10559,9 @@ static int lfsr_file_flush_(lfs_t *lfs, lfsr_file_t *file, lfsr_bid_t bid; lfsr_tag_t tag; lfsr_bid_t weight; - lfsr_ecksum_t becksum; int err = lfsr_bshrub_lookupnext(lfs, file, lfs_smax32(pos - (lfs->cfg->crystal_thresh-1), 0), - &bid, &tag, &weight, &bptr, &becksum); + &bid, &tag, &weight, &bptr); if (err) { LFS_ASSERT(err != LFS_ERR_NOENT); return err; @@ -10623,33 +10579,24 @@ static int lfsr_file_flush_(lfs_t *lfs, lfsr_file_t *file, } else { crystal_start = lfs_min32(bid+1, pos); - // wait, found block-level erased-state? + // wait, found erased-state? if (tag == LFSR_TAG_BLOCK - && becksum.cksize != -1 - // data not truncated? + && bptr.data.u.disk.block == file->eblock && bptr.data.u.disk.off + lfsr_data_size(bptr.data) - == bptr.cksize + == file->eoff // not clobbering data? && crystal_start - (bid-(weight-1)) >= lfsr_data_size(bptr.data) // enough for prog alignment? && crystal_end - crystal_start >= lfs->cfg->prog_size) { - LFS_ASSERT(bptr.cksize + becksum.cksize - <= lfs->cfg->block_size); + // mark as unerased in case of failure + file->eblock = 0; + file->eoff = -1; - err = lfsr_ecksum_validate(lfs, &becksum, - bptr.data.u.disk.block, bptr.cksize); - if (err && err != LFS_ERR_CORRUPT) { - return err; - } - - // found _valid_ block-level erased-state? eagerly - // append - if (err != LFS_ERR_CORRUPT) { - block_start = bid-(weight-1); - goto compact; - } + // try to use erased-state + block_start = bid-(weight-1); + goto compact; } } } @@ -10665,7 +10612,7 @@ static int lfsr_file_flush_(lfs_t *lfs, lfsr_file_t *file, lfs_min32( crystal_start + (lfs->cfg->crystal_thresh-1), lfsr_bshrub_size(&file->bshrub)-1), - &bid, &tag, &weight, &bptr, NULL); + &bid, &tag, &weight, &bptr); if (err) { LFS_ASSERT(err != LFS_ERR_NOENT); return err; @@ -10706,12 +10653,11 @@ static int lfsr_file_flush_(lfs_t *lfs, lfsr_file_t *file, lfsr_bid_t bid; lfsr_tag_t tag; lfsr_bid_t weight; - lfsr_ecksum_t becksum; int err = lfsr_bshrub_lookupnext(lfs, file, lfs_min32( crystal_start-1, lfsr_bshrub_size(&file->bshrub)-1), - &bid, &tag, &weight, &bptr, &becksum); + &bid, &tag, &weight, &bptr); if (err) { LFS_ASSERT(err != LFS_ERR_NOENT); return err; @@ -10723,32 +10669,20 @@ static int lfsr_file_flush_(lfs_t *lfs, lfsr_file_t *file, && lfsr_data_size(bptr.data) > 0) { block_start = bid-(weight-1); - // wait, found block-level erased-state? + // wait, found erased-state? if (tag == LFSR_TAG_BLOCK - && becksum.cksize != -1 - // data not truncated? + && bptr.data.u.disk.block == file->eblock && bptr.data.u.disk.off + lfsr_data_size(bptr.data) - == bptr.cksize + == file->eoff // not clobbering data? && crystal_start - (bid-(weight-1)) - >= lfsr_data_size(bptr.data) - // enough for prog alignment? - && crystal_end - crystal_start - >= lfs->cfg->prog_size) { - LFS_ASSERT(bptr.cksize + becksum.cksize - <= lfs->cfg->block_size); + >= lfsr_data_size(bptr.data)) { + // mark as unerased in case of failure + file->eblock = 0; + file->eoff = -1; - err = lfsr_ecksum_validate(lfs, &becksum, - bptr.data.u.disk.block, bptr.cksize); - if (err && err != LFS_ERR_CORRUPT) { - return err; - } - - // found _valid_ block-level erased-state? eagerly - // append - if (err != LFS_ERR_CORRUPT) { - goto compact; - } + // try to use erased-state + goto compact; } // no? is our left neighbor at least our left block neighbor? @@ -10821,7 +10755,7 @@ static int lfsr_file_flush_(lfs_t *lfs, lfsr_file_t *file, lfsr_bid_t weight_; lfsr_bptr_t bptr_; err = lfsr_bshrub_lookupnext(lfs, file, pos_, - &bid_, &tag_, &weight_, &bptr_, NULL); + &bid_, &tag_, &weight_, &bptr_); if (err) { LFS_ASSERT(err != LFS_ERR_NOENT); return err; @@ -10831,7 +10765,7 @@ static int lfsr_file_flush_(lfs_t *lfs, lfsr_file_t *file, // loop may never terminate if (bid_-(weight_-1) >= crystal_end // is this data a pure hole? stop early to better - // leverage becksums in sparse files + // leverage erased-state in sparse files && (pos_ >= bid_-(weight_-1) + lfsr_data_size(bptr_.data) // does this data exceed our block_size? @@ -10915,28 +10849,20 @@ static int lfsr_file_flush_(lfs_t *lfs, lfsr_file_t *file, bptr.cksize - bptr.data.u.disk.off); lfs_off_t block_end = block_start + lfsr_data_size(bptr.data); - // do we have space for a block ecksum? - lfsr_ecksum_t becksum = {.cksize=-1}; - if (bptr.cksize < lfs->cfg->block_size) { - becksum.cksize = lfs->cfg->prog_size; - becksum.cksum = 0; - err = lfsr_bd_cksum(lfs, - bptr.data.u.disk.block, bptr.cksize, becksum.cksize, - becksum.cksize, - &becksum.cksum); - if (err && err != LFS_ERR_CORRUPT) { - return err; - } - } - // and write it into our tree err = lfsr_file_carve(lfs, file, block_start, block_end - block_start, 0, - LFSR_TAG_BLOCK, &bptr, &becksum); + LFSR_TAG_BLOCK, &bptr); if (err) { return err; } + // keep track of any remaining erased-state + if (bptr.cksize < lfs->cfg->block_size) { + file->eblock = bptr.data.u.disk.block; + file->eoff = bptr.cksize; + } + // note compacting fragments -> blocks may not actually make any // progress on flushing the buffer on the first pass d = lfs_max32(pos, block_end) - pos; @@ -10969,7 +10895,7 @@ fragment:; lfsr_bptr_t bptr; int err = lfsr_bshrub_lookupnext(lfs, file, fragment_start-1, - &bid, &tag, &weight, &bptr, NULL); + &bid, &tag, &weight, &bptr); if (err) { LFS_ASSERT(err != LFS_ERR_NOENT); return err; @@ -11006,7 +10932,7 @@ fragment:; lfsr_bptr_t bptr; int err = lfsr_bshrub_lookupnext(lfs, file, fragment_end, - &bid, &tag, &weight, &bptr, NULL); + &bid, &tag, &weight, &bptr); if (err) { LFS_ASSERT(err != LFS_ERR_NOENT); return err; @@ -11036,7 +10962,7 @@ fragment:; lfsr_data_t data = lfsr_data_fromcat(datas, data_count); int err = lfsr_file_carve(lfs, file, fragment_start, fragment_end - fragment_start, 0, - LFSR_TAG_DATA, (const lfsr_bptr_t*)&data, NULL); + LFSR_TAG_DATA, (const lfsr_bptr_t*)&data); if (err && err != LFS_ERR_RANGE) { return err; } @@ -11617,7 +11543,7 @@ int lfsr_file_truncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) { lfs_min32(size, size_), size - lfs_min32(size, size_), +size_ - size, - LFSR_TAG_DATA, NULL, NULL); + LFSR_TAG_DATA, NULL); if (err) { goto failed; } @@ -11735,7 +11661,7 @@ int lfsr_file_fruncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) { 0, lfs_smax32(size - size_, 0), +size_ - size, - LFSR_TAG_DATA, NULL, NULL); + LFSR_TAG_DATA, NULL); if (err) { goto failed; } diff --git a/lfs.h b/lfs.h index 98429ba2..900a1070 100644 --- a/lfs.h +++ b/lfs.h @@ -526,6 +526,9 @@ typedef struct lfsr_file { uint8_t *buffer; lfs_size_t buffer_size; + lfs_block_t eblock; + lfs_size_t eoff; + const struct lfs_file_config *cfg; } lfsr_file_t; diff --git a/scripts/dbgbmap.py b/scripts/dbgbmap.py index f3bdf7f4..2b92d18d 100755 --- a/scripts/dbgbmap.py +++ b/scripts/dbgbmap.py @@ -33,7 +33,6 @@ TAG_BLOCK = 0x0304 TAG_BSHRUB = 0x0308 TAG_BTREE = 0x030c TAG_DID = 0x0310 -TAG_BECKSUM = 0x0314 TAG_BRANCH = 0x031c TAG_MROOT = 0x0321 TAG_MDIR = 0x0325 diff --git a/scripts/dbgbtree.py b/scripts/dbgbtree.py index 837699b0..9be818ef 100755 --- a/scripts/dbgbtree.py +++ b/scripts/dbgbtree.py @@ -31,7 +31,6 @@ TAG_BLOCK = 0x0304 TAG_BSHRUB = 0x0308 TAG_BTREE = 0x030c TAG_DID = 0x0310 -TAG_BECKSUM = 0x0314 TAG_BRANCH = 0x031c TAG_MROOT = 0x0321 TAG_MDIR = 0x0325 @@ -199,7 +198,6 @@ def tagrepr(tag, w=None, size=None, off=None): else 'bshrub' if (tag & 0xfff) == TAG_BSHRUB else 'btree' if (tag & 0xfff) == TAG_BTREE else 'did' if (tag & 0xfff) == TAG_DID - else 'becksum' if (tag & 0xfff) == TAG_BECKSUM else 'branch' if (tag & 0xfff) == TAG_BRANCH else 'mroot' if (tag & 0xfff) == TAG_MROOT else 'mdir' if (tag & 0xfff) == TAG_MDIR diff --git a/scripts/dbglfs.py b/scripts/dbglfs.py index f24ab967..7f628fb5 100755 --- a/scripts/dbglfs.py +++ b/scripts/dbglfs.py @@ -32,7 +32,6 @@ TAG_BLOCK = 0x0304 TAG_BSHRUB = 0x0308 TAG_BTREE = 0x030c TAG_DID = 0x0310 -TAG_BECKSUM = 0x0314 TAG_BRANCH = 0x031c TAG_MROOT = 0x0321 TAG_MDIR = 0x0325 @@ -230,7 +229,6 @@ def tagrepr(tag, w=None, size=None, off=None): else 'bshrub' if (tag & 0xfff) == TAG_BSHRUB else 'btree' if (tag & 0xfff) == TAG_BTREE else 'did' if (tag & 0xfff) == TAG_DID - else 'becksum' if (tag & 0xfff) == TAG_BECKSUM else 'branch' if (tag & 0xfff) == TAG_BRANCH else 'mroot' if (tag & 0xfff) == TAG_MROOT else 'mdir' if (tag & 0xfff) == TAG_MDIR diff --git a/scripts/dbgmtree.py b/scripts/dbgmtree.py index b719e93b..cd0fffbf 100755 --- a/scripts/dbgmtree.py +++ b/scripts/dbgmtree.py @@ -31,7 +31,6 @@ TAG_BLOCK = 0x0304 TAG_BSHRUB = 0x0308 TAG_BTREE = 0x030c TAG_DID = 0x0310 -TAG_BECKSUM = 0x0314 TAG_BRANCH = 0x031c TAG_MROOT = 0x0321 TAG_MDIR = 0x0325 @@ -214,7 +213,6 @@ def tagrepr(tag, w=None, size=None, off=None): else 'bshrub' if (tag & 0xfff) == TAG_BSHRUB else 'btree' if (tag & 0xfff) == TAG_BTREE else 'did' if (tag & 0xfff) == TAG_DID - else 'becksum' if (tag & 0xfff) == TAG_BECKSUM else 'branch' if (tag & 0xfff) == TAG_BRANCH else 'mroot' if (tag & 0xfff) == TAG_MROOT else 'mdir' if (tag & 0xfff) == TAG_MDIR diff --git a/scripts/dbgrbyd.py b/scripts/dbgrbyd.py index b75d73c3..83e35b3f 100755 --- a/scripts/dbgrbyd.py +++ b/scripts/dbgrbyd.py @@ -40,7 +40,6 @@ TAG_BLOCK = 0x0304 TAG_BSHRUB = 0x0308 TAG_BTREE = 0x030c TAG_DID = 0x0310 -TAG_BECKSUM = 0x0314 TAG_BRANCH = 0x031c TAG_MROOT = 0x0321 TAG_MDIR = 0x0325 @@ -201,7 +200,6 @@ def tagrepr(tag, w=None, size=None, off=None): else 'bshrub' if (tag & 0xfff) == TAG_BSHRUB else 'btree' if (tag & 0xfff) == TAG_BTREE else 'did' if (tag & 0xfff) == TAG_DID - else 'becksum' if (tag & 0xfff) == TAG_BECKSUM else 'branch' if (tag & 0xfff) == TAG_BRANCH else 'mroot' if (tag & 0xfff) == TAG_MROOT else 'mdir' if (tag & 0xfff) == TAG_MDIR diff --git a/scripts/dbgtag.py b/scripts/dbgtag.py index 03076500..85bc62f1 100755 --- a/scripts/dbgtag.py +++ b/scripts/dbgtag.py @@ -29,7 +29,6 @@ TAG_BLOCK = 0x0304 TAG_BSHRUB = 0x0308 TAG_BTREE = 0x030c TAG_DID = 0x0310 -TAG_BECKSUM = 0x0314 TAG_BRANCH = 0x031c TAG_MROOT = 0x0321 TAG_MDIR = 0x0325 @@ -159,7 +158,6 @@ def tagrepr(tag, w=None, size=None, off=None): else 'bshrub' if (tag & 0xfff) == TAG_BSHRUB else 'btree' if (tag & 0xfff) == TAG_BTREE else 'did' if (tag & 0xfff) == TAG_DID - else 'becksum' if (tag & 0xfff) == TAG_BECKSUM else 'branch' if (tag & 0xfff) == TAG_BRANCH else 'mroot' if (tag & 0xfff) == TAG_MROOT else 'mdir' if (tag & 0xfff) == TAG_MDIR