From cb1319c9e64c22043b38f93295c7c2b92be89fba Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Fri, 7 Jul 2023 16:31:52 -0500 Subject: [PATCH] Expanded fuzz testing a bit, found/fixed an mid neighbor update bug This bug was just overlooked in testing the mtree, fortunately dir fuzzing found it. Though since this depends on neighboring mdirs, it probably would have been found quicker with smaller block sizes. At the moment I am only testing on NOR-liked geometry (4KiB blocks). The fix is easy, we can use the difference in the mtree size to determine if a split or drop happened in mdir commit, since at most one of these can happen on any mdir commit. Also added an explicit test for mid updates when splitting and dropping. --- lfs.c | 31 +++++-- tests/t3_mtree.toml | 217 +++++++++++++++++++++++++++++++++++++++++--- tests/t4_alloc.toml | 2 +- tests/t5_dirs.toml | 16 +++- 4 files changed, 237 insertions(+), 29 deletions(-) diff --git a/lfs.c b/lfs.c index 4d687d78..001241c6 100644 --- a/lfs.c +++ b/lfs.c @@ -4966,14 +4966,14 @@ static inline int lfsr_mtree_isinlined(lfs_t *lfs) { return lfsr_btree_weight(&lfs->mtree) == 0; } -static inline lfs_ssize_t lfsr_mtree_weight(lfs_t *lfs) { +static inline lfs_size_t lfsr_mtree_weight(lfs_t *lfs) { return lfsr_btree_weight(&lfs->mtree); } static int lfsr_mtree_lookup(lfs_t *lfs, lfs_ssize_t mid, lfsr_mdir_t *mdir_) { // TODO should we really allow -1=>mroot lookup? LFS_ASSERT(mid >= -1); - LFS_ASSERT(mid < lfsr_mtree_weight(lfs)); + LFS_ASSERT(mid < (lfs_ssize_t)lfsr_mtree_weight(lfs)); // looking up mroot? if (mid < 0) { @@ -5649,17 +5649,18 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, lfs_ssize_t *rid, } // success?? update in-device state - lfs->mroot = mroot_; - lfs->mtree = mtree_; // update any opened mdirs for (lfsr_openedmdir_t *opened = lfs->opened; opened; opened = opened->next) { - if (opened->mdir.mid == mdir->mid - // avoid double-updating our current mdir - && &opened->mdir != mdir) { - LFS_ASSERT(opened->rid <= (lfs_ssize_t)opened->mdir.rbyd.weight); + // avoid double-updating our current mdir + if (&opened->mdir == mdir) { + continue; + } + + if (opened->mdir.mid == mdir->mid) { + LFS_ASSERT(opened->rid < (lfs_ssize_t)opened->mdir.rbyd.weight); LFS_ASSERT(opened->rid != -1); // first play out any attrs that change our rid @@ -5681,12 +5682,24 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, lfs_ssize_t *rid, } else if ((lfs_size_t)opened->rid < mdir_.rbyd.weight) { opened->mdir = mdir_; } else { + LFS_ASSERT(lfsr_btree_weight(&mtree_) + != lfsr_mtree_weight(lfs)); opened->rid = opened->rid - mdir_.rbyd.weight; opened->mdir = msibling_; } + + // update mid if we had a split or drop + } else if (opened->mdir.mid > mdir->mid + && lfsr_btree_weight(&mtree_) != lfsr_mtree_weight(lfs)) { + opened->mdir.mid += lfsr_btree_weight(&mtree_) + - lfsr_mtree_weight(lfs); } } + // update our mroot and mtree + lfs->mroot = mroot_; + lfs->mtree = mtree_; + // update mdir to follow requested rid lfs_ssize_t rid_ = *rid; LFS_ASSERT(rid_ <= (lfs_ssize_t)mdir->rbyd.weight); @@ -6904,7 +6917,7 @@ int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) { if (rid >= (lfs_ssize_t)dir->mdir.mdir.rbyd.weight) { // out of mdirs? lfs_ssize_t mid = dir->mdir.mdir.mid + 1; - if (mid >= lfsr_mtree_weight(lfs)) { + if (mid >= (lfs_ssize_t)lfsr_mtree_weight(lfs)) { return LFS_ERR_NOENT; } diff --git a/tests/t3_mtree.toml b/tests/t3_mtree.toml index 3c8ec639..7d4bcf0a 100644 --- a/tests/t3_mtree.toml +++ b/tests/t3_mtree.toml @@ -426,7 +426,7 @@ code = ''' // try looking up each entry lfs_size_t i = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -451,7 +451,7 @@ code = ''' // try looking up each entry i = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -535,7 +535,7 @@ code = ''' lfs_size_t count_ = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -564,7 +564,7 @@ code = ''' count_ = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -1306,7 +1306,7 @@ code = ''' // try looking up each entry lfs_size_t i = N - REMAINING; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -1331,7 +1331,7 @@ code = ''' // try looking up each entry i = N - REMAINING; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -1387,7 +1387,7 @@ code = ''' // try looking up each entry lfs_size_t i = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -1517,7 +1517,7 @@ code = ''' lfs_size_t count_ = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -1550,7 +1550,7 @@ code = ''' count_ = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -2629,7 +2629,7 @@ code = ''' lfs_size_t count_ = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -2662,7 +2662,7 @@ code = ''' count_ = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -3224,6 +3224,193 @@ code = ''' lfsr_unmount(&lfs) => 0; ''' +[cases.t3_mtree_neighbor_mid_split] +# this should be set so only one entry can fit in a metadata block +defines.SIZE = 'BLOCK_SIZE / 4' +# make it so blocks relocate every two compacts +defines.BLOCK_CYCLES = 2 +in = 'lfs.c' +code = ''' + const char *alphas = "abcdefghijklmnopqrstuvwxyz"; + lfs_t lfs; + lfsr_format(&lfs, cfg) => 0; + lfsr_mount(&lfs, cfg) => 0; + lfs_alloc_ack(&lfs); + // remove root dstart for now + lfsr_mdir_commit(&lfs, &lfs.mroot, &(lfs_ssize_t){-1}, LFSR_ATTRS( + LFSR_ATTR(0, UNR, -1, NULL, 0))) => 0; + + //// create a situation where we have 3 mdirs in our tree + + // first force mroot to uninlined+split + uint8_t buffer[SIZE]; + memset(buffer, alphas[0 % 26], SIZE); + lfsr_mdir_commit(&lfs, &lfs.mroot, &(lfs_ssize_t){-1}, LFSR_ATTRS( + LFSR_ATTR(0, INLINED, +1, buffer, SIZE))) => 0; + memset(buffer, alphas[1 % 26], SIZE); + lfsr_mdir_commit(&lfs, &lfs.mroot, &(lfs_ssize_t){-1}, LFSR_ATTRS( + LFSR_ATTR(1, INLINED, +1, buffer, SIZE))) => 0; + + // force mroot to compact + lfs.mroot.rbyd.off = BLOCK_SIZE; + lfsr_mdir_commit(&lfs, &lfs.mroot, &(lfs_ssize_t){-1}, NULL, 0) => 0; + + // we should now have 2 mdirs + assert(lfsr_mtree_weight(&lfs) == 2); + + // now force one of our siblings to split + lfsr_mdir_t mdir; + lfsr_mtree_lookup(&lfs, 1, &mdir) => 0; + assert(mdir.rbyd.weight == 1); + memset(buffer, alphas[2 % 26], SIZE); + lfsr_mdir_commit(&lfs, &mdir, &(lfs_ssize_t){1}, LFSR_ATTRS( + LFSR_ATTR(1, INLINED, +1, buffer, SIZE))) => 0; + + // force mdir to compact + mdir.rbyd.off = BLOCK_SIZE; + lfsr_mdir_commit(&lfs, &mdir, &(lfs_ssize_t){-1}, NULL, 0) => 0; + + // we should now have 3 mdirs + assert(lfsr_mtree_weight(&lfs) == 3); + + + //// Now test splitting updates mids correctly + + // setup our neighbors + lfsr_openedmdir_t left_neighbor; + lfsr_mtree_lookup(&lfs, 0, &left_neighbor.mdir) => 0; + assert(left_neighbor.mdir.rbyd.weight == 1); + left_neighbor.rid = 0; + + lfsr_openedmdir_t right_neighbor; + lfsr_mtree_lookup(&lfs, 2, &right_neighbor.mdir) => 0; + assert(right_neighbor.mdir.rbyd.weight == 1); + right_neighbor.rid = 0; + + lfsr_mdir_addopened(&lfs, &left_neighbor); + lfsr_mdir_addopened(&lfs, &right_neighbor); + + // cause middle mdir to split + lfsr_mtree_lookup(&lfs, 1, &mdir) => 0; + assert(mdir.rbyd.weight == 1); + memset(buffer, alphas[3 % 26], SIZE); + lfsr_mdir_commit(&lfs, &mdir, &(lfs_ssize_t){1}, LFSR_ATTRS( + LFSR_ATTR(1, INLINED, +1, buffer, SIZE))) => 0; + + // force mdir to compact + mdir.rbyd.off = BLOCK_SIZE; + lfsr_mdir_commit(&lfs, &mdir, &(lfs_ssize_t){-1}, NULL, 0) => 0; + + // we should now have 4 mdirs + assert(lfsr_mtree_weight(&lfs) == 4); + + // assert that our neighbors were updated correctly + assert(left_neighbor.rid == 0); + assert(left_neighbor.mdir.mid == 0); + lfsr_mtree_lookup(&lfs, 0, &mdir) => 0; + assert(memcmp(&left_neighbor.mdir, &mdir, sizeof(lfsr_mdir_t)) == 0); + + assert(right_neighbor.rid == 0); + assert(right_neighbor.mdir.mid == 3); + lfsr_mtree_lookup(&lfs, 3, &mdir) => 0; + assert(memcmp(&right_neighbor.mdir, &mdir, sizeof(lfsr_mdir_t)) == 0); + + lfsr_mdir_removeopened(&lfs, &left_neighbor); + lfsr_mdir_removeopened(&lfs, &right_neighbor); + lfsr_unmount(&lfs) => 0; +''' + +[cases.t3_mtree_neighbor_mid_drop] +# this should be set so only one entry can fit in a metadata block +defines.SIZE = 'BLOCK_SIZE / 4' +# make it so blocks relocate every two compacts +defines.BLOCK_CYCLES = 2 +in = 'lfs.c' +code = ''' + const char *alphas = "abcdefghijklmnopqrstuvwxyz"; + lfs_t lfs; + lfsr_format(&lfs, cfg) => 0; + lfsr_mount(&lfs, cfg) => 0; + lfs_alloc_ack(&lfs); + // remove root dstart for now + lfsr_mdir_commit(&lfs, &lfs.mroot, &(lfs_ssize_t){-1}, LFSR_ATTRS( + LFSR_ATTR(0, UNR, -1, NULL, 0))) => 0; + + //// create a situation where we have 3 mdirs in our tree + + // first force mroot to uninlined+split + uint8_t buffer[SIZE]; + memset(buffer, alphas[0 % 26], SIZE); + lfsr_mdir_commit(&lfs, &lfs.mroot, &(lfs_ssize_t){-1}, LFSR_ATTRS( + LFSR_ATTR(0, INLINED, +1, buffer, SIZE))) => 0; + memset(buffer, alphas[1 % 26], SIZE); + lfsr_mdir_commit(&lfs, &lfs.mroot, &(lfs_ssize_t){-1}, LFSR_ATTRS( + LFSR_ATTR(1, INLINED, +1, buffer, SIZE))) => 0; + + // force mroot to compact + lfs.mroot.rbyd.off = BLOCK_SIZE; + lfsr_mdir_commit(&lfs, &lfs.mroot, &(lfs_ssize_t){-1}, NULL, 0) => 0; + + // we should now have 2 mdirs + assert(lfsr_mtree_weight(&lfs) == 2); + + // now force one of our siblings to split + lfsr_mdir_t mdir; + lfsr_mtree_lookup(&lfs, 1, &mdir) => 0; + assert(mdir.rbyd.weight == 1); + memset(buffer, alphas[2 % 26], SIZE); + lfsr_mdir_commit(&lfs, &mdir, &(lfs_ssize_t){1}, LFSR_ATTRS( + LFSR_ATTR(1, INLINED, +1, buffer, SIZE))) => 0; + + // force mdir to compact + mdir.rbyd.off = BLOCK_SIZE; + lfsr_mdir_commit(&lfs, &mdir, &(lfs_ssize_t){-1}, NULL, 0) => 0; + + // we should now have 3 mdirs + assert(lfsr_mtree_weight(&lfs) == 3); + + + //// Now test dropping updates mids correctly + + // setup our neighbors + lfsr_openedmdir_t left_neighbor; + lfsr_mtree_lookup(&lfs, 0, &left_neighbor.mdir) => 0; + assert(left_neighbor.mdir.rbyd.weight == 1); + left_neighbor.rid = 0; + + lfsr_openedmdir_t right_neighbor; + lfsr_mtree_lookup(&lfs, 2, &right_neighbor.mdir) => 0; + assert(right_neighbor.mdir.rbyd.weight == 1); + right_neighbor.rid = 0; + + lfsr_mdir_addopened(&lfs, &left_neighbor); + lfsr_mdir_addopened(&lfs, &right_neighbor); + + // cause middle mdir to drop + lfsr_mtree_lookup(&lfs, 1, &mdir) => 0; + assert(mdir.rbyd.weight == 1); + lfsr_mdir_commit(&lfs, &mdir, &(lfs_ssize_t){0}, LFSR_ATTRS( + LFSR_ATTR(0, UNR, -1, NULL, 0))) => 0; + + // we should now have 2 mdirs + assert(lfsr_mtree_weight(&lfs) == 2); + + // assert that our neighbors were updated correctly + assert(left_neighbor.rid == 0); + assert(left_neighbor.mdir.mid == 0); + lfsr_mtree_lookup(&lfs, 0, &mdir) => 0; + assert(memcmp(&left_neighbor.mdir, &mdir, sizeof(lfsr_mdir_t)) == 0); + + assert(right_neighbor.rid == 0); + assert(right_neighbor.mdir.mid == 1); + lfsr_mtree_lookup(&lfs, 1, &mdir) => 0; + assert(memcmp(&right_neighbor.mdir, &mdir, sizeof(lfsr_mdir_t)) == 0); + + lfsr_mdir_removeopened(&lfs, &left_neighbor); + lfsr_mdir_removeopened(&lfs, &right_neighbor); + lfsr_unmount(&lfs) => 0; +''' + ## mtree traversal ## @@ -3749,7 +3936,7 @@ code = ''' // try looking up each entry lfs_size_t i = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -3831,7 +4018,7 @@ code = ''' // try looking up each entry i = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -3915,7 +4102,7 @@ code = ''' lfs_size_t count_ = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; @@ -4002,7 +4189,7 @@ code = ''' count_ = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; diff --git a/tests/t4_alloc.toml b/tests/t4_alloc.toml index 18c0acc1..4eaf0846 100644 --- a/tests/t4_alloc.toml +++ b/tests/t4_alloc.toml @@ -142,7 +142,7 @@ code = ''' // test that all of our metadata entries are still there lfs_size_t i = 0; for (lfs_ssize_t mid = (lfsr_mtree_isinlined(&lfs) ? -1 : 0); - mid < lfsr_mtree_weight(&lfs); + mid < (lfs_ssize_t)lfsr_mtree_weight(&lfs); mid++) { lfsr_mdir_t mdir; lfsr_mtree_lookup(&lfs, mid, &mdir) => 0; diff --git a/tests/t5_dirs.toml b/tests/t5_dirs.toml index 5d161be2..22d91e64 100644 --- a/tests/t5_dirs.toml +++ b/tests/t5_dirs.toml @@ -547,6 +547,9 @@ code = ''' [cases.t5_dirs_mkdir_fuzz] defines.N = [1, 2, 4, 8, 16, 32, 64, 128, 256, 512] +# 0 => do this test in the root dir +# 1 => do this test in a directory named "parent" +defines.PARENT = [0, 1] defines.SAMPLES = 10 # -1 => all pseudo-random seeds # n => reproduce a specific seed @@ -561,6 +564,9 @@ code = ''' lfs_t lfs; lfsr_format(&lfs, cfg) => 0; lfsr_mount(&lfs, cfg) => 0; + if (PARENT) { + lfsr_mkdir(&lfs, "parent") => 0; + } // set up a simulation to compare against lfs_size_t *sim = malloc(N*sizeof(lfs_size_t)); @@ -590,7 +596,7 @@ code = ''' // create a directory here char name[256]; - sprintf(name, "dir%04d", x); + sprintf(name, "%s/dir%04d", (PARENT ? "parent" : ""), x); lfsr_mkdir(&lfs, name) => 0; next:; } @@ -598,15 +604,17 @@ code = ''' // test that our directories match our simulation for (lfs_size_t j = 0; j < sim_size; j++) { char name[256]; - sprintf(name, "dir%04d", sim[j]); + sprintf(name, "%s/dir%04d", (PARENT ? "parent" : ""), sim[j]); struct lfs_info info; lfsr_stat(&lfs, name, &info) => 0; - assert(strcmp(info.name, name) == 0); + char name2[256]; + sprintf(name2, "dir%04d", sim[j]); + assert(strcmp(info.name, name2) == 0); assert(info.type == LFS_TYPE_DIR); } lfsr_dir_t dir; - lfsr_dir_open(&lfs, &dir, "/") => 0; + lfsr_dir_open(&lfs, &dir, (PARENT ? "parent" : "/")) => 0; struct lfs_info info; lfsr_dir_read(&lfs, &dir, &info) => 0; assert(strcmp(info.name, ".") == 0);