From c37bab6040f374030b3b09f50ba5812532d2ca5b Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sun, 6 Aug 2023 01:21:05 -0500 Subject: [PATCH] Reworked rbyd/btree/mdir structs again so redund blocks are at the end For a couple reasons: 1. Organizing the overlaps this way avoid potential undefined behavior. It turns out C does define the overlap the "initial sequence" of union members, as long as the types are the same. But when we overlapped the block with the size/tag fields in lfsr_btree_t, it was probably undefined behavior. At the very least, it would introduce a need for quite a bit of preprocessing to make it work with different integer sizes and redundancy levels. 2. Overlapping the blocks at the end of the rbyd struct means our block array is natural ordered such that the first block is the "active" block, i.e. the block with the most recent revision count that passes checksums. This has been useful as a debugging tool, so I would like to continue the pattern. It is possible to mostly preserve this order with the previous method by intentional reversing the block array when logging or writing to disk, but it's a bit cumbersome. 2. It's unlikely we'll be able to use readonly variants of the rbyd/mdir structs for RAM savings. Unfortunately C makes this too cumbersome. Though if we do this should be revisited. Here are the new overlaps. Note it's no longer possible to truncate the types when readonly. If readonly struct are useful this will need to be revisited again: lfsr_rbyd_t lfsr_btree_t lfsr_mdir_t 8b 8b 8b 8b .----+----+----+----. 8b 8b 8b 8b 8b 8b 8b 8b | mid.bid | mid.rid | .----+----+----+----. .----+----+----+----. |----+----+----+----| | weight |.>| weight | | weight | |----+----+----+----| |----+----+----+----| |----+----+----+----| | trunk | | tag | size | | trunk | |----+----+----+----| |----+----+----+----| |----+----+----+----| | off | | inlined data | | off | |----+----+----+----| | | | |----+----+----+----| | crc | | v | | crc | |----+----+----+----| | | |----+----+----+----| | block |..| |.>| blocks | '----+----+----+----' '----+----+----+----' | | | | '----+----+----+----' --- lfs.c | 32 ++++++++++++++++---------------- lfs.h | 20 +++++++++----------- tests/test_mtree.toml | 14 +++++++------- 3 files changed, 32 insertions(+), 34 deletions(-) diff --git a/lfs.c b/lfs.c index 8d607e23..5998fdb2 100644 --- a/lfs.c +++ b/lfs.c @@ -5640,9 +5640,9 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, "-> 0x{%"PRIx32",%"PRIx32"}" ", 0x{%"PRIx32",%"PRIx32"}", mdir->mid.bid, - mdir->u.m.blocks[1], mdir->u.m.blocks[0], - mdir_.u.m.blocks[1], mdir_.u.m.blocks[0], - msibling_.u.m.blocks[1], msibling_.u.m.blocks[0]); + mdir->u.m.blocks[0], mdir->u.m.blocks[1], + mdir_.u.m.blocks[0], mdir_.u.m.blocks[1], + msibling_.u.m.blocks[0], msibling_.u.m.blocks[1]); // because of defered commits, both children can still be reduced // to zero, need to catch this here @@ -5651,10 +5651,10 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, if (mdir_.u.m.weight == 0 && msibling_.u.m.weight == 0) { LFS_DEBUG("Dropping mdir %"PRId16" 0x{%"PRIx32",%"PRIx32"}", mdir_.mid.bid, - mdir_.u.m.blocks[1], mdir_.u.m.blocks[0]); + mdir_.u.m.blocks[0], mdir_.u.m.blocks[1]); LFS_DEBUG("Dropping mdir %"PRId16" 0x{%"PRIx32",%"PRIx32"}", msibling_.mid.bid, - msibling_.u.m.blocks[1], msibling_.u.m.blocks[0]); + msibling_.u.m.blocks[0], msibling_.u.m.blocks[1]); // mark as dropped mdir_.u.r.rbyd.trunk = 0; msibling_.u.r.rbyd.trunk = 0; @@ -5669,7 +5669,7 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, } else if (mdir_.u.m.weight == 0) { LFS_DEBUG("Dropping mdir %"PRId16" 0x{%"PRIx32",%"PRIx32"}", mdir_.mid.bid, - mdir_.u.m.blocks[1], mdir_.u.m.blocks[0]); + mdir_.u.m.blocks[0], mdir_.u.m.blocks[1]); // mark as dropped mdir_.u.r.rbyd.trunk = 0; @@ -5691,7 +5691,7 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, } else if (msibling_.u.m.weight == 0) { LFS_DEBUG("Dropping mdir %"PRId16" 0x{%"PRIx32",%"PRIx32"}", msibling_.mid.bid, - msibling_.u.m.blocks[1], msibling_.u.m.blocks[0]); + msibling_.u.m.blocks[0], msibling_.u.m.blocks[1]); // mark as dropped msibling_.u.r.rbyd.trunk = 0; @@ -5758,7 +5758,7 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, } else if (mdir->mid.bid != -1 && mdir_.u.m.weight == 0) { LFS_DEBUG("Dropping mdir %"PRId16" 0x{%"PRIx32",%"PRIx32"}", mdir->mid.bid, - mdir->u.m.blocks[1], mdir->u.m.blocks[0]); + mdir->u.m.blocks[0], mdir->u.m.blocks[1]); // mark as dropped mdir_.u.r.rbyd.trunk = 0; @@ -5784,8 +5784,8 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, LFS_DEBUG("Relocating mdir %"PRId16" 0x{%"PRIx32",%"PRIx32"} " "-> 0x{%"PRIx32",%"PRIx32"}", mdir->mid.bid, - mdir->u.m.blocks[1], mdir->u.m.blocks[0], - mdir_.u.m.blocks[1], mdir_.u.m.blocks[0]); + mdir->u.m.blocks[0], mdir->u.m.blocks[1], + mdir_.u.m.blocks[0], mdir_.u.m.blocks[1]); // update our mtree uint8_t buf[LFSR_MDIR_DSIZE]; @@ -5918,8 +5918,8 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, LFS_DEBUG("Relocating mroot 0x{%"PRIx32",%"PRIx32"} " "-> 0x{%"PRIx32",%"PRIx32"}", - mchildroot.u.m.blocks[1], mchildroot.u.m.blocks[0], - mchildroot_[1], mchildroot_[0]); + mchildroot.u.m.blocks[0], mchildroot.u.m.blocks[1], + mchildroot_[0], mchildroot_[1]); // commit mrootchild uint8_t buf[LFSR_MDIR_DSIZE]; @@ -5951,9 +5951,9 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, LFS_DEBUG("Extending mroot 0x{%"PRIx32",%"PRIx32"}" " -> 0x{%"PRIx32",%"PRIx32"}" ", 0x{%"PRIx32",%"PRIx32"}", - mchildroot.u.m.blocks[1], mchildroot.u.m.blocks[0], - mchildroot.u.m.blocks[1], mchildroot.u.m.blocks[0], - mchildroot_[1], mchildroot_[0]); + mchildroot.u.m.blocks[0], mchildroot.u.m.blocks[1], + mchildroot.u.m.blocks[0], mchildroot.u.m.blocks[1], + mchildroot_[0], mchildroot_[1]); // copy magic/config from current mroot lfsr_data_t magic; @@ -6534,7 +6534,7 @@ cycle_detect:; traversal->tortoise_blocks) == 0) { LFS_ERROR("Cycle detected during mtree traversal " "(0x{%"PRIx32",%"PRIx32"})", - traversal->mdir.u.m.blocks[1], traversal->mdir.u.m.blocks[0]); + traversal->mdir.u.m.blocks[0], traversal->mdir.u.m.blocks[1]); return LFS_ERR_CORRUPT; } if (traversal->tortoise_step diff --git a/lfs.h b/lfs.h index 947ede2e..e1005424 100644 --- a/lfs.h +++ b/lfs.h @@ -346,8 +346,6 @@ typedef struct lfs_cache { // TODO do we get ram savings with a lfsr_rorbyd_t substruct? need to measure typedef struct lfsr_rbyd { - // note this lines up with arrays of redundant blocks in other structures - lfs_block_t block; // note this lines up with weight in lfsr_btree_t lfs_size_t weight; // off=0, trunk=0 => not yet committed @@ -356,6 +354,8 @@ typedef struct lfsr_rbyd { lfs_off_t trunk; lfs_off_t off; uint32_t crc; + // note this lines up with arrays of redundant blocks in lfsr_mdir_t + lfs_block_t block; } lfsr_rbyd_t; // The maximum size of inlined pointers in a btree, this depends on littlefs's @@ -372,20 +372,15 @@ typedef struct lfsr_btree { // weight is common to both representations and its sign-bit indicates // if the btree is inlined struct { - lfs_size_t _padding; lfs_size_t weight; } b; struct { - // TODO complex ifdefs here to handle tag/size > 1/2 a word? - lfsr_tag_t tag; - uint8_t size; - uint8_t _padding; lfs_size_t weight; + lfsr_tag_t tag; + uint16_t size; uint8_t buffer[LFSR_BTREE_INLINESIZE]; } i; struct { - // note the sign bit of the rbyd.block indicates if the btree is - // inlined or a normal btree lfsr_rbyd_t rbyd; } r; } u; @@ -397,12 +392,15 @@ typedef struct lfsr_mdir { // here we make sure to line up our block array so it overlaps with // the block stored as the first entry in the rbyd struct { - lfs_block_t blocks[2]; lfs_size_t weight; + lfs_off_t trunk; + lfs_off_t off; + uint32_t crc; + lfs_block_t blocks[2]; } m; struct { - lfs_block_t redund_block; lfsr_rbyd_t rbyd; + lfs_block_t redund_block; } r; } u; } lfsr_mdir_t; diff --git a/tests/test_mtree.toml b/tests/test_mtree.toml index a4d29dbe..1c171d0f 100644 --- a/tests/test_mtree.toml +++ b/tests/test_mtree.toml @@ -3506,7 +3506,7 @@ code = ''' mid_.bid, mid_.rid, tag_, - mdir->u.m.blocks[1], mdir->u.m.blocks[0]); + mdir->u.m.blocks[0], mdir->u.m.blocks[1]); // keep track of seen blocks seen[mdir->u.m.blocks[1] / 8] |= 1 << (mdir->u.m.blocks[1] % 8); @@ -3631,7 +3631,7 @@ code = ''' mid_.bid, mid_.rid, tag_, - mdir->u.m.blocks[1], mdir->u.m.blocks[0]); + mdir->u.m.blocks[0], mdir->u.m.blocks[1]); // keep track of seen blocks seen[mdir->u.m.blocks[1] / 8] |= 1 << (mdir->u.m.blocks[1] % 8); @@ -3767,7 +3767,7 @@ code = ''' mid_.bid, mid_.rid, tag_, - mdir->u.m.blocks[1], mdir->u.m.blocks[0]); + mdir->u.m.blocks[0], mdir->u.m.blocks[1]); // keep track of seen blocks seen[mdir->u.m.blocks[1] / 8] |= 1 << (mdir->u.m.blocks[1] % 8); @@ -3895,7 +3895,7 @@ code = ''' mid_.bid, mid_.rid, tag_, - mdir->u.m.blocks[1], mdir->u.m.blocks[0]); + mdir->u.m.blocks[0], mdir->u.m.blocks[1]); // keep track of seen blocks seen[mdir->u.m.blocks[1] / 8] |= 1 << (mdir->u.m.blocks[1] % 8); @@ -4031,7 +4031,7 @@ code = ''' mid_.bid, mid_.rid, tag_, - mdir->u.m.blocks[1], mdir->u.m.blocks[0]); + mdir->u.m.blocks[0], mdir->u.m.blocks[1]); // keep track of seen blocks seen[mdir->u.m.blocks[1] / 8] |= 1 << (mdir->u.m.blocks[1] % 8); @@ -4194,7 +4194,7 @@ code = ''' mid_.bid, mid_.rid, tag_, - mdir->u.m.blocks[1], mdir->u.m.blocks[0]); + mdir->u.m.blocks[0], mdir->u.m.blocks[1]); // keep track of seen blocks seen[mdir->u.m.blocks[1] / 8] |= 1 << (mdir->u.m.blocks[1] % 8); @@ -4298,7 +4298,7 @@ code = ''' mid_.bid, mid_.rid, tag_, - mdir->u.m.blocks[1], mdir->u.m.blocks[0]); + mdir->u.m.blocks[0], mdir->u.m.blocks[1]); } else { // this shouldn't happen printf("traversal: %d.%d 0x%x %d\n",