From dffd8fa0fa77f90f0ac6d7ee1a8d80551f07ce4c Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 12 Aug 2024 01:58:00 -0500 Subject: [PATCH] Fixed writing of unaligned fragments to new files 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. --- lfs.c | 10 +- tests/test_fwrite.toml | 322 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 315 insertions(+), 17 deletions(-) diff --git a/lfs.c b/lfs.c index 260bc84a..16074c50 100644 --- a/lfs.c +++ b/lfs.c @@ -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; } diff --git a/tests/test_fwrite.toml b/tests/test_fwrite.toml index 04feb0d7..cb68c05a 100644 --- a/tests/test_fwrite.toml +++ b/tests/test_fwrite.toml @@ -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) {