From 1f82c0f27f8098f229106a95b09e6a97d0a24944 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Thu, 3 Oct 2024 16:34:45 -0500 Subject: [PATCH 1/3] Added some metadata_max testing - Added METADATA_MAX to test_runner. - Added METADATA_MAX to bench_runner. - Added a simple metadata_max test to test_superblocks, for lack of better location. There have been several issues floating around related to metadata_max and LFS_ERR_NOSPC which makes me think there's a bug in our metadata_max logic. metadata_max was a quick patch and is relatively untested, so an undetected bug isn't too surprising. This commit adds at least some testing over metadata_max. Sure enough, the new test_superblocks_metadata_max test reveals a curious LFS_ERR_NAMETOOLONG error that shouldn't be there. More investigation needed. --- runners/bench_runner.c | 1 + runners/bench_runner.h | 17 ++++++++++------- runners/test_runner.c | 5 +++++ runners/test_runner.h | 19 +++++++++++-------- tests/test_superblocks.toml | 27 +++++++++++++++++++++++++++ 5 files changed, 54 insertions(+), 15 deletions(-) diff --git a/runners/bench_runner.c b/runners/bench_runner.c index 387889d1..d49f9761 100644 --- a/runners/bench_runner.c +++ b/runners/bench_runner.c @@ -1322,6 +1322,7 @@ void perm_run( .cache_size = CACHE_SIZE, .lookahead_size = LOOKAHEAD_SIZE, .compact_thresh = COMPACT_THRESH, + .metadata_max = METADATA_MAX, .inline_max = INLINE_MAX, }; diff --git a/runners/bench_runner.h b/runners/bench_runner.h index 174733c1..848b5e8f 100644 --- a/runners/bench_runner.h +++ b/runners/bench_runner.h @@ -96,12 +96,13 @@ intmax_t bench_define(size_t define); #define CACHE_SIZE_i 6 #define LOOKAHEAD_SIZE_i 7 #define COMPACT_THRESH_i 8 -#define INLINE_MAX_i 9 -#define BLOCK_CYCLES_i 10 -#define ERASE_VALUE_i 11 -#define ERASE_CYCLES_i 12 -#define BADBLOCK_BEHAVIOR_i 13 -#define POWERLOSS_BEHAVIOR_i 14 +#define METADATA_MAX_i 9 +#define INLINE_MAX_i 10 +#define BLOCK_CYCLES_i 11 +#define ERASE_VALUE_i 12 +#define ERASE_CYCLES_i 13 +#define BADBLOCK_BEHAVIOR_i 14 +#define POWERLOSS_BEHAVIOR_i 15 #define READ_SIZE bench_define(READ_SIZE_i) #define PROG_SIZE bench_define(PROG_SIZE_i) @@ -112,6 +113,7 @@ intmax_t bench_define(size_t define); #define CACHE_SIZE bench_define(CACHE_SIZE_i) #define LOOKAHEAD_SIZE bench_define(LOOKAHEAD_SIZE_i) #define COMPACT_THRESH bench_define(COMPACT_THRESH_i) +#define METADATA_MAX bench_define(METADATA_MAX_i) #define INLINE_MAX bench_define(INLINE_MAX_i) #define BLOCK_CYCLES bench_define(BLOCK_CYCLES_i) #define ERASE_VALUE bench_define(ERASE_VALUE_i) @@ -129,6 +131,7 @@ intmax_t bench_define(size_t define); BENCH_DEF(CACHE_SIZE, lfs_max(64,lfs_max(READ_SIZE,PROG_SIZE))) \ BENCH_DEF(LOOKAHEAD_SIZE, 16) \ BENCH_DEF(COMPACT_THRESH, 0) \ + BENCH_DEF(METADATA_MAX, 0) \ BENCH_DEF(INLINE_MAX, 0) \ BENCH_DEF(BLOCK_CYCLES, -1) \ BENCH_DEF(ERASE_VALUE, 0xff) \ @@ -137,7 +140,7 @@ intmax_t bench_define(size_t define); BENCH_DEF(POWERLOSS_BEHAVIOR, LFS_EMUBD_POWERLOSS_NOOP) #define BENCH_GEOMETRY_DEFINE_COUNT 4 -#define BENCH_IMPLICIT_DEFINE_COUNT 15 +#define BENCH_IMPLICIT_DEFINE_COUNT 16 #endif diff --git a/runners/test_runner.c b/runners/test_runner.c index ff526730..37cd1e7d 100644 --- a/runners/test_runner.c +++ b/runners/test_runner.c @@ -1347,6 +1347,7 @@ static void run_powerloss_none( .cache_size = CACHE_SIZE, .lookahead_size = LOOKAHEAD_SIZE, .compact_thresh = COMPACT_THRESH, + .metadata_max = METADATA_MAX, .inline_max = INLINE_MAX, #ifdef LFS_MULTIVERSION .disk_version = DISK_VERSION, @@ -1425,6 +1426,7 @@ static void run_powerloss_linear( .cache_size = CACHE_SIZE, .lookahead_size = LOOKAHEAD_SIZE, .compact_thresh = COMPACT_THRESH, + .metadata_max = METADATA_MAX, .inline_max = INLINE_MAX, #ifdef LFS_MULTIVERSION .disk_version = DISK_VERSION, @@ -1520,6 +1522,7 @@ static void run_powerloss_log( .cache_size = CACHE_SIZE, .lookahead_size = LOOKAHEAD_SIZE, .compact_thresh = COMPACT_THRESH, + .metadata_max = METADATA_MAX, .inline_max = INLINE_MAX, #ifdef LFS_MULTIVERSION .disk_version = DISK_VERSION, @@ -1613,6 +1616,7 @@ static void run_powerloss_cycles( .cache_size = CACHE_SIZE, .lookahead_size = LOOKAHEAD_SIZE, .compact_thresh = COMPACT_THRESH, + .metadata_max = METADATA_MAX, .inline_max = INLINE_MAX, #ifdef LFS_MULTIVERSION .disk_version = DISK_VERSION, @@ -1804,6 +1808,7 @@ static void run_powerloss_exhaustive( .cache_size = CACHE_SIZE, .lookahead_size = LOOKAHEAD_SIZE, .compact_thresh = COMPACT_THRESH, + .metadata_max = METADATA_MAX, .inline_max = INLINE_MAX, #ifdef LFS_MULTIVERSION .disk_version = DISK_VERSION, diff --git a/runners/test_runner.h b/runners/test_runner.h index 0f0e594e..ecdf9c11 100644 --- a/runners/test_runner.h +++ b/runners/test_runner.h @@ -89,13 +89,14 @@ intmax_t test_define(size_t define); #define CACHE_SIZE_i 6 #define LOOKAHEAD_SIZE_i 7 #define COMPACT_THRESH_i 8 -#define INLINE_MAX_i 9 -#define BLOCK_CYCLES_i 10 -#define ERASE_VALUE_i 11 -#define ERASE_CYCLES_i 12 -#define BADBLOCK_BEHAVIOR_i 13 -#define POWERLOSS_BEHAVIOR_i 14 -#define DISK_VERSION_i 15 +#define METADATA_MAX_i 9 +#define INLINE_MAX_i 10 +#define BLOCK_CYCLES_i 11 +#define ERASE_VALUE_i 12 +#define ERASE_CYCLES_i 13 +#define BADBLOCK_BEHAVIOR_i 14 +#define POWERLOSS_BEHAVIOR_i 15 +#define DISK_VERSION_i 16 #define READ_SIZE TEST_DEFINE(READ_SIZE_i) #define PROG_SIZE TEST_DEFINE(PROG_SIZE_i) @@ -106,6 +107,7 @@ intmax_t test_define(size_t define); #define CACHE_SIZE TEST_DEFINE(CACHE_SIZE_i) #define LOOKAHEAD_SIZE TEST_DEFINE(LOOKAHEAD_SIZE_i) #define COMPACT_THRESH TEST_DEFINE(COMPACT_THRESH_i) +#define METADATA_MAX TEST_DEFINE(METADATA_MAX_i) #define INLINE_MAX TEST_DEFINE(INLINE_MAX_i) #define BLOCK_CYCLES TEST_DEFINE(BLOCK_CYCLES_i) #define ERASE_VALUE TEST_DEFINE(ERASE_VALUE_i) @@ -124,6 +126,7 @@ intmax_t test_define(size_t define); TEST_DEF(CACHE_SIZE, lfs_max(64,lfs_max(READ_SIZE,PROG_SIZE))) \ TEST_DEF(LOOKAHEAD_SIZE, 16) \ TEST_DEF(COMPACT_THRESH, 0) \ + TEST_DEF(METADATA_MAX, 0) \ TEST_DEF(INLINE_MAX, 0) \ TEST_DEF(BLOCK_CYCLES, -1) \ TEST_DEF(ERASE_VALUE, 0xff) \ @@ -133,7 +136,7 @@ intmax_t test_define(size_t define); TEST_DEF(DISK_VERSION, 0) #define TEST_GEOMETRY_DEFINE_COUNT 4 -#define TEST_IMPLICIT_DEFINE_COUNT 16 +#define TEST_IMPLICIT_DEFINE_COUNT 17 #endif diff --git a/tests/test_superblocks.toml b/tests/test_superblocks.toml index e93f02eb..5911c9b0 100644 --- a/tests/test_superblocks.toml +++ b/tests/test_superblocks.toml @@ -523,3 +523,30 @@ code = ''' assert(memcmp(buffer, "hello!", 6) == 0); lfs_unmount(&lfs) => 0; ''' + +# test that metadata_max does not cause problems for superblock compaction +[cases.test_superblocks_metadata_max] +defines.METADATA_MAX = [ + 'lfs_max(512, PROG_SIZE)', + 'lfs_max(BLOCK_SIZE/2, PROG_SIZE)', + 'BLOCK_SIZE' +] +defines.N = [10, 100, 1000] +code = ''' + lfs_t lfs; + lfs_format(&lfs, cfg) => 0; + lfs_mount(&lfs, cfg) => 0; + for (int i = 0; i < N; i++) { + lfs_file_t file; + char name[256]; + sprintf(name, "hello%03x", i); + lfs_file_open(&lfs, &file, name, + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; + lfs_file_close(&lfs, &file) => 0; + struct lfs_info info; + lfs_stat(&lfs, name, &info) => 0; + assert(strcmp(info.name, name) == 0); + assert(info.type == LFS_TYPE_REG); + } + lfs_unmount(&lfs) => 0; +''' From 2d62d2f4c9f4743e090ce503fd5d4f9eb104da70 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Fri, 4 Oct 2024 13:19:40 -0500 Subject: [PATCH 2/3] Fixed metadata_max==prog_size commit->end calculation The inconsistency here between the use of block_size vs metadata_max was suspicious. Turns out there's a bug when metadata_max == prog_size. We correctly use metadata_max for the block_size/2 check, but we weren't using it for the block_size-40 check. The second check seems unnecessary after the first, but it protects against running out of space in a commit for commit-related metadata (checksums, tail pointers, etc) when we can't program half-blocks. Turns out this is also needed when limiting metadata_max to a single prog, otherwise we risk erroring with LFS_ERR_NOSPC early. Found by ajheck, dpkristensen, NLLK, and likely others. --- lfs.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lfs.c b/lfs.c index d35d5d6d..1f8c9c65 100644 --- a/lfs.c +++ b/lfs.c @@ -2128,13 +2128,14 @@ static int lfs_dir_splittingcompact(lfs_t *lfs, lfs_mdir_t *dir, // And we cap at half a block to avoid degenerate cases with // nearly-full metadata blocks. // + lfs_size_t metadata_max = (lfs->cfg->metadata_max) + ? lfs->cfg->metadata_max + : lfs->cfg->block_size; if (end - split < 0xff && size <= lfs_min( - lfs->cfg->block_size - 40, + metadata_max - 40, lfs_alignup( - (lfs->cfg->metadata_max - ? lfs->cfg->metadata_max - : lfs->cfg->block_size)/2, + metadata_max/2, lfs->cfg->prog_size))) { break; } From ea431bd6aefd6f4f2a00b1b90bb15bcb0c9a411d Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Fri, 4 Oct 2024 13:29:03 -0500 Subject: [PATCH 3/3] Added some checks that metadata_max makes sense Like the read/prog/block_size checks, these are just asserts. If these invariants are broken the filesystem will break in surprising ways. --- lfs.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lfs.c b/lfs.c index 1f8c9c65..40daf759 100644 --- a/lfs.c +++ b/lfs.c @@ -4210,6 +4210,15 @@ static int lfs_init(lfs_t *lfs, const struct lfs_config *cfg) { LFS_ASSERT(lfs->cfg->compact_thresh == (lfs_size_t)-1 || lfs->cfg->compact_thresh <= lfs->cfg->block_size); + // check that metadata_max is a multiple of read_size and prog_size, + // and a factor of the block_size + LFS_ASSERT(!lfs->cfg->metadata_max + || lfs->cfg->metadata_max % lfs->cfg->read_size == 0); + LFS_ASSERT(!lfs->cfg->metadata_max + || lfs->cfg->metadata_max % lfs->cfg->prog_size == 0); + LFS_ASSERT(!lfs->cfg->metadata_max + || lfs->cfg->block_size % lfs->cfg->metadata_max == 0); + // setup read cache if (lfs->cfg->read_buffer) { lfs->rcache.buffer = lfs->cfg->read_buffer;