Github/littlefs
1
Fork 0
Code Actions Activity

Extended alloc tests to more disk sizes, fixed alloc ckpoint bug

Browse Source 
I thought it was a bit funny we test various disk sizes in test_grow,
but no where else! test_grow actually found several bugs when reworking
the lookahead buffer related to small disks, so I figured we should have
some more intentional tests... And behold! A bug!

The issue is that we implicitly call lfs_alloc_ckpoint in
lfsr_mdir_commit. Originally the thinking was that this would be fine
since any in-flight blocks should be committed to a tracked btree/bshrub
first, but lfsr_bshrub_commit goes _through_ lfsr_mdir_commit. Bit of a
problem.

So if we call lfsr_bshrub_commit to add a recently allocated block, it
may end up calling lfsr_mdir_commit, erronously ckpointing the
allocator, and then clobbering the new block if the mdir needs to be
relocated, split, etc.

---

The fix here is to just move lfs_alloc_ckpoint out of lfsr_mdir_commit.
This adds a bit of noise, but it's probably a good thing for alloc
ckpoints to be explicit.

At least lfs_alloc_ckpoint is cheap:

           code          stack
  before: 36412           2680
  after:  36472 (+0.2%)   2680 (+0.0%)
This commit is contained in:
Christopher Haster
2024-07-16 18:41:22 -05:00
parent 4fc03f95a7
commit ac600ae35e
2 changed files with 84 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+18 -9
View File
@@ -7331,9 +7331,6 @@ static int lfsr_mroot_parent(lfs_t *lfs, const lfsr_mptr_t *mptr,
}
}
// needed in lfsr_mdir_commit
static void lfs_alloc_ckpoint(lfs_t *lfs);
// high-level mdir commit
//
// this is atomic and updates any opened mdirs, lfs_t, etc
@@ -7354,9 +7351,6 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir,
// lfs->mroot must have mid=-1
LFS_ASSERT(lfs->mroot.mid == -1);
// checkpoint the allocator
lfs_alloc_ckpoint(lfs);
// play out any attrs that affect our grm _before_ committing to disk,
// keep in mind we revert to on-disk gstate if we run into an error
lfsr_smid_t mid_ = mdir->mid;
@@ -8697,6 +8691,7 @@ static int lfsr_mtree_traverse(lfs_t *lfs, lfsr_traversal_t *t,
// needed in lfsr_mtree_gc
static int lfsr_mdir_fixorphans(lfs_t *lfs, lfsr_mdir_t *mdir);
static void lfs_alloc_ckpoint(lfs_t *lfs);
static void lfs_alloc_markfree(lfs_t *lfs);
// high-level mutating traversal, handle extra features that require
@@ -8762,6 +8757,10 @@ dropped:;
? lfs->cfg->gc_compact_thresh
: lfs->cfg->block_size - lfs->cfg->block_size/8);
// checkpoint the allocator
lfs_alloc_ckpoint(lfs);
// compact the mdir
err = lfsr_mdir_compact(lfs, mdir);
if (err) {
goto failed;
@@ -9177,6 +9176,7 @@ int lfsr_mkdir(lfs_t *lfs, const char *path) {
// mid updates, since the mid technically doesn't exist yet...
// commit our bookmark and a grm to self-remove in case of powerloss
lfs_alloc_ckpoint(lfs);
err = lfsr_mdir_commit(lfs, &mdir, LFSR_ATTRS(
LFSR_ATTR(LFSR_TAG_BOOKMARK, +1, LFSR_DATA_LEB128(did_))));
if (err) {
@@ -9196,6 +9196,7 @@ int lfsr_mkdir(lfs_t *lfs, const char *path) {
// commit our new directory into our parent, zeroing the grm in the
// process
lfsr_grm_pop(lfs);
lfs_alloc_ckpoint(lfs);
err = lfsr_mdir_commit(lfs, &mdir, LFSR_ATTRS(
LFSR_ATTR_NAME(
LFSR_TAG_SUP | LFSR_TAG_DIR, (!exists) ? +1 : 0,
@@ -9334,6 +9335,7 @@ int lfsr_remove(lfs_t *lfs, const char *path) {
bool zombie = lfsr_omdir_ismidopen(lfs, mdir.mid);
// remove the metadata entry
lfs_alloc_ckpoint(lfs);
err = lfsr_mdir_commit(lfs, &mdir, LFSR_ATTRS(
// create an orphan if zombied
//
@@ -9489,6 +9491,7 @@ int lfsr_rename(lfs_t *lfs, const char *old_path, const char *new_path) {
// rename our entry, copying all tags associated with the old rid to the
// new rid, while also marking the old rid for removal
lfs_alloc_ckpoint(lfs);
err = lfsr_mdir_commit(lfs, &new_mdir, LFSR_ATTRS(
LFSR_ATTR_NAME(
LFSR_TAG_SUP | old_tag, (!exists) ? +1 : 0,
@@ -10020,6 +10023,7 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file,
// create an orphan entry if we don't have one, this reserves the
// mid until first sync
if (!err) {
lfs_alloc_ckpoint(lfs);
err = lfsr_mdir_commit(lfs, &file->o.o.mdir, LFSR_ATTRS(
LFSR_ATTR_NAME(
LFSR_TAG_ORPHAN, +1,
@@ -11444,9 +11448,11 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) {
goto failed;
}
// checkpoint the allocator
lfs_alloc_ckpoint(lfs);
// commit!
LFS_ASSERT(attr_count <= sizeof(attrs)/sizeof(lfsr_attr_t));
err = lfsr_mdir_commit(lfs, &file->o.o.mdir,
attrs, attr_count);
if (err) {
@@ -12915,8 +12921,9 @@ static int lfsr_fs_fixgrm(lfs_t *lfs) {
// mark grm as taken care of
lfsr_grm_pop(lfs);
// remove the rid while also updating our grm
// checkpoint the allocator
lfs_alloc_ckpoint(lfs);
// remove the rid while atomically updating our grm
err = lfsr_mdir_commit(lfs, &mdir, LFSR_ATTRS(
LFSR_ATTR(LFSR_TAG_RM, -1, LFSR_DATA_NULL())));
if (err) {
@@ -12959,6 +12966,7 @@ static int lfsr_mdir_fixorphans(lfs_t *lfs, lfsr_mdir_t *mdir) {
lfsr_mid_bid(lfs, mdir->mid) >> lfs->mdir_bits,
lfsr_mid_rid(lfs, mdir->mid));
lfs_alloc_ckpoint(lfs);
err = lfsr_mdir_commit(lfs, mdir, LFSR_ATTRS(
LFSR_ATTR(LFSR_TAG_RM, -1, LFSR_DATA_NULL())));
if (err) {
@@ -13191,6 +13199,7 @@ int lfsr_fs_grow(lfs_t *lfs, lfs_size_t block_count_) {
lfs_alloc_discard(lfs);
// update our on-disk config
lfs_alloc_ckpoint(lfs);
int err = lfsr_mdir_commit(lfs, &lfs->mroot, LFSR_ATTRS(
LFSR_ATTR(
LFSR_TAG_GEOMETRY, 0,
tests/test_alloc.toml
+66 -27
View File
@@ -5,23 +5,29 @@
#
# It's counter-intuitive, but we run the alloc tests _after_ file/dir tests,
# since you can usually ignore allocator issues temporarily by making the test
# device really big (-DDISK_SIZE=16777216, etc)
# device really big
#
after = ['test_mtree', 'test_dirs', 'test_files']
# TODO test all of these with weird block sizes? would be nice to make this
# easy via the test_runner, either by handling it there or letting a single
# config limit the block count by a couple blocks
# test that we can alloc
[cases.test_alloc_alloc]
in = 'lfs.c'
defines.COUNT = [
'BLOCK_COUNT',
'BLOCK_COUNT-1',
'BLOCK_COUNT/2',
'BLOCK_COUNT/4',
'5',
'2',
]
defines.ERASE = [false, true]
in = 'lfs.c'
code = '''
// test various block counts
struct lfs_config cfg = *CFG;
cfg.block_count = COUNT;
lfs_t lfs;
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
lfsr_format(&lfs, &cfg) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, &cfg) => 0;
// start allocating
lfs_alloc_ckpoint(&lfs);
@@ -36,25 +42,36 @@ code = '''
alloced += 1;
// our allocator should stop at some point...
assert(alloced < 2*BLOCK_COUNT);
assert(alloced < 2*COUNT);
}
// excluding our mroot, we should have allocated exactly
// block_count-2 blocks
printf("alloced %d/%d blocks\n", alloced, (lfs_block_t)BLOCK_COUNT);
assert(alloced == BLOCK_COUNT-2);
printf("alloced %d/%d blocks\n", alloced, (lfs_block_t)COUNT);
assert(alloced == COUNT-2);
lfsr_unmount(&lfs) => 0;
'''
# test that we can realloc after an ack
[cases.test_alloc_reuse]
in = 'lfs.c'
defines.COUNT = [
'BLOCK_COUNT',
'BLOCK_COUNT-1',
'BLOCK_COUNT/2',
'BLOCK_COUNT/4',
'5',
'2',
]
defines.ERASE = [false, true]
in = 'lfs.c'
code = '''
// test various block counts
struct lfs_config cfg = *CFG;
cfg.block_count = COUNT;
lfs_t lfs;
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
lfsr_format(&lfs, &cfg) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, &cfg) => 0;
// start allocating
lfs_alloc_ckpoint(&lfs);
@@ -69,13 +86,13 @@ code = '''
alloced += 1;
// our allocator should stop at some point...
assert(alloced < 2*BLOCK_COUNT);
assert(alloced < 2*COUNT);
}
// excluding our mroot, we should have allocated exactly
// block_count-2 blocks
printf("alloced %d/%d blocks\n", alloced, (lfs_block_t)BLOCK_COUNT);
assert(alloced == BLOCK_COUNT-2);
printf("alloced %d/%d blocks\n", alloced, (lfs_block_t)COUNT);
assert(alloced == COUNT-2);
// ack again, effectively releasing all the previously alloced blocks
lfs_alloc_ckpoint(&lfs);
@@ -90,13 +107,13 @@ code = '''
alloced += 1;
// our allocator should stop at some point...
assert(alloced < 2*BLOCK_COUNT);
assert(alloced < 2*COUNT);
}
// excluding our mroot, we should have allocated exactly
// block_count-2 blocks
printf("alloced %d/%d blocks\n", alloced, (lfs_block_t)BLOCK_COUNT);
assert(alloced == BLOCK_COUNT-2);
printf("alloced %d/%d blocks\n", alloced, (lfs_block_t)COUNT);
assert(alloced == COUNT-2);
lfsr_unmount(&lfs) => 0;
'''
@@ -614,10 +631,21 @@ code = '''
# nospc tests mostly test that things still work when block allocation
# wraparound occurs
[cases.test_alloc_nospc_dirs]
defines.COUNT = [
'BLOCK_COUNT',
'BLOCK_COUNT-1',
'BLOCK_COUNT/2',
'BLOCK_COUNT/4',
'5',
'2',
]
code = '''
// test various block counts
struct lfs_config cfg = *CFG;
cfg.block_count = COUNT;
lfs_t lfs;
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
lfsr_format(&lfs, &cfg) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, &cfg) => 0;
// create directories until we run out of space
lfs_size_t n = 0;
@@ -635,7 +663,7 @@ code = '''
// remount?
if (remount) {
lfsr_unmount(&lfs) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, &cfg) => 0;
}
// check that our mkdir worked until we ran out of space
@@ -676,6 +704,14 @@ code = '''
'''
[cases.test_alloc_nospc_files]
defines.COUNT = [
'BLOCK_COUNT',
'BLOCK_COUNT-1',
'BLOCK_COUNT/2',
'BLOCK_COUNT/4',
'5',
'2',
]
defines.SIZE = [
'0',
'FILE_BUFFER_SIZE/2',
@@ -686,9 +722,12 @@ defines.SIZE = [
'8*BLOCK_SIZE',
]
code = '''
// test various block counts
struct lfs_config cfg = *CFG;
cfg.block_count = COUNT;
lfs_t lfs;
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
lfsr_format(&lfs, &cfg) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, &cfg) => 0;
// create files until we run out of space
uint32_t prng = 42;
@@ -728,7 +767,7 @@ code = '''
// remount?
if (remount) {
lfsr_unmount(&lfs) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, &cfg) => 0;
}
// check that our file writes worked until we ran out of space