From 649640c605a41fb1a8cb4cbaa4a620e9edbedbd6 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Fri, 31 May 2019 06:42:15 -0500 Subject: [PATCH 1/4] Fixed workaround for erase sizes >1024 B Introduced in 0b76635, the workaround for erases sizes >1024 is to commit with an unaligned CRC tag. Upon reading an unaligned CRC, littlefs should treat the metadata pair as "requires erased". While necessary for portability, this also lets us workaround the lack of handling of erases sizes >1024. Unfortunately, this workaround wasn't implemented correctly (by me) in the case that the metadata-pair does not immediately compact. This is solved here by added the erase check to lfs_dir_commit. Note this is still only a part of a workaround which should be replaced. One potential solution is to pad the commit with multiple smaller CRC tags until we reach the next prog_size boundary. found by kazink --- lfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lfs.c b/lfs.c index c554135..af3a5d7 100644 --- a/lfs.c +++ b/lfs.c @@ -1247,7 +1247,7 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { // build crc tag bool reset = ~lfs_frombe32(tag) >> 31; tag = LFS_MKTAG(LFS_TYPE_CRC + reset, 0x3ff, - off - (commit->off+sizeof(lfs_tag_t))); + lfs_min(off - (commit->off+sizeof(lfs_tag_t)), 0x3fe)); // write out crc uint32_t footer[2]; @@ -1760,6 +1760,8 @@ static int lfs_dir_commit(lfs_t *lfs, lfs_mdir_t *dir, // successful commit, update dir dir->off = commit.off; dir->etag = commit.ptag; + // workaround to handle prog_size >1024 + dir->erased = (dir->off % lfs->cfg->prog_size == 0); // note we able to have already handled move here if (lfs_gstate_hasmovehere(&lfs->gpending, dir->pair)) { From ef1c926940f176b20078e56ae4dddd268d9c0497 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 24 Jul 2019 14:19:39 -0500 Subject: [PATCH 2/4] Increased testing to include geometries that can't be fully tested This is primarily to get better test coverage over devices with very large erase/prog/read sizes. The unfortunate state of the tests is that most of them rely on a specific block device size, so that ENOSPC and ECORRUPT errors occur in specific situations. This should be improved in the future, but at least for now we can open up some of the simpler tests to run on these different configurations. Also added testing over both 0x00 and 0xff erase values in emubd. Also added a number of small file tests that expose issues prevalent on NAND devices. --- .travis.yml | 12 ++++++++ emubd/lfs_emubd.c | 2 +- emubd/lfs_emubd.h | 16 ++-------- tests/template.fmt | 2 +- tests/test_files.sh | 71 +++++++++++++++++++++++++++++++++++++++++---- 5 files changed, 81 insertions(+), 22 deletions(-) diff --git a/.travis.yml b/.travis.yml index dcfedf6..04f8720 100644 --- a/.travis.yml +++ b/.travis.yml @@ -23,8 +23,20 @@ script: - make test QUIET=1 CFLAGS+="-DLFS_BLOCK_COUNT=1023 -DLFS_LOOKAHEAD_SIZE=256" - make clean test QUIET=1 CFLAGS+="-DLFS_INLINE_MAX=0" + - make clean test QUIET=1 CFLAGS+="-DLFS_EMUBD_ERASE_VALUE=0xff" - make clean test QUIET=1 CFLAGS+="-DLFS_NO_INTRINSICS" + # additional configurations that don't support all tests (this should be + # fixed but at the moment it is what it is) + - make test_files QUIET=1 + CFLAGS+="-DLFS_READ_SIZE=1 -DLFS_BLOCK_SIZE=4096" + - make test_files QUIET=1 + CFLAGS+="-DLFS_READ_SIZE=\(2*1024\) -DLFS_BLOCK_SIZE=\(64*1024\)" + - make test_files QUIET=1 + CFLAGS+="-DLFS_READ_SIZE=\(8*1024\) -DLFS_BLOCK_SIZE=\(64*1024\)" + - make test_files QUIET=1 + CFLAGS+="-DLFS_READ_SIZE=11 -DLFS_BLOCK_SIZE=704" + # compile and find the code size with the smallest configuration - make clean size OBJ="$(ls lfs*.o | tr '\n' ' ')" diff --git a/emubd/lfs_emubd.c b/emubd/lfs_emubd.c index 3f31bfa..3c77945 100644 --- a/emubd/lfs_emubd.c +++ b/emubd/lfs_emubd.c @@ -83,7 +83,7 @@ int lfs_emubd_create(const struct lfs_config *cfg, const char *path) { snprintf(emu->child, LFS_NAME_MAX, ".stats"); FILE *f = fopen(emu->path, "r"); if (!f) { - memset(&emu->stats, 0, sizeof(emu->stats)); + memset(&emu->stats, LFS_EMUBD_ERASE_VALUE, sizeof(emu->stats)); } else { size_t res = fread(&emu->stats, sizeof(emu->stats), 1, f); lfs_emubd_fromle32(emu); diff --git a/emubd/lfs_emubd.h b/emubd/lfs_emubd.h index 64afa3e..0fd78c1 100644 --- a/emubd/lfs_emubd.h +++ b/emubd/lfs_emubd.h @@ -17,20 +17,8 @@ extern "C" // Config options -#ifndef LFS_EMUBD_READ_SIZE -#define LFS_EMUBD_READ_SIZE 1 -#endif - -#ifndef LFS_EMUBD_PROG_SIZE -#define LFS_EMUBD_PROG_SIZE 1 -#endif - -#ifndef LFS_EMUBD_ERASE_SIZE -#define LFS_EMUBD_ERASE_SIZE 512 -#endif - -#ifndef LFS_EMUBD_TOTAL_SIZE -#define LFS_EMUBD_TOTAL_SIZE 524288 +#ifndef LFS_EMUBD_ERASE_VALUE +#define LFS_EMUBD_ERASE_VALUE 0x00 #endif diff --git a/tests/template.fmt b/tests/template.fmt index 7fdec7c..b52d907 100644 --- a/tests/template.fmt +++ b/tests/template.fmt @@ -82,7 +82,7 @@ uintmax_t test; #endif #ifndef LFS_CACHE_SIZE -#define LFS_CACHE_SIZE 64 +#define LFS_CACHE_SIZE (64 % LFS_PROG_SIZE == 0 ? 64 : LFS_PROG_SIZE) #endif #ifndef LFS_LOOKAHEAD_SIZE diff --git a/tests/test_files.sh b/tests/test_files.sh index 5251c61..950636a 100755 --- a/tests/test_files.sh +++ b/tests/test_files.sh @@ -135,20 +135,79 @@ tests/test.py << TEST lfs_unmount(&lfs) => 0; TEST -echo "--- Many file test ---" +echo "--- Many files test ---" tests/test.py << TEST lfs_format(&lfs, &cfg) => 0; TEST tests/test.py << TEST - // Create 300 files of 6 bytes + // Create 300 files of 7 bytes lfs_mount(&lfs, &cfg) => 0; - lfs_mkdir(&lfs, "directory") => 0; for (unsigned i = 0; i < 300; i++) { snprintf((char*)buffer, sizeof(buffer), "file_%03d", i); - lfs_file_open(&lfs, &file[0], (char*)buffer, LFS_O_WRONLY | LFS_O_CREAT) => 0; - size = 6; - memcpy(wbuffer, "Hello", size); + lfs_file_open(&lfs, &file[0], (char*)buffer, + LFS_O_RDWR | LFS_O_CREAT | LFS_O_EXCL) => 0; + size = 7; + snprintf((char*)wbuffer, size, "Hi %03d", i); lfs_file_write(&lfs, &file[0], wbuffer, size) => size; + lfs_file_rewind(&lfs, &file[0]) => 0; + lfs_file_read(&lfs, &file[0], rbuffer, size) => size; + memcmp(wbuffer, rbuffer, size) => 0; + lfs_file_close(&lfs, &file[0]) => 0; + } + lfs_unmount(&lfs) => 0; +TEST + +echo "--- Many files with flush test ---" +tests/test.py << TEST + lfs_format(&lfs, &cfg) => 0; +TEST +tests/test.py << TEST + // Create 300 files of 7 bytes + lfs_mount(&lfs, &cfg) => 0; + for (unsigned i = 0; i < 300; i++) { + snprintf((char*)buffer, sizeof(buffer), "file_%03d", i); + lfs_file_open(&lfs, &file[0], (char*)buffer, + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; + size = 7; + snprintf((char*)wbuffer, size, "Hi %03d", i); + lfs_file_write(&lfs, &file[0], wbuffer, size) => size; + lfs_file_close(&lfs, &file[0]) => 0; + + snprintf((char*)buffer, sizeof(buffer), "file_%03d", i); + lfs_file_open(&lfs, &file[0], (char*)buffer, LFS_O_RDONLY) => 0; + size = 7; + snprintf((char*)wbuffer, size, "Hi %03d", i); + lfs_file_read(&lfs, &file[0], rbuffer, size) => size; + memcmp(wbuffer, rbuffer, size) => 0; + lfs_file_close(&lfs, &file[0]) => 0; + } + lfs_unmount(&lfs) => 0; +TEST + +echo "--- Many files with power cycle test ---" +tests/test.py << TEST + lfs_format(&lfs, &cfg) => 0; +TEST +tests/test.py << TEST + // Create 300 files of 7 bytes + lfs_mount(&lfs, &cfg) => 0; + for (unsigned i = 0; i < 300; i++) { + snprintf((char*)buffer, sizeof(buffer), "file_%03d", i); + lfs_file_open(&lfs, &file[0], (char*)buffer, + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; + size = 7; + snprintf((char*)wbuffer, size, "Hi %03d", i); + lfs_file_write(&lfs, &file[0], wbuffer, size) => size; + lfs_file_close(&lfs, &file[0]) => 0; + lfs_unmount(&lfs) => 0; + + lfs_mount(&lfs, &cfg) => 0; + snprintf((char*)buffer, sizeof(buffer), "file_%03d", i); + lfs_file_open(&lfs, &file[0], (char*)buffer, LFS_O_RDONLY) => 0; + size = 7; + snprintf((char*)wbuffer, size, "Hi %03d", i); + lfs_file_read(&lfs, &file[0], rbuffer, size) => size; + memcmp(wbuffer, rbuffer, size) => 0; lfs_file_close(&lfs, &file[0]) => 0; } lfs_unmount(&lfs) => 0; From 312326c4e47ed9bc19f0b120faf9d126a15c0bd1 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 24 Jul 2019 14:24:29 -0500 Subject: [PATCH 3/4] Added a better solution for large prog sizes A current limitation of the lfs tag is the 10-bit (1024) length field. This field is used to indicate padding for commits and effectively limits the size of commits to 1KiB. Because commits must be prog size aligned, this is a problem on devices with prog size > 1024. [---- 6KiB erase block ----] [-- 2KiB prog size --|-- 2KiB prog size --|-- 2KiB prog size --] [ 1KiB commit | ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ] This can be increased to 12-bit (4096), but for NAND devices this is still to small to completely solve the issue. The previous workaround was to just create unaligned commits. This can occur naturally if littlefs is used on portable media as the prog size does not have to be consistent on different drivers. If littlefs sees an unaligned commit, it treats the dir as unerased and must compact the dir if it creates any new commits. Unfortunately this isn't great. It effectively means that every small commit forced an erase on devices with prog size > 1024. This is pretty terrible. [---- 6KiB erase block ----] [-- 2KiB prog size --|-- 2KiB prog size --|-- 2KiB prog size --] [ 1KiB commit |------------------- wasted ---------------------] A different solution, implemented here, is to use multiple crc tags to pad the commit until the remaining space fits in the padding. This effectively looks like multiple empty commits and has a small runtime cost to parse these tags, but otherwise does no harm. [---- 6KiB erase block ----] [-- 2KiB prog size --|-- 2KiB prog size --|-- 2KiB prog size --] [ 1KiB commit | noop | 1KiB commit | noop | 1KiB commit | noop ] It was a bit tricky to implement, but now we can effectively support unlimited prog sizes since there's no limit to the number of commits in a block. found by kazink and joicetm --- lfs.c | 113 ++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 66 insertions(+), 47 deletions(-) diff --git a/lfs.c b/lfs.c index af3a5d7..a8724bb 100644 --- a/lfs.c +++ b/lfs.c @@ -1232,65 +1232,85 @@ static int lfs_dir_commitattr(lfs_t *lfs, struct lfs_commit *commit, static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { // align to program units - lfs_off_t off = lfs_alignup(commit->off + 2*sizeof(uint32_t), + const lfs_off_t off1 = commit->off + sizeof(lfs_tag_t); + const lfs_off_t end = lfs_alignup(off1 + sizeof(uint32_t), lfs->cfg->prog_size); - // read erased state from next program unit - lfs_tag_t tag; - int err = lfs_bd_read(lfs, - NULL, &lfs->rcache, sizeof(tag), - commit->block, off, &tag, sizeof(tag)); - if (err && err != LFS_ERR_CORRUPT) { - return err; - } + // create crc tags to fill up remainder of commit, note that + // padding is not crcd, which lets fetches skip padding but + // makes committing a bit more complicated + while (commit->off < end) { + lfs_off_t off = commit->off + sizeof(lfs_tag_t); + lfs_off_t noff = lfs_min(end - off, 0x3fe) + off; + if (noff < end) { + noff = lfs_min(noff, end - 2*sizeof(uint32_t)); + } - // build crc tag - bool reset = ~lfs_frombe32(tag) >> 31; - tag = LFS_MKTAG(LFS_TYPE_CRC + reset, 0x3ff, - lfs_min(off - (commit->off+sizeof(lfs_tag_t)), 0x3fe)); + // read erased state from next program unit + lfs_tag_t tag = 0xffffffff; + int err = lfs_bd_read(lfs, + NULL, &lfs->rcache, sizeof(tag), + commit->block, noff, &tag, sizeof(tag)); + if (err && err != LFS_ERR_CORRUPT) { + return err; + } - // write out crc - uint32_t footer[2]; - footer[0] = lfs_tobe32(tag ^ commit->ptag); - commit->crc = lfs_crc(commit->crc, &footer[0], sizeof(footer[0])); - footer[1] = lfs_tole32(commit->crc); - err = lfs_bd_prog(lfs, - &lfs->pcache, &lfs->rcache, false, - commit->block, commit->off, &footer, sizeof(footer)); - if (err) { - return err; - } - commit->off += sizeof(tag)+lfs_tag_size(tag); - commit->ptag = tag ^ (reset << 31); + // build crc tag + bool reset = ~lfs_frombe32(tag) >> 31; + tag = LFS_MKTAG(LFS_TYPE_CRC + reset, 0x3ff, noff - off); - // flush buffers - err = lfs_bd_sync(lfs, &lfs->pcache, &lfs->rcache, false); - if (err) { - return err; - } - - // successful commit, check checksum to make sure - uint32_t crc = 0xffffffff; - lfs_size_t size = commit->off - lfs_tag_size(tag) - commit->begin; - for (lfs_off_t i = 0; i < size; i++) { - // leave it up to caching to make this efficient - uint8_t dat; - err = lfs_bd_read(lfs, - NULL, &lfs->rcache, size-i, - commit->block, commit->begin+i, &dat, 1); + // write out crc + uint32_t footer[2]; + footer[0] = lfs_tobe32(tag ^ commit->ptag); + commit->crc = lfs_crc(commit->crc, &footer[0], sizeof(footer[0])); + footer[1] = lfs_tole32(commit->crc); + err = lfs_bd_prog(lfs, + &lfs->pcache, &lfs->rcache, false, + commit->block, commit->off, &footer, sizeof(footer)); if (err) { return err; } - crc = lfs_crc(crc, &dat, 1); + commit->off += sizeof(tag)+lfs_tag_size(tag); + commit->ptag = tag ^ (reset << 31); + commit->crc = 0xffffffff; // reset crc for next "commit" } + // flush buffers + int err = lfs_bd_sync(lfs, &lfs->pcache, &lfs->rcache, false); if (err) { return err; } - if (crc != commit->crc) { - return LFS_ERR_CORRUPT; + // successful commit, check checksums to make sure + lfs_off_t off = commit->begin; + lfs_off_t noff = off1; + while (off < end) { + uint32_t crc = 0xffffffff; + for (lfs_off_t i = off; i < noff+sizeof(uint32_t); i++) { + // leave it up to caching to make this efficient + uint8_t dat; + err = lfs_bd_read(lfs, + NULL, &lfs->rcache, noff+sizeof(uint32_t)-i, + commit->block, i, &dat, 1); + if (err) { + return err; + } + + crc = lfs_crc(crc, &dat, 1); + } + + // detected write error? + if (crc != 0) { + return LFS_ERR_CORRUPT; + } + + // skip padding + off = lfs_min(end - noff, 0x3fe) + noff; + if (off < end) { + off = lfs_min(off, end - 2*sizeof(uint32_t)); + } + noff = off + sizeof(uint32_t); } return 0; @@ -1583,11 +1603,11 @@ static int lfs_dir_compact(lfs_t *lfs, } // successful compaction, swap dir pair to indicate most recent + LFS_ASSERT(commit.off % lfs->cfg->prog_size == 0); lfs_pair_swap(dir->pair); dir->count = end - begin; dir->off = commit.off; dir->etag = commit.ptag; - dir->erased = (dir->off % lfs->cfg->prog_size == 0); // note we able to have already handled move here if (lfs_gstate_hasmovehere(&lfs->gpending, dir->pair)) { lfs_gstate_xormove(&lfs->gpending, @@ -1758,10 +1778,9 @@ static int lfs_dir_commit(lfs_t *lfs, lfs_mdir_t *dir, } // successful commit, update dir + LFS_ASSERT(commit.off % lfs->cfg->prog_size == 0); dir->off = commit.off; dir->etag = commit.ptag; - // workaround to handle prog_size >1024 - dir->erased = (dir->off % lfs->cfg->prog_size == 0); // note we able to have already handled move here if (lfs_gstate_hasmovehere(&lfs->gpending, dir->pair)) { From 19838371fb6e24ddbcec196b2269472f5eafef7f Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Fri, 26 Jul 2019 12:29:37 -0500 Subject: [PATCH 4/4] Fixed issue where sed buffering (QUIET=1) caused Travis timeout --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 185d8e5..640dd63 100644 --- a/Makefile +++ b/Makefile @@ -46,7 +46,7 @@ test: test_format test_dirs test_files test_seek test_truncate \ test_%: tests/test_%.sh ifdef QUIET - @./$< | sed -n '/^[-=]/p' + @./$< | sed -nu '/^[-=]/p' else ./$< endif