From b6a0b7afe25d576309aa73835d6e202c528b9455 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Fri, 16 May 2025 11:10:52 -0500 Subject: [PATCH] Implemented 1-commit shrubs for small in-cache files This adds an alternative sync path for small in-cache files, where we combine the shrub commit with the file sync commit, potentially writing everything out in a single prog. This is reminiscent of bmoss (old inlined) files, but notably avoids the additional on-disk data-structure and extra code necessary to manage it. --- The motivation for this comes from ongoing benchmarking, where we're seeing a fairly significant regression in small-file performance on NAND flash. Especially curious since the whole goal of this work was to make NAND flash tractable. But it makes sense: 2 commits are more than 1. While the separate shrub + sync commits are barely noticeable on NOR flash, on NAND flash, with its huge >512B prog sizes, the extra commit is hard to miss. In theory, the most performant solution would be to merge all bshrub commits with sync commits whenever possible. This is technically doable, and may make sense for a more performance-focused littlefs driver, but it would 1. require an invasive code rewrite, 2. entangle lfsr_file_sync -> lfsr_file_flush -> lfsr_file_carve, and 3. add even more code. If we only merge shrub + sync commits when the file fits in the cache, we can skip lfsr_file_flush, craft a simple shrubcommit by hand, and avoid all of this mess. While still speeding up the most common write path for small files. And sure enough, our bench-many benchmark, which creates ~1000 4 byte files, shows a ~2x speed improvement on bs=128KiB NAND (basically just because we compact/split ~5 times instead of ~10 times). --- Unfortunately the shrub commit requires quite a bit of state to set up, and in the middle of lfsr_file_sync, one of the more critical functions on our stack hot-path. So this does have a big cost: code stack ctx before: 35836 2368 636 after: 35992 (+0.4%) 2408 (+1.7%) 636 (+0.0%) Though this is also a perfect contender to be compile-time ifdefed. It may be worth adding something like LFS_NO_MERGESHRUBCOMMITS (better name?) to claw back some of the cost if you don't care about performances as much. This could also probably be a bit cheaper if our file write configs were organized differently... At the moment we need to check inline_size, fragment_size, _and_ crystal_thresh since these can sometimes overlap. But this is waiting on the future config rework. --- Actually... Looking at this closer, I'm not sure the added commit logic should really be included in the hot-path cost... lfsr_file_flush is the hot path, and flush -> sync are sequential operations that don't really share stack (with the shrub commit we humorously _never_ call flush). The commit logic is only being dragged in because our stack measurements are pessimistic about shrinkwrapping, which is a bit frustrating. I've explored shrinkwrapping in stack.py before, but the idea pretty much failed. Unfortunately GCC simply doesn't make this info available short of parsing the per-arch disassembly. --- lfs.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/lfs.c b/lfs.c index f32faa07..be29fd6d 100644 --- a/lfs.c +++ b/lfs.c @@ -12715,15 +12715,25 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { // flush succeeds but mdir commit fails it's ok to fall back to // our flushed state // - err = lfsr_file_flush(lfs, file); - if (err) { - goto failed; + // though don't flush quite yet if our file is small and can be + // combined with sync in a single commit + if (file->cache.size < lfsr_file_size_(file) + || file->cache.size > lfs->cfg->inline_size + || file->cache.size > lfs->cfg->fragment_size + || file->cache.size >= lfs->cfg->crystal_thresh) { + err = lfsr_file_flush(lfs, file); + if (err) { + goto failed; + } } // build a commit of any pending file metadata - lfsr_rattr_t rattrs[3]; + lfsr_rattr_t rattrs[4]; lfs_size_t rattr_count = 0; lfsr_data_t name_data; + lfsr_rattr_t shrub_rattrs[1]; + lfs_size_t shrub_rattr_count = 0; + lfsr_shrubcommit_t shrub_commit; // not created yet? need to convert to normal file if (lfsr_o_isuncreat(file->b.o.flags)) { @@ -12744,6 +12754,31 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { &name_data); } + // pending small file flush? + if (lfsr_o_isunflush(file->b.o.flags)) { + // this only works if the file is entirely in our cache + LFS_ASSERT(file->cache.pos == 0); + LFS_ASSERT(file->cache.size == lfsr_file_size_(file)); + + // reset the bshrub + lfsr_bshrub_init(&file->b); + + // build a small shrub commit + if (file->cache.size > 0) { + shrub_rattrs[shrub_rattr_count++] = LFSR_RATTR_DATA( + LFSR_TAG_DATA, +file->cache.size, + (const lfsr_data_t*)&file->cache); + + LFS_ASSERT(shrub_rattr_count + <= sizeof(shrub_rattrs)/sizeof(lfsr_rattr_t)); + shrub_commit.bshrub = &file->b; + shrub_commit.rid = 0; + shrub_commit.rattrs = shrub_rattrs; + shrub_commit.rattr_count = shrub_rattr_count; + rattrs[rattr_count++] = LFSR_RATTR_SHRUBCOMMIT(&shrub_commit); + } + } + // pending file changes? if (lfsr_o_isunsync(file->b.o.flags)) { // make sure data is on-disk before committing metadata @@ -12753,15 +12788,19 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { } // zero size files should have no bshrub/btree - LFS_ASSERT(file->b.shrub.weight > 0 + LFS_ASSERT(lfsr_file_size_(file) > 0 || lfsr_bshrub_isbnull(&file->b)); // no bshrub/btree? - if (lfsr_bshrub_isbnull(&file->b)) { + if (lfsr_bshrub_isbnull(&file->b) + && !(lfsr_o_isunflush(file->b.o.flags) + && file->cache.size > 0)) { rattrs[rattr_count++] = LFSR_RATTR( LFSR_TAG_RM | LFSR_TAG_MASK8 | LFSR_TAG_STRUCT, 0); // bshrub? - } else if (lfsr_bshrub_isbshrub(&file->b)) { + } else if (lfsr_bshrub_isbshrub(&file->b) + || (lfsr_o_isunflush(file->b.o.flags) + && file->cache.size > 0)) { rattrs[rattr_count++] = LFSR_RATTR_SHRUB( LFSR_TAG_MASK8 | LFSR_TAG_BSHRUB, 0, // note we use the staged trunk here @@ -12900,7 +12939,11 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { } // mark as synced - file->b.o.flags &= ~(LFS_o_UNSYNC | LFS_o_UNCREAT | LFS_O_DESYNC); + file->b.o.flags &= ~( + LFS_o_UNSYNC + | LFS_o_UNFLUSH + | LFS_o_UNCREAT + | LFS_O_DESYNC); return 0; failed:;