From 0634d13e07d831a2bda7efeb38287dcaf06ffac2 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sat, 3 May 2025 17:15:26 -0500 Subject: [PATCH 1/2] tests: Added non-reentrant variants of orphan/relocation tests These are the same as the related reentrant variants, but by opting out of powerloss testing, we can test a much larger number of states without having to worry about the impact on powerloss testing runtime. Bumped CYCLES from 20 -> 2000. This reveals an orphan remove bug found by Hugh-Baoa. --- tests/test_orphans.toml | 69 ++++++++++++++- tests/test_relocations.toml | 168 ++++++++++++++++++++++++++++++++++++ 2 files changed, 236 insertions(+), 1 deletion(-) diff --git a/tests/test_orphans.toml b/tests/test_orphans.toml index d7040ed0..76882800 100644 --- a/tests/test_orphans.toml +++ b/tests/test_orphans.toml @@ -207,7 +207,8 @@ code = ''' [cases.test_orphans_reentrant] reentrant = true # TODO fix this case, caused by non-DAG trees -if = '!(DEPTH == 3 && CACHE_SIZE != 64)' +# NOTE the second condition is required +if = '!(DEPTH == 3 && CACHE_SIZE != 64) && 2*FILES < BLOCK_COUNT' defines = [ {FILES=6, DEPTH=1, CYCLES=20}, {FILES=26, DEPTH=1, CYCLES=20}, @@ -271,3 +272,69 @@ code = ''' lfs_unmount(&lfs) => 0; ''' +# non-reentrant testing for orphans, this is the same as reentrant +# testing, but we test way more states than we could under powerloss +[cases.test_orphans_nonreentrant] +# TODO fix this case, caused by non-DAG trees +# NOTE the second condition is required +if = '!(DEPTH == 3 && CACHE_SIZE != 64) && 2*FILES < BLOCK_COUNT' +defines = [ + {FILES=6, DEPTH=1, CYCLES=2000}, + {FILES=26, DEPTH=1, CYCLES=2000}, + {FILES=3, DEPTH=3, CYCLES=2000}, +] +code = ''' + lfs_t lfs; + lfs_format(&lfs, cfg) => 0; + lfs_mount(&lfs, cfg) => 0; + + uint32_t prng = 1; + const char alpha[] = "abcdefghijklmnopqrstuvwxyz"; + for (unsigned i = 0; i < CYCLES; i++) { + // create random path + char full_path[256]; + for (unsigned d = 0; d < DEPTH; d++) { + sprintf(&full_path[2*d], "/%c", alpha[TEST_PRNG(&prng) % FILES]); + } + + // if it does not exist, we create it, else we destroy + struct lfs_info info; + int res = lfs_stat(&lfs, full_path, &info); + if (res == LFS_ERR_NOENT) { + // create each directory in turn, ignore if dir already exists + for (unsigned d = 0; d < DEPTH; d++) { + char path[1024]; + strcpy(path, full_path); + path[2*d+2] = '\0'; + int err = lfs_mkdir(&lfs, path); + assert(!err || err == LFS_ERR_EXIST); + } + + for (unsigned d = 0; d < DEPTH; d++) { + char path[1024]; + strcpy(path, full_path); + path[2*d+2] = '\0'; + lfs_stat(&lfs, path, &info) => 0; + assert(strcmp(info.name, &path[2*d+1]) == 0); + assert(info.type == LFS_TYPE_DIR); + } + } else { + // is valid dir? + assert(strcmp(info.name, &full_path[2*(DEPTH-1)+1]) == 0); + assert(info.type == LFS_TYPE_DIR); + + // try to delete path in reverse order, ignore if dir is not empty + for (int d = DEPTH-1; d >= 0; d--) { + char path[1024]; + strcpy(path, full_path); + path[2*d+2] = '\0'; + int err = lfs_remove(&lfs, path); + assert(!err || err == LFS_ERR_NOTEMPTY); + } + + lfs_stat(&lfs, full_path, &info) => LFS_ERR_NOENT; + } + } + lfs_unmount(&lfs) => 0; +''' + diff --git a/tests/test_relocations.toml b/tests/test_relocations.toml index d20cb8cf..060e865b 100644 --- a/tests/test_relocations.toml +++ b/tests/test_relocations.toml @@ -341,3 +341,171 @@ code = ''' } lfs_unmount(&lfs) => 0; ''' + +# non-reentrant testing for orphans, this is the same as reentrant +# testing, but we test way more states than we could under powerloss +[cases.test_relocations_nonreentrant] +# TODO fix this case, caused by non-DAG trees +# NOTE the second condition is required +if = '!(DEPTH == 3 && CACHE_SIZE != 64) && 2*FILES < BLOCK_COUNT' +defines = [ + {FILES=6, DEPTH=1, CYCLES=2000, BLOCK_CYCLES=1}, + {FILES=26, DEPTH=1, CYCLES=2000, BLOCK_CYCLES=1}, + {FILES=3, DEPTH=3, CYCLES=2000, BLOCK_CYCLES=1}, +] +code = ''' + lfs_t lfs; + lfs_format(&lfs, cfg) => 0; + lfs_mount(&lfs, cfg) => 0; + + uint32_t prng = 1; + const char alpha[] = "abcdefghijklmnopqrstuvwxyz"; + for (unsigned i = 0; i < CYCLES; i++) { + // create random path + char full_path[256]; + for (unsigned d = 0; d < DEPTH; d++) { + sprintf(&full_path[2*d], "/%c", alpha[TEST_PRNG(&prng) % FILES]); + } + + // if it does not exist, we create it, else we destroy + struct lfs_info info; + int res = lfs_stat(&lfs, full_path, &info); + if (res == LFS_ERR_NOENT) { + // create each directory in turn, ignore if dir already exists + for (unsigned d = 0; d < DEPTH; d++) { + char path[1024]; + strcpy(path, full_path); + path[2*d+2] = '\0'; + int err = lfs_mkdir(&lfs, path); + assert(!err || err == LFS_ERR_EXIST); + } + + for (unsigned d = 0; d < DEPTH; d++) { + char path[1024]; + strcpy(path, full_path); + path[2*d+2] = '\0'; + lfs_stat(&lfs, path, &info) => 0; + assert(strcmp(info.name, &path[2*d+1]) == 0); + assert(info.type == LFS_TYPE_DIR); + } + } else { + // is valid dir? + assert(strcmp(info.name, &full_path[2*(DEPTH-1)+1]) == 0); + assert(info.type == LFS_TYPE_DIR); + + // try to delete path in reverse order, ignore if dir is not empty + for (unsigned d = DEPTH-1; d+1 > 0; d--) { + char path[1024]; + strcpy(path, full_path); + path[2*d+2] = '\0'; + int err = lfs_remove(&lfs, path); + assert(!err || err == LFS_ERR_NOTEMPTY); + } + + lfs_stat(&lfs, full_path, &info) => LFS_ERR_NOENT; + } + } + lfs_unmount(&lfs) => 0; +''' + +# non-reentrant testing for relocations, but now with random renames! +[cases.test_relocations_nonreentrant_renames] +# TODO fix this case, caused by non-DAG trees +# NOTE the second condition is required +if = '!(DEPTH == 3 && CACHE_SIZE != 64) && 2*FILES < BLOCK_COUNT' +defines = [ + {FILES=6, DEPTH=1, CYCLES=2000, BLOCK_CYCLES=1}, + {FILES=26, DEPTH=1, CYCLES=2000, BLOCK_CYCLES=1}, + {FILES=3, DEPTH=3, CYCLES=2000, BLOCK_CYCLES=1}, +] +code = ''' + lfs_t lfs; + lfs_format(&lfs, cfg) => 0; + lfs_mount(&lfs, cfg) => 0; + + uint32_t prng = 1; + const char alpha[] = "abcdefghijklmnopqrstuvwxyz"; + for (unsigned i = 0; i < CYCLES; i++) { + // create random path + char full_path[256]; + for (unsigned d = 0; d < DEPTH; d++) { + sprintf(&full_path[2*d], "/%c", alpha[TEST_PRNG(&prng) % FILES]); + } + + // if it does not exist, we create it, else we destroy + struct lfs_info info; + int res = lfs_stat(&lfs, full_path, &info); + assert(!res || res == LFS_ERR_NOENT); + if (res == LFS_ERR_NOENT) { + // create each directory in turn, ignore if dir already exists + for (unsigned d = 0; d < DEPTH; d++) { + char path[1024]; + strcpy(path, full_path); + path[2*d+2] = '\0'; + int err = lfs_mkdir(&lfs, path); + assert(!err || err == LFS_ERR_EXIST); + } + + for (unsigned d = 0; d < DEPTH; d++) { + char path[1024]; + strcpy(path, full_path); + path[2*d+2] = '\0'; + lfs_stat(&lfs, path, &info) => 0; + assert(strcmp(info.name, &path[2*d+1]) == 0); + assert(info.type == LFS_TYPE_DIR); + } + } else { + assert(strcmp(info.name, &full_path[2*(DEPTH-1)+1]) == 0); + assert(info.type == LFS_TYPE_DIR); + + // create new random path + char new_path[256]; + for (unsigned d = 0; d < DEPTH; d++) { + sprintf(&new_path[2*d], "/%c", alpha[TEST_PRNG(&prng) % FILES]); + } + + // if new path does not exist, rename, otherwise destroy + res = lfs_stat(&lfs, new_path, &info); + assert(!res || res == LFS_ERR_NOENT); + if (res == LFS_ERR_NOENT) { + // stop once some dir is renamed + for (unsigned d = 0; d < DEPTH; d++) { + char path[1024]; + strcpy(&path[2*d], &full_path[2*d]); + path[2*d+2] = '\0'; + strcpy(&path[128+2*d], &new_path[2*d]); + path[128+2*d+2] = '\0'; + int err = lfs_rename(&lfs, path, path+128); + assert(!err || err == LFS_ERR_NOTEMPTY); + if (!err) { + strcpy(path, path+128); + } + } + + for (unsigned d = 0; d < DEPTH; d++) { + char path[1024]; + strcpy(path, new_path); + path[2*d+2] = '\0'; + lfs_stat(&lfs, path, &info) => 0; + assert(strcmp(info.name, &path[2*d+1]) == 0); + assert(info.type == LFS_TYPE_DIR); + } + + lfs_stat(&lfs, full_path, &info) => LFS_ERR_NOENT; + } else { + // try to delete path in reverse order, + // ignore if dir is not empty + for (unsigned d = DEPTH-1; d+1 > 0; d--) { + char path[1024]; + strcpy(path, full_path); + path[2*d+2] = '\0'; + int err = lfs_remove(&lfs, path); + assert(!err || err == LFS_ERR_NOTEMPTY); + } + + lfs_stat(&lfs, full_path, &info) => LFS_ERR_NOENT; + } + } + } + lfs_unmount(&lfs) => 0; +''' From a3d6bec5f0e90a5a5b96cccab21fd1bf8fb97f17 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sat, 3 May 2025 17:20:02 -0500 Subject: [PATCH 2/2] Fixed a double deorphan caused by relocation mid dir remove Long story short: There is a specific case where removing a directory can trigger a deorphan pass, but lfs_remove did not check for this, would try to clean up the (already cleaned) directory orphan, and trigger an assert: lfs.c:4890:assert: assert failed with false, expected eq true LFS_ASSERT(lfs_tag_size(lfs->gstate.tag) > 0x000 || orphans >= 0); The specific case being a remove commit that triggers a relocation that creates an orphan. This is also possible in lfs_rename, but only if you're renaming a directory that implies a remove, which is a pretty rare operation. --- This was probably an oversight introduced in the non-recursive commit logic rework. Fortunately the fix is to just check if we even have an orphan before trying to remove it. We can rely on this instead of the file type, so this fix shouldn't even increase the code size. Found and root-caused by Hugh-Baoa --- lfs.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lfs.c b/lfs.c index 28a632b6..05b1ca10 100644 --- a/lfs.c +++ b/lfs.c @@ -3932,7 +3932,9 @@ static int lfs_remove_(lfs_t *lfs, const char *path) { } lfs->mlist = dir.next; - if (lfs_tag_type3(tag) == LFS_TYPE_DIR) { + if (lfs_gstate_hasorphans(&lfs->gstate)) { + LFS_ASSERT(lfs_tag_type3(tag) == LFS_TYPE_DIR); + // fix orphan err = lfs_fs_preporphans(lfs, -1); if (err) { @@ -4076,8 +4078,10 @@ static int lfs_rename_(lfs_t *lfs, const char *oldpath, const char *newpath) { } lfs->mlist = prevdir.next; - if (prevtag != LFS_ERR_NOENT - && lfs_tag_type3(prevtag) == LFS_TYPE_DIR) { + if (lfs_gstate_hasorphans(&lfs->gstate)) { + LFS_ASSERT(prevtag != LFS_ERR_NOENT + && lfs_tag_type3(prevtag) == LFS_TYPE_DIR); + // fix orphan err = lfs_fs_preporphans(lfs, -1); if (err) {