Github/littlefs
1
Fork 0
Code Actions Activity

Fixed writing of unaligned fragments to new files

Browse Source 
This was only noticed when forcing btrees for other unrelated tests
(INLINED_SIZE=0, CRYSTAL_THRESH=-1), where even simple file writes would
end up with some unaligned fragments the size of our file buffer.

It was hard to notice without forcing btrees, since our crystallization
algorithm has a tendency to fix alignment issues.

The problem was that we weren't bypassing the file buffer correctly when
buffer.size == 0. We relied on the LFS_F_UNFLUSH flag to know if we
could do a bypassing write, but inlined files set the LFS_F_UNFLUSH flag
even for empty files. This led to blocked bypassing writes, attempts
to merge with empty buffers, and unaligned fragments.

To avoid this, lfsr_file_write now checks for buffer.size == 0
explicitly. There may be a better solution, but for now this gets the
job done.

---

To make sure we don't end up with unaligned fragments again in the
future, I've extend the fwrite litmus tests to check for well-aligned
fragments in addition to blocks:

- test_fwrite_simple_litmus_fragments
- test_fwrite_incr_litmus_fragments

These fixes end up adding a bit of code, as checking for both the
unflushed flag and buffer.size == 0 has a cost:

           code          stack
  before: 36424           2680
  after:  36452 (+0.1%)   2680 (+0.0%)

But hey, file aren't stuck with unaligned fragments anymore.
This commit is contained in:
Christopher Haster
2024-08-12 01:58:00 -05:00
parent 9d4b4d2557
commit dffd8fa0fa
2 changed files with 315 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+7 -3
View File
@@ -11846,7 +11846,8 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file,
// strictly necessary, but enforces a more intuitive write order
// and avoids weird cases with low-level write heuristics
//
if (!lfsr_f_isunflush(file->o.o.flags)
if ((!lfsr_f_isunflush(file->o.o.flags)
|| file->buffer.size == 0)
&& size >= lfsr_file_buffersize(lfs, file)) {
err = lfsr_file_flush_(lfs, file,
pos, buffer_, size);
@@ -11864,6 +11865,7 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file,
lfsr_file_buffersize(lfs, file));
file->buffer.size = lfsr_file_buffersize(lfs, file);
file->o.o.flags &= ~LFS_F_UNFLUSH;
written += size;
pos += size;
buffer_ += size;
@@ -11881,14 +11883,16 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file,
// 2. Bypassing the buffer above means we only write to the
// buffer once, and flush at most twice.
//
if (!lfsr_f_isunflush(file->o.o.flags)
if ((!lfsr_f_isunflush(file->o.o.flags)
|| file->buffer.size == 0)
|| (pos >= file->buffer.pos
&& pos <= file->buffer.pos + file->buffer.size
&& pos
< file->buffer.pos
+ lfsr_file_buffersize(lfs, file))) {
// unused buffer? we can move it where we need it
if (!lfsr_f_isunflush(file->o.o.flags)) {
if ((!lfsr_f_isunflush(file->o.o.flags)
|| file->buffer.size == 0)) {
file->buffer.pos = pos;
file->buffer.size = 0;
}
tests/test_fwrite.toml
+308 -14
View File
@@ -98,15 +98,154 @@ code = '''
lfsr_unmount(&lfs) => 0;
'''
# test that simple fragment-aligned writes are optimal
[cases.test_fwrite_simple_litmus_fragments]
defines.N = [0, 1, 2, 3, 4]
defines.SIZE = 'N*FRAGMENT_SIZE'
# force a btree node
defines.INLINE_SIZE = 0
defines.CRYSTAL_THRESH = -1
defines.SYNC = [false, true]
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 fragments? we need our internal btree traversal
// API to check this
//
lfs_size_t fragments = 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));
// keep track of how many fragments we've seen
fragments += 1;
} 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));
// we disabled block crystallization so this shouldn't
// happen
assert(false);
} 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 fragments?
assert(fragments == N);
}
lfsr_unmount(&lfs) => 0;
'''
# test that simple block-aligned writes always end up as compact blocks
[cases.test_fwrite_simple_litmus]
[cases.test_fwrite_simple_litmus_blocks]
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;
@@ -342,18 +481,17 @@ code = '''
lfsr_unmount(&lfs) => 0;
'''
# test that incremental block-aligned writes always end up as compact blocks
[cases.test_fwrite_incr_litmus]
# test that incremental fragment-aligned writes are optimal
[cases.test_fwrite_incr_litmus_fragments]
defines.N = [0, 1, 2, 3, 4]
defines.SIZE = 'N*BLOCK_SIZE'
defines.SIZE = 'N*FRAGMENT_SIZE'
defines.CHUNK = [32, 8, 1]
# force a btree node
defines.INLINE_SIZE = 0
defines.CRYSTAL_THRESH = -1
defines.SYNC = [false, true]
defines.REMOUNT = [false, true]
if = [
'CHUNK <= SIZE',
# this just saves testing time
'SIZE <= 4*1024*FRAGMENT_SIZE',
]
if = 'CHUNK <= SIZE'
in = 'lfs.c'
code = '''
lfs_t lfs;
@@ -370,7 +508,163 @@ code = '''
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;
lfsr_file_write(&lfs, &file, &wbuf[i], lfs_min(CHUNK, SIZE-i))
=> lfs_min(CHUNK, SIZE-i);
// 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 fragments? we need our internal btree traversal
// API to check this
//
lfs_size_t fragments = 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));
// keep track of how many fragments we've seen
fragments += 1;
} 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));
// we disabled block crystallization so this shouldn't
// happen
assert(false);
} 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 fragments?
assert(fragments == N);
}
lfsr_unmount(&lfs) => 0;
'''
# test that incremental block-aligned writes always end up as compact blocks
[cases.test_fwrite_incr_litmus_blocks]
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'
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], lfs_min(CHUNK, SIZE-i))
=> lfs_min(CHUNK, SIZE-i);
// sync?
if (SYNC) {