From adcf9924fec3c3430fc6b995bfedd013f511720a Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sun, 30 Jul 2023 14:05:30 -0500 Subject: [PATCH] Deduplicated uninling/split routes in lfsr_mdir_commit This means no special case for uninling-but-not-splitting, but allows the entire split route to be deduplicated, simplifying things. The main downside is that for littlefs to go from a single inlined mdir filesystem to an mtree filesystem it requires a minimum of 2 mdir allocations (4 blocks) in all cases. This can be avoided, but I think is worth the tradeoff since it generally occurs once in a filesystem's lifetime. This does make 4 block block devices a bit awkward, but those geometries are always going to be a bit awkward with littlefs's design. At least this implementation avoids an unecessary B-tree node where possible... code stack before: 22586 2320 after: 22414 (-0.8%) 2120 (-8.6%) I _think_, but haven't verified, the significant stack saving comes from the fact that since there's one route through lfsr_mtree_split_, lfsr_mtree_split_ can be inlined into lfsr_mdir_commit. This avoids the marshalling of all its arguments for the function call, which I've noticed can have a surprising cost. --- Also fixed a bug where dstart was not updated with mid changes after splits/drops. The mdir commit cleanup code has a lot of duplication now, makes me wonder if there's a better way to structure this. --- lfs.c | 259 +++++++++++++++++++-------------------------- tests/t5_dirs.toml | 4 +- 2 files changed, 111 insertions(+), 152 deletions(-) diff --git a/lfs.c b/lfs.c index d37e864a..e400deda 100644 --- a/lfs.c +++ b/lfs.c @@ -5698,9 +5698,6 @@ static int lfsr_mtree_split_(lfs_t *lfs, lfsr_btree_t *mtree_, } } - // we should have something to split here - LFS_ASSERT(split_id > 0 && split_id < mdir->rbyd.weight); - // compact into new mdir tags < split_id int err = lfsr_mdir_compact_(lfs, mdir_, mid, start_id, split_id, mdir, attrs, attr_count, NULL, 0); @@ -5710,7 +5707,7 @@ static int lfsr_mtree_split_(lfs_t *lfs, lfsr_btree_t *mtree_, } // compact into new mdir tags >= split_id - err = lfsr_mdir_compact_(lfs, msibling_, mid+1, split_id, end_id, + err = lfsr_mdir_compact_(lfs, msibling_, mid, split_id, end_id, mdir, attrs, attr_count, NULL, 0); if (err) { LFS_ASSERT(err != LFS_ERR_RANGE); @@ -5727,7 +5724,70 @@ static int lfsr_mtree_split_(lfs_t *lfs, lfsr_btree_t *mtree_, // because of defered commits, both children can still be reduced // to zero, need to catch this here - if (mdir_->rbyd.weight > 0 && msibling_->rbyd.weight > 0) { + + // both siblings reduced to zero + if (mdir_->rbyd.weight == 0 && msibling_->rbyd.weight == 0) { + LFS_DEBUG("Dropping mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", + mdir_->mid, + mdir_->rbyd.block, mdir_->redund_block); + LFS_DEBUG("Dropping mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", + msibling_->mid, + msibling_->rbyd.block, msibling_->redund_block); + mdir_->rbyd.trunk = 0; + msibling_->rbyd.trunk = 0; + + // update our mtree + int err = lfsr_btree_pop(lfs, mtree_, mid); + if (err) { + return err; + } + + // one sibling reduced to zero + } else if (mdir_->rbyd.weight == 0) { + LFS_DEBUG("Dropping mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", + mdir_->mid, + mdir_->rbyd.block, mdir_->redund_block); + mdir_->rbyd.trunk = 0; + + // update our mtree + uint8_t buf[LFSR_MPTR_DSIZE]; + lfs_ssize_t d = lfsr_mdir_todisk(lfs, msibling_, buf); + if (d < 0) { + return d; + } + + int err = lfsr_btree_set(lfs, mtree_, mid, LFSR_TAG_MDIR, 1, + LFSR_DATA_BUF(buf, d)); + if (err) { + return err; + } + + // other sibling reduced to zero + } else if (msibling_->rbyd.weight == 0) { + LFS_DEBUG("Dropping mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", + msibling_->mid, + msibling_->rbyd.block, msibling_->redund_block); + msibling_->rbyd.trunk = 0; + + // update our mtree + uint8_t buf[LFSR_MPTR_DSIZE]; + lfs_ssize_t d = lfsr_mdir_todisk(lfs, mdir_, buf); + if (d < 0) { + return d; + } + + int err = lfsr_btree_set(lfs, mtree_, mid, LFSR_TAG_MDIR, 1, + LFSR_DATA_BUF(buf, d)); + if (err) { + return err; + } + + // no siblings reduced to zero + } else { + // adjust our sibling's mid, do this here in case other sibling + // was dropped + msibling_->mid += 1; + // update out mtree // lookup first name in sibling to use as the split name @@ -5736,7 +5796,7 @@ static int lfsr_mtree_split_(lfs_t *lfs, lfsr_btree_t *mtree_, // case they introduce a new name! lfsr_tag_t stag; lfsr_data_t sdata; - err = lfsr_mdir_lookupnext(lfs, msibling_, 0, LFSR_TAG_NAME, + int err = lfsr_mdir_lookupnext(lfs, msibling_, 0, LFSR_TAG_NAME, NULL, &stag, &sdata); if (err) { LFS_ASSERT(err != LFS_ERR_NOENT); @@ -5763,63 +5823,6 @@ static int lfsr_mtree_split_(lfs_t *lfs, lfsr_btree_t *mtree_, if (err) { return err; } - - // one sibling reduced to zero - } else if (mdir_->rbyd.weight > 0) { - LFS_DEBUG("Dropping mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", - mdir_->mid, - mdir_->rbyd.block, mdir_->redund_block); - mdir_->rbyd.trunk = 0; - - // update our mtree - uint8_t buf[LFSR_MPTR_DSIZE]; - lfs_ssize_t d = lfsr_mdir_todisk(lfs, mdir_, buf); - if (d < 0) { - return d; - } - - err = lfsr_btree_set(lfs, mtree_, mid, LFSR_TAG_MDIR, 1, - LFSR_DATA_BUF(buf, d)); - if (err) { - return err; - } - - // other sibling reduced to zero - } else if (msibling_->rbyd.weight > 0) { - LFS_DEBUG("Dropping mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", - msibling_->mid, - msibling_->rbyd.block, msibling_->redund_block); - msibling_->rbyd.trunk = 0; - - // update our mtree - uint8_t buf[LFSR_MPTR_DSIZE]; - lfs_ssize_t d = lfsr_mdir_todisk(lfs, msibling_, buf); - if (d < 0) { - return d; - } - - err = lfsr_btree_set(lfs, mtree_, mid, LFSR_TAG_MDIR, 1, - LFSR_DATA_BUF(buf, d)); - if (err) { - return err; - } - - // both siblings reduced to zero - } else { - LFS_DEBUG("Dropping mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", - mdir_->mid, - mdir_->rbyd.block, mdir_->redund_block); - LFS_DEBUG("Dropping mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", - msibling_->mid, - msibling_->rbyd.block, msibling_->redund_block); - mdir_->rbyd.trunk = 0; - msibling_->rbyd.trunk = 0; - - // update our mtree - err = lfsr_btree_pop(lfs, mtree_, mid); - if (err) { - return err; - } } return 0; @@ -5882,92 +5885,29 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, lfs_ssize_t *rid, // // we need to update the mroot to track that a prog failed mroot_ = mdir_; - - // do we still need to split? - // - // note init_id is changed to 0 here, ignoring -1 attrs - int fits = lfsr_rbyd_estimate(lfs, &mdir->rbyd, 0, -1, - lfs->cfg->block_size/2, - &split_id); - if (fits < 0) { - return fits; - } - - // uninlining, but not splitting - if (fits) { - // compact into new mdir tags >= 0 - err = lfsr_mdir_compact_(lfs, &mdir_, 0, 0, -1, - mdir, attrs, attr_count, NULL, 0); - if (err) { - LFS_ASSERT(err != LFS_ERR_RANGE); - return err; - } - - LFS_DEBUG("Uninlining mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"} " - "-> 0x{%"PRIx32",%"PRIx32"}" - ", 0x{%"PRIx32",%"PRIx32"}", - mdir->mid, - mdir->rbyd.block, mdir->redund_block, - mdir->rbyd.block, mdir->redund_block, - mdir_.rbyd.block, mdir_.redund_block); - - // because of defered commits, our child can still be - // reduced to zero, need to catch this here - if (mdir_.rbyd.weight > 0) { - // update our mtree - uint8_t buf[LFSR_MPTR_DSIZE]; - lfs_ssize_t d = lfsr_mdir_todisk(lfs, &mdir_, buf); - if (d < 0) { - return d; - } - - err = lfsr_btree_push(lfs, &mtree_, 0, LFSR_TAG_MDIR, 1, - LFSR_DATA_BUF(buf, d)); - if (err) { - return err; - } - - } else { - LFS_DEBUG("Dropping mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", - mdir_.mid, - mdir_.rbyd.block, mdir_.redund_block); - mdir_.rbyd.trunk = 0; - - // don't really need to update our mtree here - } - - // uninlining and splitting - } else { - LFS_DEBUG("Uninlining mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", - mdir->mid, - mdir->rbyd.block, mdir->redund_block); - - // let lfsr_mtree_split_ do most of the work - int err = lfsr_mtree_split_(lfs, &mtree_, - &mdir_, &msibling_, 0, -1, - mdir, split_id, - attrs, attr_count); - if (err) { - return err; - } - } - - dirtymtree = true; - - // splitting a normal mdir - } else { - // let lfsr_mtree_split_ do most of the work - err = lfsr_mtree_split_(lfs, &mtree_, - &mdir_, &msibling_, -1, -1, - mdir, split_id, - attrs, attr_count); - if (err) { - return err; - } - - dirtymtree = true; } + // TODO should lfsr_rbyd_estimate just always do this? + // find estimate again, this time with start_id=0 so we + // ignore any -1 attrs + int fits = lfsr_rbyd_estimate(lfs, &mdir->rbyd, 0, -1, + lfs->cfg->block_size/2, + &split_id); + if (fits < 0) { + return fits; + } + + // let lfsr_mtree_split_ do most of the work + err = lfsr_mtree_split_(lfs, &mtree_, + &mdir_, &msibling_, 0, -1, + mdir, split_id, + attrs, attr_count); + if (err) { + return err; + } + + dirtymtree = true; + // mdir reduced to zero? need to drop? } else if (mdir->mid != LFSR_MID_MROOT && mdir_.rbyd.weight == 0) { LFS_DEBUG("Dropping mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", @@ -6291,16 +6231,35 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, lfs_ssize_t *rid, && opened->rid >= (lfs_ssize_t)mdir_.rbyd.weight) { LFS_ASSERT(lfsr_btree_weight(&mtree_) != lfsr_mtree_weight(lfs)); - opened->rid = opened->rid - mdir_.rbyd.weight; + opened->rid -= mdir_.rbyd.weight; opened->mdir = msibling_; } else { opened->mdir = mdir_; } - } else if (opened->mdir.mid > mdir->mid - && lfsr_btree_weight(&mtree_) != lfsr_mtree_weight(lfs)) { + } else if (opened->mdir.mid > mdir->mid) { opened->mdir.mid += lfsr_btree_weight(&mtree_) - lfsr_mtree_weight(lfs); } + + // update dstarts if we had a split or drop + if (type == LFS_TYPE_DIR) { + if (((lfsr_dir_t*)opened)->dstart_mid == mdir->mid) { + if (msibling_.rbyd.weight > 0 + && ((lfsr_dir_t*)opened)->dstart_rid + >= (lfs_ssize_t)mdir_.rbyd.weight) { + LFS_ASSERT(lfsr_btree_weight(&mtree_) + != lfsr_mtree_weight(lfs)); + ((lfsr_dir_t*)opened)->dstart_rid -= mdir_.rbyd.weight; + ((lfsr_dir_t*)opened)->dstart_mid = msibling_.mid; + } else { + ((lfsr_dir_t*)opened)->dstart_mid = mdir_.mid; + } + } else if (((lfsr_dir_t*)opened)->dstart_mid > mdir->mid) { + ((lfsr_dir_t*)opened)->dstart_mid + += lfsr_btree_weight(&mtree_) + - lfsr_mtree_weight(lfs); + } + } } } diff --git a/tests/t5_dirs.toml b/tests/t5_dirs.toml index 3848168a..2e3d4646 100644 --- a/tests/t5_dirs.toml +++ b/tests/t5_dirs.toml @@ -6086,7 +6086,7 @@ code = ''' # # This is a useful feature, but it's unintuitive if this should have # well-defined behavior, so make sure to test for it -[cases.t5_dirs_rm_recursive] +[cases.t5_dirs_recursive_rm] defines.N = [1, 2, 4, 8, 16, 32, 64, 128, 256, 512] defines.PARENT = [false, true] # 0 => don't seek @@ -6208,7 +6208,7 @@ code = ''' # # This is a useful feature, but it's unintuitive if this should have # well-defined behavior, so make sure to test for it -[cases.t5_dirs_mv_recursive] +[cases.t5_dirs_recursive_mv] defines.N = [1, 2, 4, 8, 16, 32, 64, 128, 256, 512] defines.BEFORE = [false, true] # 0 => no