From 9cf115685b71bef80adaf5f657b17d77691a76d4 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 20 Mar 2024 00:38:54 -0500 Subject: [PATCH] Don't duplicate config across all mroots, only magic So, duplicating the config across multiple mroots allows a better chance of manual recovery if things go wrong, right? .--------. .--------. .|littlefs|->|littlefs| ||bs=4096 | ||bs=4096 | ||bc=256 | ||bc=256 | ||crc32c | ||root dir| || | ||crc32c | |'--------' |'--------' '--------' '--------' Well, this was the original thinking. But now I'm starting to think the duplicated config isn't actually all that useful: 1. The config may be out-of-date, since only the last mroot is mutable. This is becoming more common as littlefs evolves (on-disk version bumps, compat flag changes, fs_grow, etc). 2. The most important bit of information is where the mtree is. And this information is _only_ available on the last mroot because of mutability requirements. At the very least, all mroots point to the mtree (not Rome). So finding the mtree may not be that hard if you find any mroot. But this also gives you all the config sooo... 3. gstate is going to be hard (impossible?) to reconstruct anyways. Though for reading this may only be an issue for interrupted grms. 4. Let's be honest, manual recovery is not going to be a common occurence for these devices. Point 1. is a the main issue and actually highlights a real risk with duplicated config: It's easy to pick up out-of-date config. In other implementations this risks easy mistakes that are hard to notice until complex filesystem states. Assuming the first mroot contains up-to-date config for the obvious example. Even in our current implementation, duplicated config already poses some tricky hard-to-get-right problems. What happens if we run into an unknown compat flag on a not-last mroot? Hint, our implementation was broken! --- So this commit changes mroot extension to _not_ duplicate config, but instead just rewrite the magic string and mroot chain to the new mroot anchor: .--------. .--------. .|littlefs|->|littlefs| ||crc32c | ||bs=4096 | || | ||bc=256 | || | ||root dir| || | ||crc32c | |'--------' |'--------' '--------' '--------' This leads to a nice simplification in lfsr_mdir_commit, so that's a plus. It does make lfsr_mount a bit trickier, since we need to figure out which mroot is the last mroot before checking for config. But we would need something trickier in lfsr_mount anyways to handle the above out-of-date issues anyways. The current implementation just does a redundant mroot lookup to figure out if the current mroot is the last mroot. In theory this could be avoided, but I couldn't figure out how to without making the code unreasonably complex (lfsr_mount is already intertwined with lfsr_traversal_read). The end result is a bit of code and stack savings, thanks to lfsr_mdir_commit being on the stack-depth hot-path (deep-path?): code stack before: 34060 2880 after: 33996 (-0.2%) 2864 (-0.6%) It's also worth noting that there are plans to add block-level redundancy at some point. Maybe it's best to leave recovering from missing blocks to block-level redundancy which is actually designed for this, and let the mroot chain do what the mroot chain does best: allowing the mroots to participate in wear-leveling. --- lfs.c | 114 ++++++++++++++++++++++------------------------------------ 1 file changed, 43 insertions(+), 71 deletions(-) diff --git a/lfs.c b/lfs.c index a02a2378..c2e8b30c 100644 --- a/lfs.c +++ b/lfs.c @@ -6806,52 +6806,20 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, mrootchild.rbyd.blocks[0], mrootchild.rbyd.blocks[1], mrootchild_.rbyd.blocks[0], mrootchild_.rbyd.blocks[1]); - // compact into the new mroot anchor + // commit the new mroot anchor lfsr_mdir_t mrootanchor_; err = lfsr_mdir_swap__(lfs, &mrootanchor_, &mrootchild, -1); if (err) { return err; } - // copy only the config over - lfsr_tag_t tag = 0; - while (true) { - lfsr_srid_t rid; - lfsr_rid_t weight; - lfsr_data_t data; - err = lfsr_rbyd_lookupnext(lfs, &mrootchild.rbyd, - -1, tag+1, - &rid, &tag, &weight, &data); - if (err) { - if (err == LFS_ERR_NOENT) { - break; - } - return err; - } - if (rid != -1 || lfsr_tag_suptype(tag) != LFSR_TAG_CONFIG) { - break; - } - - // write the tag - err = lfsr_rbyd_appendcompactattr(lfs, &mrootanchor_.rbyd, - tag, weight, data); - if (err) { - LFS_ASSERT(err != LFS_ERR_RANGE); - return err; - } - } - - err = lfsr_rbyd_appendcompaction(lfs, &mrootanchor_.rbyd, 0); - if (err) { - LFS_ASSERT(err != LFS_ERR_RANGE); - return err; - } - - // and commit our new mroot err = lfsr_mdir_commit__(lfs, &mrootanchor_, -1, -1, -1, LFSR_ATTRS( LFSR_ATTR( - LFSR_TAG_SUB | LFSR_TAG_MROOT, 0, + LFSR_TAG_MAGIC, 0, + LFSR_DATA_BUF("littlefs", 8)), + LFSR_ATTR( + LFSR_TAG_MROOT, 0, LFSR_DATA_FROMMPTR(lfsr_mdir_mptr(&mrootchild_))))); if (err) { LFS_ASSERT(err != LFS_ERR_RANGE); @@ -7942,31 +7910,10 @@ static int lfs_init(lfs_t *lfs, const struct lfs_config *cfg); static int lfs_deinit(lfs_t *lfs); static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { - // has magic string? - lfsr_data_t data; - int err = lfsr_mdir_lookup(lfs, mroot, LFSR_TAG_MAGIC, - &data); - if (err) { - if (err == LFS_ERR_NOENT) { - LFS_ERROR("No littlefs magic found"); - return LFS_ERR_INVAL; - } - return err; - } - - lfs_scmp_t cmp = lfsr_data_cmp(lfs, data, "littlefs", 8); - if (cmp < 0) { - return cmp; - } - - // treat corrupted magic as no magic - if (cmp != LFS_CMP_EQ) { - LFS_ERROR("No littlefs magic found"); - return LFS_ERR_INVAL; - } - // check the disk version - err = lfsr_mdir_lookup(lfs, mroot, LFSR_TAG_VERSION, + uint8_t version[2] = {0, 0}; + lfsr_data_t data; + int err = lfsr_mdir_lookup(lfs, mroot, LFSR_TAG_VERSION, &data); if (err) { if (err == LFS_ERR_NOENT) { @@ -7975,8 +7922,6 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { } return err; } - - uint8_t version[2] = {0, 0}; lfs_ssize_t d = lfsr_data_read(lfs, &data, version, 2); if (d < 0) { return err; @@ -8008,7 +7953,6 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { } } - // incompatible rcompat flags? if (lfsr_rcompat_isincompat(rcompat)) { LFS_ERROR("Incompatible rcompat flags 0x%0"PRIx16 " (!= 0x%0"PRIx16")", @@ -8032,7 +7976,6 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { } } - // incompatible wcompat flags? // TODO switch to readonly? if (lfsr_wcompat_isincompat(wcompat)) { LFS_ERROR("Incompatible wcompat flags 0x%0"PRIx16 @@ -8046,6 +7989,7 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { // ignore these anyways // check the on-disk geometry + lfsr_geometry_t geometry; err = lfsr_mdir_lookup(lfs, mroot, LFSR_TAG_GEOMETRY, &data); if (err) { @@ -8055,8 +7999,6 @@ static int lfsr_mountmroot(lfs_t *lfs, const lfsr_mdir_t *mroot) { } return err; } - - lfsr_geometry_t geometry; err = lfsr_data_readgeometry(lfs, &data, &geometry); if (err) { return err; @@ -8174,14 +8116,44 @@ static int lfsr_mountinited(lfs_t *lfs) { if (tinfo.tag == LFSR_TAG_MDIR) { // found an mroot? if (tinfo.u.mdir.mid == -1) { - err = lfsr_mountmroot(lfs, &tinfo.u.mdir); + // check for the magic string, all mroot should have this + lfsr_data_t data; + int err = lfsr_mdir_lookup(lfs, &tinfo.u.mdir, LFSR_TAG_MAGIC, + &data); if (err) { + if (err == LFS_ERR_NOENT) { + LFS_ERROR("No littlefs magic found"); + return LFS_ERR_INVAL; + } return err; } - // keep track of the last mroot we see, this is the - // active mroot - lfs->mroot = tinfo.u.mdir; + // treat corrupted magic as no magic + lfs_scmp_t cmp = lfsr_data_cmp(lfs, data, "littlefs", 8); + if (cmp < 0) { + return cmp; + } + if (cmp != LFS_CMP_EQ) { + LFS_ERROR("No littlefs magic found"); + return LFS_ERR_INVAL; + } + + // are we the last mroot? + err = lfsr_mdir_lookup(lfs, &tinfo.u.mdir, LFSR_TAG_MROOT, + NULL); + if (err && err != LFS_ERR_NOENT) { + return err; + } + if (err == LFS_ERR_NOENT) { + // track active mroot + lfs->mroot = tinfo.u.mdir; + + // mount/validate config in active mroot + err = lfsr_mountmroot(lfs, &lfs->mroot); + if (err) { + return err; + } + } } else { // found a direct mdir? keep track of this