From e4ba43dd5fd5c5dd00eaccd5116e59b67133b028 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sun, 23 Jul 2023 11:48:17 -0500 Subject: [PATCH] Extended grm to support two atomics removes Ugh. I overlooked a weird corner case in rename's behavior that requires changes to the grm to support. POSIX's rename, which lfsr_rename is trying to match, supports renaming files over existing files, effectively removing the previous file during the rename. This is supported, even if the files are directories, but with the additional requirement that the previous directory is empty (matching the behavior of lfsr_remove). This creates a weird situation for littlefs. In order to remove directories in littlefs, we need to atomically remove both the dstart entry that reserves the directory's did and the directories entry in its parent. This is made possible by using the grm to mark one entry as pending removed while removing the other. But in order to rename atomically, we need to use the grm to mark the source of the rename as removed while creating/replacing the destination of the rename. So we end up needing two grms simultaneously. This is extra annoying because the niche case of renaming a directory over another empty directory is the only case where we need two grms, but this requirement almost doubles the grm size both in-ram and reserved in every mdir, from 11 bytes to 21 bytes, and increases the lfs_t size by 28 bytes. --- Anyways, this commit extends the grm to support up to two pending removes. Fortunately the implementation was simple since we already have a type field that can be extended, and grm operations just needed to be changed from if statements to for loops. --- lfs.c | 312 ++++++++++++++++++++++++++-------------------- lfs.h | 14 ++- scripts/dbglfs.py | 17 ++- 3 files changed, 197 insertions(+), 146 deletions(-) diff --git a/lfs.c b/lfs.c index 23347985..bbf1bb16 100644 --- a/lfs.c +++ b/lfs.c @@ -1563,47 +1563,57 @@ static int lfsr_gdelta_xor(lfs_t *lfs, // GRM (global remove) things static inline bool lfsr_grm_hasrm(const lfsr_grm_t *grm) { - return grm->mid != LFSR_MID_RM; + return grm->rms[0].mid != LFSR_MID_RM; } -static inline void lfsr_grm_clearrm(lfsr_grm_t *grm) { - grm->mid = LFSR_MID_RM; +static inline uint8_t lfsr_grm_count(const lfsr_grm_t *grm) { + return (grm->rms[0].mid != LFSR_MID_RM) + + (grm->rms[1].mid != LFSR_MID_RM); +} + +static inline void lfsr_grm_pushrm(lfsr_grm_t *grm, + lfs_size_t mid, lfs_size_t rid) { + LFS_ASSERT(grm->rms[1].mid == LFSR_MID_RM); + grm->rms[1] = grm->rms[0]; + grm->rms[0].mid = mid; + grm->rms[0].rid = rid; +} + +static inline void lfsr_grm_poprm(lfsr_grm_t *grm) { + grm->rms[0] = grm->rms[1]; + grm->rms[1].mid = LFSR_MID_RM; } static lfs_ssize_t lfsr_grm_todisk(lfs_t *lfs, const lfsr_grm_t *grm, uint8_t buffer[static LFSR_GRM_DSIZE]) { (void)lfs; - // encode no-rm as zero-size - if (!lfsr_grm_hasrm(grm)) { - return 0; - } - // We encode grms with a byte indicating if a remove is pending. This - // sounds a bit wasteful, but avoids issues with signed-leb128 encoding, - // and allows grm to possible be expanded to other operations in the - // future. - // - // maybe grm=2 will encode the mroot in the future? who knows, spooky - // + // first encode the number of grms, this can be 0, 1, or 2 and may + // be extended to a general purpose leb128 type field in the future + // encode no-rm as zero-size + uint8_t count = lfsr_grm_count(grm); lfs_ssize_t d = 0; - buffer[d] = 0x01; + buffer[d] = count; d += 1; // TODO is this really the best way to do this? should we just allow // mid=0 to be mroot when mtree is inlined? - // map mid=-1 (mroot) to mid=0 - lfs_ssize_t d_ = lfs_toleb128(lfs_smax32(grm->mid, 0), &buffer[d], 5); - if (d_ < 0) { - return d_; - } - d += d_; + for (uint8_t i = 0; i < count; i++) { + // map mid=-1 (mroot) to mid=0 + lfs_ssize_t d_ = lfs_toleb128( + lfs_smax32(grm->rms[i].mid, 0), &buffer[d], 5); + if (d_ < 0) { + return d_; + } + d += d_; - d_ = lfs_toleb128(grm->rid, &buffer[d], 5); - if (d_ < 0) { - return d_; + d_ = lfs_toleb128(grm->rms[i].rid, &buffer[d], 5); + if (d_ < 0) { + return d_; + } + d += d_; } - d += d_; return d; } @@ -1614,50 +1624,49 @@ static inline int lfsr_mtree_isinlined(lfs_t *lfs); static lfs_ssize_t lfsr_grm_fromdisk(lfs_t *lfs, lfsr_grm_t *grm, lfsr_data_t data) { lfs_ssize_t d = 0; - uint8_t op; - lfs_ssize_t d_ = lfsr_data_read(lfs, data, d, &op, 1); + uint8_t count = 0; + lfs_ssize_t d_ = lfsr_data_read(lfs, data, d, &count, 1); if (d_ < 0) { return d_; } d += d_; - // no rm, note we accept truncated grms here - if (op == 0 || d_ == 0) { - lfsr_grm_clearrm(grm); - return 0; - } + // clear first + grm->rms[0].mid = LFSR_MID_RM; + grm->rms[1].mid = LFSR_MID_RM; - lfs_size_t mid; - d_ = lfsr_data_readleb128(lfs, data, d, &mid); - if (d_ < 0) { - return d_; + LFS_ASSERT(count <= 2); + for (uint8_t i = 0; i < count; i++) { + lfs_size_t mid; + d_ = lfsr_data_readleb128(lfs, data, d, &mid); + if (d_ < 0) { + return d_; + } + d += d_; + // TODO should these checks be in lfsr_data_readleb128? + LFS_ASSERT(mid < 0x7fffffff); + + lfs_size_t rid; + d_ = lfsr_data_readleb128(lfs, data, d, &rid); + if (d_ < 0) { + return d_; + } + d += d_; + // TODO should these checks be in lfsr_data_readleb128? + LFS_ASSERT(rid < 0x7fffffff); + + // TODO is this really the best way to do this? should we just allow + // mid=0 to be mroot when mtree is inlined? + + // adjust mid if mtree is inlined + if (lfsr_mtree_isinlined(lfs)) { + LFS_ASSERT(mid == 0); + mid = LFSR_MID_MROOT; + } + + grm->rms[i].mid = mid; + grm->rms[i].rid = rid; } - d += d_; - - lfs_size_t rid; - d_ = lfsr_data_readleb128(lfs, data, d, &rid); - if (d_ < 0) { - return d_; - } - d += d_; - - // TODO wait assert or error? - LFS_ASSERT(op == 1); - // TODO should these checks be in lfsr_data_readleb128? - LFS_ASSERT(mid < 0x7fffffff); - LFS_ASSERT(rid < 0x7fffffff); - - // TODO is this really the best way to do this? should we just allow - // mid=0 to be mroot when mtree is inlined? - - // adjust mid if mtree is inlined - if (lfsr_mtree_isinlined(lfs)) { - LFS_ASSERT(mid == 0); - mid = LFSR_MID_MROOT; - } - - grm->mid = mid; - grm->rid = rid; return d; } @@ -5961,21 +5970,25 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, lfs_ssize_t *rid, return d; } - if (grm.mid == mdir->mid) { - LFS_ASSERT(grm.rid <= mdir->rbyd.weight); - // TODO do we need this if we allow mid=0 => mroot when inlined? - // update mid if we are uninlining - grm.mid = lfs_smax32(grm.mid, 0); + for (uint8_t j = 0; j < 2; j++) { + if (grm.rms[j].mid == mdir->mid) { + LFS_ASSERT(grm.rms[j].rid <= mdir->rbyd.weight); + // TODO do we need this if we allow mid=0 => mroot when + // inlined? + // update mid if we are uninlining + grm.rms[j].mid = lfs_smax32(grm.rms[j].mid, 0); - if (grm.rid >= mdir_.rbyd.weight) { - grm.mid += 1; - grm.rid -= mdir_.rbyd.weight; + if (grm.rms[j].rid >= mdir_.rbyd.weight) { + grm.rms[j].mid += 1; + grm.rms[j].rid -= mdir_.rbyd.weight; + } + // update mid if we had a split or drop + } else if (grm.rms[j].mid > mdir->mid + && lfsr_btree_weight(&mtree_) + != lfsr_mtree_weight(lfs)) { + grm.rms[j].mid += lfsr_btree_weight(&mtree_) + - lfsr_mtree_weight(lfs); } - // update mid if we had a split or drop - } else if (grm.mid > mdir->mid - && lfsr_btree_weight(&mtree_) != lfsr_mtree_weight(lfs)) { - grm.mid += lfsr_btree_weight(&mtree_) - - lfsr_mtree_weight(lfs); } // make sure to zero so we don't end up with trailing garbage @@ -6127,29 +6140,33 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, lfs_ssize_t *rid, } // repeat the fix of our grm - if (lfs->grm_.mid == mdir->mid) { - LFS_ASSERT(lfs->grm_.rid <= mdir->rbyd.weight); - // TODO do we need this if we allow mid=0 => mroot when inlined? - // update mid if we are uninlining - lfs->grm_.mid = lfs_smax32(lfs->grm_.mid, 0); + for (uint8_t j = 0; j < 2; j++) { + if (lfs->grm_.rms[j].mid == mdir->mid) { + LFS_ASSERT(lfs->grm_.rms[j].rid <= mdir->rbyd.weight); + // TODO do we need this if we allow mid=0 => mroot + // when inlined? + // update mid if we are uninlining + lfs->grm_.rms[j].mid = lfs_smax32(lfs->grm_.rms[j].mid, 0); - if (lfs->grm_.rid >= mdir_.rbyd.weight) { - lfs->grm_.mid += 1; - lfs->grm_.rid -= mdir_.rbyd.weight; + if (lfs->grm_.rms[j].rid >= mdir_.rbyd.weight) { + lfs->grm_.rms[j].mid += 1; + lfs->grm_.rms[j].rid -= mdir_.rbyd.weight; + } + // update mid if we had a split or drop + } else if (lfs->grm_.rms[j].mid > mdir->mid + && lfsr_btree_weight(&mtree_) + != lfsr_mtree_weight(lfs)) { + lfs->grm_.rms[j].mid += lfsr_btree_weight(&mtree_) + - lfsr_mtree_weight(lfs); } - // update mid if we had a split or drop - } else if (lfs->grm_.mid > mdir->mid - && lfsr_btree_weight(&mtree_) != lfsr_mtree_weight(lfs)) { - lfs->grm_.mid += lfsr_btree_weight(&mtree_) - - lfsr_mtree_weight(lfs); - } - // TODO this is a big cludge, support for mid=0 when inlined? - // adjust mid if mtree is inlined - if (lfsr_btree_weight(&mtree_) == 0) { - LFS_ASSERT(lfs->grm_.mid <= 0); - if (lfs->grm_.mid == 0) { - lfs->grm_.mid = LFSR_MID_MROOT; + // TODO this is a big cludge, support for mid=0 when inlined? + // adjust mid if mtree is inlined + if (lfsr_btree_weight(&mtree_) == 0) { + LFS_ASSERT(lfs->grm_.rms[j].mid <= 0); + if (lfs->grm_.rms[j].mid == 0) { + lfs->grm_.rms[j].mid = LFSR_MID_MROOT; + } } } @@ -7084,22 +7101,17 @@ static int lfsr_mountinited(lfs_t *lfs) { } if (lfsr_grm_hasrm(&lfs->grm_)) { - LFS_DEBUG("Found pending grm %"PRId32".%"PRId32, - lfs->grm_.mid, - lfs->grm_.rid); + LFS_DEBUG("Found pending grm %"PRId32".%"PRId32" %"PRId32".%"PRId32, + lfs->grm_.rms[0].mid, + lfs->grm_.rms[0].rid, + lfs->grm_.rms[1].mid, + lfs->grm_.rms[1].rid); } return 0; } static int lfsr_formatinited(lfs_t *lfs) { - LFS_DEBUG("Formatting littlefs v%"PRId32".%"PRId32" " - "(bs=%"PRId32", bc=%"PRId32")", - LFS_DISK_VERSION_MAJOR, - LFS_DISK_VERSION_MINOR, - lfs->cfg->block_size, - lfs->cfg->block_count); - uint8_t buf[LFSR_SUPERCONFIG_DSIZE]; lfs_ssize_t d = lfsr_superconfig_todisk(lfs, buf); if (d < 0) { @@ -7156,6 +7168,14 @@ int lfsr_mount(lfs_t *lfs, const struct lfs_config *cfg) { return err; } + // TODO this should use any configured values + LFS_DEBUG("Mounted littlefs v%"PRId32".%"PRId32" " + "(bs=%"PRId32", bc=%"PRId32")", + LFS_DISK_VERSION_MAJOR, + LFS_DISK_VERSION_MINOR, + lfs->cfg->block_size, + lfs->cfg->block_count); + return 0; } @@ -7169,6 +7189,13 @@ int lfsr_format(lfs_t *lfs, const struct lfs_config *cfg) { return err; } + LFS_DEBUG("Formatting littlefs v%"PRId32".%"PRId32" " + "(bs=%"PRId32", bc=%"PRId32")", + LFS_DISK_VERSION_MAJOR, + LFS_DISK_VERSION_MINOR, + lfs->cfg->block_size, + lfs->cfg->block_count); + err = lfsr_formatinited(lfs); if (err) { // make sure we clean up on error @@ -7291,7 +7318,7 @@ static int lfs_alloc(lfs_t *lfs, lfs_block_t *block) { int lfsr_mkdir(lfs_t *lfs, const char *path) { // prepare our filesystem for writing - int err= lfsr_fs_preparemutation(lfs); + int err = lfsr_fs_preparemutation(lfs); if (err) { return err; } @@ -7372,7 +7399,9 @@ int lfsr_mkdir(lfs_t *lfs, const char *path) { // uint8_t buf[LFSR_GRM_DSIZE]; lfs_ssize_t d = lfsr_grm_todisk(lfs, - &(lfsr_grm_t){.mid=mdir.mid, .rid=rid}, + &(lfsr_grm_t){.rms={ + {.mid=mdir.mid, .rid=rid}, + {.mid=LFSR_MID_RM}}}, buf); if (d < 0) { return d; @@ -7407,7 +7436,7 @@ failed_with_parent: int lfsr_remove(lfs_t *lfs, const char *path) { // prepare our filesystem for writing - int err= lfsr_fs_preparemutation(lfs); + int err = lfsr_fs_preparemutation(lfs); if (err) { return err; } @@ -7418,7 +7447,7 @@ int lfsr_remove(lfs_t *lfs, const char *path) { lfsr_tag_t tag; err = lfsr_mtree_pathlookup(lfs, path, &mdir, &rid, &tag, - NULL, NULL, 0); + NULL, NULL, NULL); if (err) { return err; } @@ -7487,7 +7516,9 @@ int lfsr_remove(lfs_t *lfs, const char *path) { // create a grm to remove the dstart entry grm_d = lfsr_grm_todisk(lfs, - &(lfsr_grm_t){.mid=dstart_mdir.mid, .rid=dstart_rid}, + &(lfsr_grm_t){.rms={ + {.mid=dstart_mdir.mid, .rid=dstart_rid}, + {.mid=LFSR_MID_RM}}}, grm_buf); if (grm_d < 0) { return grm_d; @@ -7711,46 +7742,57 @@ int lfsr_dir_rewind(lfs_t *lfs, lfsr_dir_t *dir) { /// Prepare the filesystem for mutation /// static int lfsr_fs_fixgrm(lfs_t *lfs) { - LFS_ASSERT(lfsr_grm_hasrm(&lfs->grm_)); + while (lfsr_grm_hasrm(&lfs->grm_)) { + // find our mdir + lfsr_mdir_t mdir; + LFS_ASSERT(lfs->grm_.rms[0].mid < (lfs_ssize_t)lfsr_mtree_weight(lfs)); + int err = lfsr_mtree_lookup(lfs, lfs->grm_.rms[0].mid, &mdir); + if (err) { + return err; + } - // find our mdir - lfsr_mdir_t mdir; - LFS_ASSERT(lfs->grm_.mid < (lfs_ssize_t)lfsr_mtree_weight(lfs)); - int err = lfsr_mtree_lookup(lfs, lfs->grm_.mid, &mdir); - if (err) { - return err; + // mark grm as taken care of + lfsr_grm_t grm_ = lfs->grm_; + lfsr_grm_poprm(&grm_); + + uint8_t buf[LFSR_GRM_DSIZE]; + lfs_ssize_t d = lfsr_grm_todisk(lfs, &grm_, buf); + if (d < 0) { + return d; + } + + // remove the rid while also updating our grm + LFS_ASSERT(lfs->grm_.rms[0].rid < mdir.rbyd.weight); + err = lfsr_mdir_commit(lfs, &mdir, + (lfs_ssize_t*)&lfs->grm_.rms[0].rid, LFSR_ATTRS( + LFSR_ATTR(lfs->grm_.rms[0].rid, UNR, -1, NULL, 0), + LFSR_ATTR(-1, GRM, 0, buf, d))); } - // remove the rid while also zeroing our grm - LFS_ASSERT(lfs->grm_.rid < mdir.rbyd.weight); - err = lfsr_mdir_commit(lfs, &mdir, (lfs_ssize_t*)&lfs->grm_.rid, LFSR_ATTRS( - LFSR_ATTR(lfs->grm_.rid, UNR, -1, NULL, 0), - LFSR_ATTR(-1, GRM, 0, NULL, 0))); - - // mark grm as taken care of - lfsr_grm_clearrm(&lfs->grm_); return 0; } static int lfsr_fs_preparemutation(lfs_t *lfs) { + // checkpoint the allocator + lfs_alloc_ack(lfs); + // fix pending grms if (lfsr_grm_hasrm(&lfs->grm_)) { - LFS_DEBUG("Fixing grm %"PRId32".%"PRId32, - lfs->grm_.mid, - lfs->grm_.rid); - - // checkpoint the allocator in case fixing the grm causes mtree - // manipulation - lfs_alloc_ack(lfs); + LFS_DEBUG("Fixing grm %"PRId32".%"PRId32" %"PRId32".%"PRId32, + lfs->grm_.rms[0].mid, + lfs->grm_.rms[0].rid, + lfs->grm_.rms[1].mid, + lfs->grm_.rms[1].rid); int err = lfsr_fs_fixgrm(lfs); if (err) { return err; } - } - // checkpoint the allocator - lfs_alloc_ack(lfs); + // checkpoint the allocator again since our fixgrm completed some + // work + lfs_alloc_ack(lfs); + } return 0; } diff --git a/lfs.h b/lfs.h index d088a9d0..8b58f9c2 100644 --- a/lfs.h +++ b/lfs.h @@ -386,13 +386,17 @@ typedef struct lfsr_openedmdir { // space for: // - type - 1 leb128 - 1 byte (worst case) -// - mid - 1 leb128 - 5 bytes (worst case) -// - rid - 1 leb128 - 5 bytes (worst case) -#define LFSR_GRM_DSIZE (5+5) +// - mid0 - 1 leb128 - 5 bytes (worst case) +// - rid0 - 1 leb128 - 5 bytes (worst case) +// - mid1 - 1 leb128 - 5 bytes (worst case) +// - rid1 - 1 leb128 - 5 bytes (worst case) +#define LFSR_GRM_DSIZE (1+5+5+5+5) typedef struct lfsr_grm { - lfs_ssize_t mid; - lfs_size_t rid; + struct { + lfs_ssize_t mid; + lfs_size_t rid; + } rms[2]; } lfsr_grm_t; diff --git a/scripts/dbglfs.py b/scripts/dbglfs.py index c555e35c..8db34142 100755 --- a/scripts/dbglfs.py +++ b/scripts/dbglfs.py @@ -678,13 +678,18 @@ class GState: def grepr(tag, data): if tag == TAG_GRM: d = 0 - op, d_ = fromleb128(data[d:]); d += d_ - mid, d_ = fromleb128(data[d:]); d += d_ - rid, d_ = fromleb128(data[d:]); d += d_ + count, d_ = fromleb128(data[d:]); d += d_ + rms = [] + if count <= 2: + for _ in range(count): + mid, d_ = fromleb128(data[d:]); d += d_ + rid, d_ = fromleb128(data[d:]); d += d_ + rms.append((mid, rid)) return 'grm %s' % ( - 'none' if op == 0 - else 'rm %d.%d' % (mid, rid) if op == 1 - else '0x%x' % op) + 'none' if count == 0 + else ' '.join('%d.%d' % (mid, rid) for mid, rid in rms) + if count <= 2 + else '0x%x' % count) else: return 'gstate 0x%02x %d' % (tag, len(data))