Github/littlefs
1
Fork 0
Code Actions Activity

Fixed block crystallization not triggering on boundary underflow

Browse Source 
It's expected for our crystal boundary calculation to underflow, but
when checking for holes we were using the wrong signed/unsigned
comparison, so lfsr_file_carve thought there was a hole when there
wasn't:

  -crs   pos                              pos    -crs
  .-------|       <-- this lookup    ------|       .--
  '---.   | +crs                     --.   | +crs  '--
  .   |---|---.       ended up         |---|---.   .
  .   v   v   v       looking   -->    v   v   v   .
  .   .---.           like this        .---.       .
  .   |dat|                            |dat|       .
  .   '---'                            '---'       .
  .   0   .   n                        0   .   n   .

  '---.---'                                '---.---'
   no hole                               clearly a hole

This led to unoptimal block compaction and weird block alignment for
even relatively simple files.

The crystallization threshold is only a heuristic so this didn't exactly
break anything, but it was causing block-aligned files to waste a bit of
of space which wasn't great.

---

To hopefully protect against this in the future, I've added a couple
*_litmus tests to check that at least some simple block-aligned files
end up with the correct number of branches/blocks. This should at least
give us some confidence our crystallization algorithm is working as
intended.

We don't have all that many tests (any?) over the exact topology of
files, mainly because of how many heuristics are involved. Maybe we
should look into adding a couple more.

No code changes:

           code          stack
  before: 36396           2664
  after:  36396 (+0.0%)   2664 (+0.0%)
This commit is contained in:
Christopher Haster
2024-07-29 13:12:58 -05:00
parent 0ab0406d53
commit 7fe6e2ce45
2 changed files with 421 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+7 -5
View File
@@ -10665,12 +10665,14 @@ static int lfsr_file_flush_(lfs_t *lfs, lfsr_file_t *file,
return err;
}
// if left crystal neighbor is a fragment and there is no hole
// between our own crystal and our neighbor, include as a part
// of our crystal
// if left crystal neighbor is a fragment and there is no
// obvious hole between our own crystal and our neighbor,
// include as a part of our crystal
if (tag == LFSR_TAG_DATA
&& bid-(weight-1)+lfsr_data_size(bptr.data)
>= pos - (lfs->cfg->crystal_thresh-1)) {
// hole? holes can be quite large and shouldn't trigger
// crystallization
&& (lfs_soff_t)(bid-(weight-1)+lfsr_data_size(bptr.data))
>= (lfs_soff_t)(pos - (lfs->cfg->crystal_thresh-1))) {
crystal_start = bid-(weight-1);
// otherwise our neighbor determines our crystal boundary
tests/test_fwrite.toml
+414 -32
View File
@@ -11,7 +11,236 @@ defines.CRYSTAL_SIZE = [512]
# test with different prog sizes
defines.PROG_SIZE = [1, 16]
# more complex writing patterns to inlined files
# simple file writes
[cases.test_fwrite_simple]
defines.SIZE = [
'0',
'FILE_BUFFER_SIZE/2',
'2*FILE_BUFFER_SIZE',
'BLOCK_SIZE/2',
'BLOCK_SIZE',
'2*BLOCK_SIZE',
'4*BLOCK_SIZE',
]
defines.SYNC = [false, true]
if = [
# this just saves testing time
'SIZE <= 4*1024*FRAGMENT_SIZE',
]
code = '''
lfs_t lfs;
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
uint8_t wbuf[SIZE];
uint32_t prng = 42;
for (lfs_size_t i = 0; i < SIZE; i++) {
wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26);
}
lfsr_file_write(&lfs, &file, wbuf, SIZE) => SIZE;
// sync?
if (SYNC) {
lfsr_file_sync(&lfs, &file) => 0;
}
lfsr_file_close(&lfs, &file) => 0;
for (int remount = 0; remount < 2; remount++) {
// remount?
if (remount) {
lfsr_unmount(&lfs) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
}
// check our file with stat
struct lfs_info info;
lfsr_stat(&lfs, "hello", &info) => 0;
assert(strcmp(info.name, "hello") == 0);
assert(info.type == LFS_TYPE_REG);
assert(info.size == SIZE);
// and with dir read
lfsr_dir_t dir;
lfsr_dir_open(&lfs, &dir, "/") => 0;
lfsr_dir_read(&lfs, &dir, &info) => 0;
assert(strcmp(info.name, ".") == 0);
assert(info.type == LFS_TYPE_DIR);
assert(info.size == 0);
lfsr_dir_read(&lfs, &dir, &info) => 0;
assert(strcmp(info.name, "..") == 0);
assert(info.type == LFS_TYPE_DIR);
assert(info.size == 0);
lfsr_dir_read(&lfs, &dir, &info) => 0;
assert(strcmp(info.name, "hello") == 0);
assert(info.type == LFS_TYPE_REG);
assert(info.size == SIZE);
lfsr_dir_read(&lfs, &dir, &info) => LFS_ERR_NOENT;
lfsr_dir_close(&lfs, &dir) => 0;
// try reading our file
lfsr_file_open(&lfs, &file, "hello", LFS_O_RDONLY) => 0;
// is size correct?
lfsr_file_size(&lfs, &file) => SIZE;
// try reading
uint8_t rbuf[2*SIZE];
memset(rbuf, 0xaa, 2*SIZE);
lfsr_file_read(&lfs, &file, rbuf, 2*SIZE) => SIZE;
assert(memcmp(rbuf, wbuf, SIZE) == 0);
lfsr_file_close(&lfs, &file) => 0;
}
lfsr_unmount(&lfs) => 0;
'''
# test that simple block-aligned writes always end up as compact blocks
[cases.test_fwrite_simple_litmus]
defines.N = [0, 1, 2, 3, 4]
defines.SIZE = 'N*BLOCK_SIZE'
defines.SYNC = [false, true]
if = [
# this just saves testing time
'SIZE <= 4*1024*FRAGMENT_SIZE',
]
in = 'lfs.c'
code = '''
lfs_t lfs;
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
uint8_t wbuf[SIZE];
uint32_t prng = 42;
for (lfs_size_t i = 0; i < SIZE; i++) {
wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26);
}
lfsr_file_write(&lfs, &file, wbuf, SIZE) => SIZE;
// sync?
if (SYNC) {
lfsr_file_sync(&lfs, &file) => 0;
}
lfsr_file_close(&lfs, &file) => 0;
for (int remount = 0; remount < 2; remount++) {
// remount?
if (remount) {
lfsr_unmount(&lfs) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
}
// check our file with stat
struct lfs_info info;
lfsr_stat(&lfs, "hello", &info) => 0;
assert(strcmp(info.name, "hello") == 0);
assert(info.type == LFS_TYPE_REG);
assert(info.size == SIZE);
// and with dir read
lfsr_dir_t dir;
lfsr_dir_open(&lfs, &dir, "/") => 0;
lfsr_dir_read(&lfs, &dir, &info) => 0;
assert(strcmp(info.name, ".") == 0);
assert(info.type == LFS_TYPE_DIR);
assert(info.size == 0);
lfsr_dir_read(&lfs, &dir, &info) => 0;
assert(strcmp(info.name, "..") == 0);
assert(info.type == LFS_TYPE_DIR);
assert(info.size == 0);
lfsr_dir_read(&lfs, &dir, &info) => 0;
assert(strcmp(info.name, "hello") == 0);
assert(info.type == LFS_TYPE_REG);
assert(info.size == SIZE);
lfsr_dir_read(&lfs, &dir, &info) => LFS_ERR_NOENT;
lfsr_dir_close(&lfs, &dir) => 0;
// try reading our file
lfsr_file_open(&lfs, &file, "hello", LFS_O_RDONLY) => 0;
// is size correct?
lfsr_file_size(&lfs, &file) => SIZE;
// try reading
uint8_t rbuf[2*SIZE];
memset(rbuf, 0xaa, 2*SIZE);
lfsr_file_read(&lfs, &file, rbuf, 2*SIZE) => SIZE;
assert(memcmp(rbuf, wbuf, SIZE) == 0);
lfsr_file_close(&lfs, &file) => 0;
// here's our main test, do we end up with the expected
// number of branches/blocks? we need our internal btree
// traversal API to check this
//
lfs_block_t blocks = 0;
lfsr_file_open(&lfs, &file, "hello", LFS_O_RDONLY) => 0;
lfsr_btraversal_t bt = LFSR_BTRAVERSAL();
for (lfs_block_t i = 0;; i++) {
// a bit hacky, but this catches infinite loops
assert(i < 2*BLOCK_COUNT);
lfsr_bid_t bid;
lfsr_tag_t tag;
lfsr_bptr_t bptr;
int err = lfsr_bshrub_traverse(&lfs,
&file.o.o.mdir, &file.o.bshrub, &bt,
&bid, &tag, &bptr);
assert(!err || err == LFS_ERR_NOENT);
if (err == LFS_ERR_NOENT) {
break;
}
if (tag == LFSR_TAG_BRANCH) {
lfsr_rbyd_t *rbyd = (lfsr_rbyd_t*)bptr.data.u.buffer;
printf("traversal: %d 0x%x btree 0x%x.%x\n",
bid,
tag,
rbyd->blocks[0], rbyd->trunk);
} else if (tag == LFSR_TAG_DATA) {
printf("traversal: %d 0x%x data %d\n",
bid,
tag,
lfsr_data_size(bptr.data));
// if block crystallization is working we shouldn't be
// left with any inlined data fragments
assert(false);
} else if (tag == LFSR_TAG_BLOCK) {
printf("traversal: %d 0x%x block 0x%x.%x %d\n",
bid,
tag,
bptr.data.u.disk.block,
bptr.data.u.disk.off,
lfsr_data_size(bptr.data));
// keep track of how many data blocks we've seen
blocks += 1;
} else {
// well this shouldn't happen
printf("traversal: %d 0x%x\n",
bid,
tag);
assert(false);
}
}
lfsr_file_close(&lfs, &file) => 0;
// correct number of blocks?
assert(blocks == N);
}
lfsr_unmount(&lfs) => 0;
'''
# write files incrementally
[cases.test_fwrite_incr]
@@ -37,10 +266,10 @@ code = '''
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file, truncating in case of powerloss
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_TRUNC) => 0;
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
uint8_t wbuf[SIZE];
uint32_t prng = 42;
for (lfs_size_t i = 0; i < SIZE; i++) {
@@ -53,7 +282,7 @@ code = '''
if (SYNC) {
lfsr_file_sync(&lfs, &file) => 0;
}
// remount?
if (REMOUNT) {
lfsr_file_close(&lfs, &file) => 0;
@@ -113,6 +342,164 @@ code = '''
lfsr_unmount(&lfs) => 0;
'''
# test that incremental block-aligned writes always end up as compact blocks
[cases.test_fwrite_incr_litmus]
defines.N = [0, 1, 2, 3, 4]
defines.SIZE = 'N*BLOCK_SIZE'
defines.CHUNK = [32, 8, 1]
defines.SYNC = [false, true]
defines.REMOUNT = [false, true]
if = [
'CHUNK <= SIZE',
# this just saves testing time
'SIZE <= 4*1024*FRAGMENT_SIZE',
]
in = 'lfs.c'
code = '''
lfs_t lfs;
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
uint8_t wbuf[SIZE];
uint32_t prng = 42;
for (lfs_size_t i = 0; i < SIZE; i++) {
wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26);
}
for (lfs_size_t i = 0; i < SIZE; i += CHUNK) {
lfsr_file_write(&lfs, &file, &wbuf[i], CHUNK) => CHUNK;
// sync?
if (SYNC) {
lfsr_file_sync(&lfs, &file) => 0;
}
// remount?
if (REMOUNT) {
lfsr_file_close(&lfs, &file) => 0;
lfsr_unmount(&lfs) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// note the switch to append here
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_APPEND) => 0;
}
}
lfsr_file_close(&lfs, &file) => 0;
for (int remount = 0; remount < 2; remount++) {
// remount?
if (remount) {
lfsr_unmount(&lfs) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
}
// check our file with stat
struct lfs_info info;
lfsr_stat(&lfs, "hello", &info) => 0;
assert(strcmp(info.name, "hello") == 0);
assert(info.type == LFS_TYPE_REG);
assert(info.size == SIZE);
// and with dir read
lfsr_dir_t dir;
lfsr_dir_open(&lfs, &dir, "/") => 0;
lfsr_dir_read(&lfs, &dir, &info) => 0;
assert(strcmp(info.name, ".") == 0);
assert(info.type == LFS_TYPE_DIR);
assert(info.size == 0);
lfsr_dir_read(&lfs, &dir, &info) => 0;
assert(strcmp(info.name, "..") == 0);
assert(info.type == LFS_TYPE_DIR);
assert(info.size == 0);
lfsr_dir_read(&lfs, &dir, &info) => 0;
assert(strcmp(info.name, "hello") == 0);
assert(info.type == LFS_TYPE_REG);
assert(info.size == SIZE);
lfsr_dir_read(&lfs, &dir, &info) => LFS_ERR_NOENT;
lfsr_dir_close(&lfs, &dir) => 0;
// try reading our file
lfsr_file_open(&lfs, &file, "hello", LFS_O_RDONLY) => 0;
// is size correct?
lfsr_file_size(&lfs, &file) => SIZE;
// try reading
uint8_t rbuf[2*SIZE];
memset(rbuf, 0xaa, 2*SIZE);
lfsr_file_read(&lfs, &file, rbuf, 2*SIZE) => SIZE;
assert(memcmp(rbuf, wbuf, SIZE) == 0);
lfsr_file_close(&lfs, &file) => 0;
// here's our main test, do we end up with the expected
// number of branches/blocks? we need our internal btree
// traversal API to check this
//
lfs_block_t blocks = 0;
lfsr_file_open(&lfs, &file, "hello", LFS_O_RDONLY) => 0;
lfsr_btraversal_t bt = LFSR_BTRAVERSAL();
for (lfs_block_t i = 0;; i++) {
// a bit hacky, but this catches infinite loops
assert(i < 2*BLOCK_COUNT);
lfsr_bid_t bid;
lfsr_tag_t tag;
lfsr_bptr_t bptr;
int err = lfsr_bshrub_traverse(&lfs,
&file.o.o.mdir, &file.o.bshrub, &bt,
&bid, &tag, &bptr);
assert(!err || err == LFS_ERR_NOENT);
if (err == LFS_ERR_NOENT) {
break;
}
if (tag == LFSR_TAG_BRANCH) {
lfsr_rbyd_t *rbyd = (lfsr_rbyd_t*)bptr.data.u.buffer;
printf("traversal: %d 0x%x btree 0x%x.%x\n",
bid,
tag,
rbyd->blocks[0], rbyd->trunk);
} else if (tag == LFSR_TAG_DATA) {
printf("traversal: %d 0x%x data %d\n",
bid,
tag,
lfsr_data_size(bptr.data));
// if block crystallization is working we shouldn't be
// left with any inlined data fragments
assert(false);
} else if (tag == LFSR_TAG_BLOCK) {
printf("traversal: %d 0x%x block 0x%x.%x %d\n",
bid,
tag,
bptr.data.u.disk.block,
bptr.data.u.disk.off,
lfsr_data_size(bptr.data));
// keep track of how many data blocks we've seen
blocks += 1;
} else {
// well this shouldn't happen
printf("traversal: %d 0x%x\n",
bid,
tag);
assert(false);
}
}
lfsr_file_close(&lfs, &file) => 0;
// correct number of blocks?
assert(blocks == N);
}
lfsr_unmount(&lfs) => 0;
'''
# overwrite files
# TODO this is too slow right now, but should speed up with better
# write strategies
@@ -145,10 +532,10 @@ code = '''
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file, truncating in case of powerloss
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_TRUNC) => 0;
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
// simulate our file in ram
uint8_t sim[SIZE];
uint32_t prng = 42;
@@ -325,10 +712,10 @@ code = '''
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file, truncating in case of powerloss
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_TRUNC) => 0;
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
// simulate our file in ram
uint8_t sim[SIZE];
uint32_t prng = 42;
@@ -510,10 +897,10 @@ code = '''
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file, truncating in case of powerloss
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_TRUNC) => 0;
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
// simulate our file in ram
uint8_t sim[lfs_max(FROM,TO)];
memset(sim, 0, lfs_max(FROM,TO));
@@ -635,10 +1022,10 @@ code = '''
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file, truncating in case of powerloss
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_TRUNC) => 0;
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
// simulate our file in ram
uint8_t sim[lfs_max(FROM,lfs_max(AND,TO))];
memset(sim, 0, lfs_max(FROM,lfs_max(AND,TO)));
@@ -768,10 +1155,10 @@ code = '''
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file, truncating in case of powerloss
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_TRUNC) => 0;
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
// simulate our file in ram
uint8_t sim[lfs_max(FROM,TO)];
memset(sim, 0, lfs_max(FROM,TO));
@@ -897,10 +1284,10 @@ code = '''
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file, truncating in case of powerloss
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_TRUNC) => 0;
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
// simulate our file in ram
uint8_t sim[lfs_max(FROM,lfs_max(AND,TO))];
memset(sim, 0, lfs_max(FROM,lfs_max(AND,TO)));
@@ -1035,10 +1422,10 @@ code = '''
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file, truncating in case of powerloss
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_TRUNC) => 0;
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
// simulate our file in ram
uint8_t sim[SIZE];
uint32_t prng = 42;
@@ -1079,7 +1466,7 @@ code = '''
if (SYNC) {
lfsr_file_sync(&lfs, &file) => 0;
}
// remount?
if (REMOUNT) {
lfsr_file_close(&lfs, &file) => 0;
@@ -1173,10 +1560,10 @@ code = '''
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file, truncating in case of powerloss
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_TRUNC) => 0;
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
// simulate our file in ram
uint8_t sim[SIZE];
uint32_t prng = 42;
@@ -1362,10 +1749,10 @@ code = '''
lfsr_format(&lfs, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file, truncating in case of powerloss
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "hello",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_TRUNC) => 0;
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
// simulate our file in ram
uint8_t sim[SIZE];
uint32_t prng = 42;
@@ -1605,7 +1992,7 @@ code = '''
if (SYNC) {
lfsr_file_sync(&lfs, &file) => 0;
}
// remount?
if (REMOUNT) {
lfsr_file_close(&lfs, &file) => 0;
@@ -1753,7 +2140,7 @@ code = '''
if (SYNC) {
lfsr_file_sync(&lfs, &file) => 0;
}
// remount?
if (REMOUNT) {
lfsr_file_close(&lfs, &file) => 0;
@@ -1982,7 +2369,7 @@ code = '''
if (SYNC) {
lfsr_file_sync(&lfs, &file) => 0;
}
// tell should report the new position
lfsr_file_tell(&lfs, &file) => off + chunk;
@@ -2133,7 +2520,7 @@ code = '''
if (SYNC) {
lfsr_file_sync(&lfs, &file) => 0;
}
// tell should report the new position
lfsr_file_tell(&lfs, &file) => off + chunk;
@@ -2530,8 +2917,3 @@ code = '''
'''
# TODO
# [cases.test_fwrite_push] ?
# [cases.test_fwrite_pop] ?
# [cases.test_fwrite_rwtfpp_fuzz] ?