From ab56dc5a8b9b06e3cd2b84099c140ade636f3de5 Mon Sep 17 00:00:00 2001 From: liaoweixiong Date: Fri, 15 Nov 2019 18:53:53 +0800 Subject: [PATCH 1/9] README: fix incorrect description In my point of view, file updates will commit to filesystem only when sync or close. There is a extra word 'no' here. Fixes: bdff4bc59eb6 ("Updated DESIGN.md to reflect v2 changes") Signed-off-by: liaoweixiong --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 80179833..41d58905 100644 --- a/README.md +++ b/README.md @@ -110,7 +110,7 @@ directory functions, with the deviation that the allocation of filesystem structures must be provided by the user. All POSIX operations, such as remove and rename, are atomic, even in event -of power-loss. Additionally, no file updates are not actually committed to +of power-loss. Additionally, file updates are not actually committed to the filesystem until sync or close is called on the file. ## Other notes From 5925db48da194d50c0041db4e6a7225215dc3745 Mon Sep 17 00:00:00 2001 From: Will Date: Fri, 22 Nov 2019 14:29:57 +1000 Subject: [PATCH 2/9] Fix some LFS_TRACE format specifiers --- lfs.c | 62 +++++++++++++++++++++++++++++------------------------------ 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/lfs.c b/lfs.c index 95a3b6af..6c97a51b 100644 --- a/lfs.c +++ b/lfs.c @@ -1936,7 +1936,7 @@ int lfs_dir_open(lfs_t *lfs, lfs_dir_t *dir, const char *path) { LFS_TRACE("lfs_dir_open(%p, %p, \"%s\")", (void*)lfs, (void*)dir, path); lfs_stag_t tag = lfs_dir_find(lfs, &dir->m, &path, NULL); if (tag < 0) { - LFS_TRACE("lfs_dir_open -> %d", tag); + LFS_TRACE("lfs_dir_open -> %"PRId32, tag); return tag; } @@ -1955,7 +1955,7 @@ int lfs_dir_open(lfs_t *lfs, lfs_dir_t *dir, const char *path) { lfs_stag_t res = lfs_dir_get(lfs, &dir->m, LFS_MKTAG(0x700, 0x3ff, 0), LFS_MKTAG(LFS_TYPE_STRUCT, lfs_tag_id(tag), 8), pair); if (res < 0) { - LFS_TRACE("lfs_dir_open -> %d", res); + LFS_TRACE("lfs_dir_open -> %"PRId32, res); return res; } lfs_pair_fromle32(pair); @@ -2738,14 +2738,14 @@ lfs_ssize_t lfs_file_read(lfs_t *lfs, lfs_file_t *file, // flush out any writes int err = lfs_file_flush(lfs, file); if (err) { - LFS_TRACE("lfs_file_read -> %"PRId32, err); + LFS_TRACE("lfs_file_read -> %d", err); return err; } } if (file->pos >= file->ctz.size) { // eof if past end - LFS_TRACE("lfs_file_read -> %"PRId32, 0); + LFS_TRACE("lfs_file_read -> %d", 0); return 0; } @@ -2761,7 +2761,7 @@ lfs_ssize_t lfs_file_read(lfs_t *lfs, lfs_file_t *file, file->ctz.head, file->ctz.size, file->pos, &file->block, &file->off); if (err) { - LFS_TRACE("lfs_file_read -> %"PRId32, err); + LFS_TRACE("lfs_file_read -> %d", err); return err; } } else { @@ -2781,7 +2781,7 @@ lfs_ssize_t lfs_file_read(lfs_t *lfs, lfs_file_t *file, LFS_MKTAG(LFS_TYPE_INLINESTRUCT, file->id, 0), file->off, data, diff); if (err) { - LFS_TRACE("lfs_file_read -> %"PRId32, err); + LFS_TRACE("lfs_file_read -> %d", err); return err; } } else { @@ -2789,7 +2789,7 @@ lfs_ssize_t lfs_file_read(lfs_t *lfs, lfs_file_t *file, NULL, &file->cache, lfs->cfg->block_size, file->block, file->off, data, diff); if (err) { - LFS_TRACE("lfs_file_read -> %"PRId32, err); + LFS_TRACE("lfs_file_read -> %d", err); return err; } } @@ -2818,7 +2818,7 @@ lfs_ssize_t lfs_file_write(lfs_t *lfs, lfs_file_t *file, // drop any reads int err = lfs_file_flush(lfs, file); if (err) { - LFS_TRACE("lfs_file_write -> %"PRId32, err); + LFS_TRACE("lfs_file_write -> %d", err); return err; } } @@ -2829,7 +2829,7 @@ lfs_ssize_t lfs_file_write(lfs_t *lfs, lfs_file_t *file, if (file->pos + size > lfs->file_max) { // Larger than file limit? - LFS_TRACE("lfs_file_write -> %"PRId32, LFS_ERR_FBIG); + LFS_TRACE("lfs_file_write -> %d", LFS_ERR_FBIG); return LFS_ERR_FBIG; } @@ -2855,7 +2855,7 @@ lfs_ssize_t lfs_file_write(lfs_t *lfs, lfs_file_t *file, int err = lfs_file_outline(lfs, file); if (err) { file->flags |= LFS_F_ERRED; - LFS_TRACE("lfs_file_write -> %"PRId32, err); + LFS_TRACE("lfs_file_write -> %d", err); return err; } } @@ -2872,7 +2872,7 @@ lfs_ssize_t lfs_file_write(lfs_t *lfs, lfs_file_t *file, file->pos-1, &file->block, &file->off); if (err) { file->flags |= LFS_F_ERRED; - LFS_TRACE("lfs_file_write -> %"PRId32, err); + LFS_TRACE("lfs_file_write -> %d", err); return err; } @@ -2887,7 +2887,7 @@ lfs_ssize_t lfs_file_write(lfs_t *lfs, lfs_file_t *file, &file->block, &file->off); if (err) { file->flags |= LFS_F_ERRED; - LFS_TRACE("lfs_file_write -> %"PRId32, err); + LFS_TRACE("lfs_file_write -> %d", err); return err; } } else { @@ -2908,7 +2908,7 @@ lfs_ssize_t lfs_file_write(lfs_t *lfs, lfs_file_t *file, goto relocate; } file->flags |= LFS_F_ERRED; - LFS_TRACE("lfs_file_write -> %"PRId32, err); + LFS_TRACE("lfs_file_write -> %d", err); return err; } @@ -2917,7 +2917,7 @@ relocate: err = lfs_file_relocate(lfs, file); if (err) { file->flags |= LFS_F_ERRED; - LFS_TRACE("lfs_file_write -> %"PRId32, err); + LFS_TRACE("lfs_file_write -> %d", err); return err; } } @@ -2944,7 +2944,7 @@ lfs_soff_t lfs_file_seek(lfs_t *lfs, lfs_file_t *file, // write out everything beforehand, may be noop if rdonly int err = lfs_file_flush(lfs, file); if (err) { - LFS_TRACE("lfs_file_seek -> %"PRId32, err); + LFS_TRACE("lfs_file_seek -> %d", err); return err; } @@ -2960,7 +2960,7 @@ lfs_soff_t lfs_file_seek(lfs_t *lfs, lfs_file_t *file, if (npos > lfs->file_max) { // file position out of range - LFS_TRACE("lfs_file_seek -> %"PRId32, LFS_ERR_INVAL); + LFS_TRACE("lfs_file_seek -> %d", LFS_ERR_INVAL); return LFS_ERR_INVAL; } @@ -3008,7 +3008,7 @@ int lfs_file_truncate(lfs_t *lfs, lfs_file_t *file, lfs_off_t size) { if (file->pos != oldsize) { lfs_soff_t res = lfs_file_seek(lfs, file, 0, LFS_SEEK_END); if (res < 0) { - LFS_TRACE("lfs_file_truncate -> %d", res); + LFS_TRACE("lfs_file_truncate -> %"PRId32, res); return (int)res; } } @@ -3017,7 +3017,7 @@ int lfs_file_truncate(lfs_t *lfs, lfs_file_t *file, lfs_off_t size) { while (file->pos < size) { lfs_ssize_t res = lfs_file_write(lfs, file, &(uint8_t){0}, 1); if (res < 0) { - LFS_TRACE("lfs_file_truncate -> %d", res); + LFS_TRACE("lfs_file_truncate -> %"PRId32, res); return (int)res; } } @@ -3026,7 +3026,7 @@ int lfs_file_truncate(lfs_t *lfs, lfs_file_t *file, lfs_off_t size) { // restore pos lfs_soff_t res = lfs_file_seek(lfs, file, pos, LFS_SEEK_SET); if (res < 0) { - LFS_TRACE("lfs_file_truncate -> %d", res); + LFS_TRACE("lfs_file_truncate -> %"PRId32, res); return (int)res; } @@ -3046,7 +3046,7 @@ int lfs_file_rewind(lfs_t *lfs, lfs_file_t *file) { LFS_TRACE("lfs_file_rewind(%p, %p)", (void*)lfs, (void*)file); lfs_soff_t res = lfs_file_seek(lfs, file, 0, LFS_SEEK_SET); if (res < 0) { - LFS_TRACE("lfs_file_rewind -> %d", res); + LFS_TRACE("lfs_file_rewind -> %"PRId32, res); return (int)res; } @@ -3075,7 +3075,7 @@ int lfs_stat(lfs_t *lfs, const char *path, struct lfs_info *info) { lfs_mdir_t cwd; lfs_stag_t tag = lfs_dir_find(lfs, &cwd, &path, NULL); if (tag < 0) { - LFS_TRACE("lfs_stat -> %d", tag); + LFS_TRACE("lfs_stat -> %"PRId32, tag); return (int)tag; } @@ -3096,7 +3096,7 @@ int lfs_remove(lfs_t *lfs, const char *path) { lfs_mdir_t cwd; lfs_stag_t tag = lfs_dir_find(lfs, &cwd, &path, NULL); if (tag < 0 || lfs_tag_id(tag) == 0x3ff) { - LFS_TRACE("lfs_remove -> %d", (tag < 0) ? tag : LFS_ERR_INVAL); + LFS_TRACE("lfs_remove -> %"PRId32, (tag < 0) ? tag : LFS_ERR_INVAL); return (tag < 0) ? (int)tag : LFS_ERR_INVAL; } @@ -3107,7 +3107,7 @@ int lfs_remove(lfs_t *lfs, const char *path) { lfs_stag_t res = lfs_dir_get(lfs, &cwd, LFS_MKTAG(0x700, 0x3ff, 0), LFS_MKTAG(LFS_TYPE_STRUCT, lfs_tag_id(tag), 8), pair); if (res < 0) { - LFS_TRACE("lfs_remove -> %d", res); + LFS_TRACE("lfs_remove -> %"PRId32, res); return (int)res; } lfs_pair_fromle32(pair); @@ -3170,7 +3170,7 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) { lfs_mdir_t oldcwd; lfs_stag_t oldtag = lfs_dir_find(lfs, &oldcwd, &oldpath, NULL); if (oldtag < 0 || lfs_tag_id(oldtag) == 0x3ff) { - LFS_TRACE("lfs_rename -> %d", (oldtag < 0) ? oldtag : LFS_ERR_INVAL); + LFS_TRACE("lfs_rename -> %"PRId32, (oldtag < 0) ? oldtag : LFS_ERR_INVAL); return (oldtag < 0) ? (int)oldtag : LFS_ERR_INVAL; } @@ -3180,7 +3180,7 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) { lfs_stag_t prevtag = lfs_dir_find(lfs, &newcwd, &newpath, &newid); if ((prevtag < 0 || lfs_tag_id(prevtag) == 0x3ff) && !(prevtag == LFS_ERR_NOENT && newid != 0x3ff)) { - LFS_TRACE("lfs_rename -> %d", (prevtag < 0) ? prevtag : LFS_ERR_INVAL); + LFS_TRACE("lfs_rename -> %"PRId32, (prevtag < 0) ? prevtag : LFS_ERR_INVAL); return (prevtag < 0) ? (int)prevtag : LFS_ERR_INVAL; } @@ -3201,7 +3201,7 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) { lfs_stag_t res = lfs_dir_get(lfs, &newcwd, LFS_MKTAG(0x700, 0x3ff, 0), LFS_MKTAG(LFS_TYPE_STRUCT, newid, 8), prevpair); if (res < 0) { - LFS_TRACE("lfs_rename -> %d", res); + LFS_TRACE("lfs_rename -> %"PRId32, res); return (int)res; } lfs_pair_fromle32(prevpair); @@ -3296,7 +3296,7 @@ lfs_ssize_t lfs_getattr(lfs_t *lfs, const char *path, id = 0; int err = lfs_dir_fetch(lfs, &cwd, lfs->root); if (err) { - LFS_TRACE("lfs_getattr -> %"PRId32, err); + LFS_TRACE("lfs_getattr -> %d", err); return err; } } @@ -3307,7 +3307,7 @@ lfs_ssize_t lfs_getattr(lfs_t *lfs, const char *path, buffer); if (tag < 0) { if (tag == LFS_ERR_NOENT) { - LFS_TRACE("lfs_getattr -> %"PRId32, LFS_ERR_NOATTR); + LFS_TRACE("lfs_getattr -> %d", LFS_ERR_NOATTR); return LFS_ERR_NOATTR; } @@ -3763,7 +3763,7 @@ int lfs_fs_traverse(lfs_t *lfs, if (tag == LFS_ERR_NOENT) { continue; } - LFS_TRACE("lfs_fs_traverse -> %d", tag); + LFS_TRACE("lfs_fs_traverse -> %"PRId32, tag); return tag; } lfs_ctz_fromle32(&ctz); @@ -4072,11 +4072,11 @@ lfs_ssize_t lfs_fs_size(lfs_t *lfs) { lfs_size_t size = 0; int err = lfs_fs_traverse(lfs, lfs_fs_size_count, &size); if (err) { - LFS_TRACE("lfs_fs_size -> %"PRId32, err); + LFS_TRACE("lfs_fs_size -> %d", err); return err; } - LFS_TRACE("lfs_fs_size -> %"PRId32, err); + LFS_TRACE("lfs_fs_size -> %d", err); return size; } From 64dedee2d141e5beca9080aa9a5cc53b3348af48 Mon Sep 17 00:00:00 2001 From: grunwald-m Date: Fri, 17 May 2019 13:48:37 +0200 Subject: [PATCH 3/9] prepare upstream bugfix of lfs -> call lfs_dir_fetchmatch with ftag=-1 in order to set the invalid bit and never let the function match a dir --- lfs.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lfs.c b/lfs.c index 95a3b6af..00210d74 100644 --- a/lfs.c +++ b/lfs.c @@ -975,9 +975,9 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, static int lfs_dir_fetch(lfs_t *lfs, lfs_mdir_t *dir, const lfs_block_t pair[2]) { - // note, mask=-1, tag=0 can never match a tag since this + // note, mask=-1, tag=-1 can never match a tag since this // pattern has the invalid bit set - return (int)lfs_dir_fetchmatch(lfs, dir, pair, -1, 0, NULL, NULL, NULL); + return (int)lfs_dir_fetchmatch(lfs, dir, pair, -1, -1, NULL, NULL, NULL); } static int lfs_dir_getgstate(lfs_t *lfs, const lfs_mdir_t *dir, @@ -1643,7 +1643,6 @@ relocate: if (err && (err != LFS_ERR_NOSPC && !exhausted)) { return err; } - continue; } From 60e67ae080d016ed5c6e9140acf561ba97a838fb Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 26 Nov 2019 16:42:49 -0600 Subject: [PATCH 4/9] Fixed implicit change-of-sign warning in lfs_dir_fetch Warning on MDK v5.27.1 Found by geniusgogo --- lfs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lfs.c b/lfs.c index 00210d74..24dbec31 100644 --- a/lfs.c +++ b/lfs.c @@ -977,7 +977,8 @@ static int lfs_dir_fetch(lfs_t *lfs, lfs_mdir_t *dir, const lfs_block_t pair[2]) { // note, mask=-1, tag=-1 can never match a tag since this // pattern has the invalid bit set - return (int)lfs_dir_fetchmatch(lfs, dir, pair, -1, -1, NULL, NULL, NULL); + return (int)lfs_dir_fetchmatch(lfs, dir, pair, + (lfs_tag_t)-1, (lfs_tag_t)-1, NULL, NULL, NULL); } static int lfs_dir_getgstate(lfs_t *lfs, const lfs_mdir_t *dir, From aae22c8256b2bb580cce226d320d6c74a7d8a579 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Thu, 14 Nov 2019 14:25:42 -0600 Subject: [PATCH 5/9] Fixed issue with directories falling out of date after block relocation This is caused by dir->head not being updated when dir->m.pair may be. This causes the two to fall out of sync and later dir rewinds to fail. This bug stems all the way back from the first commits of littlefs, so it's surprising it has avoided detection for this long. Perhaps because lfs_dir_rewind is not used often. --- Makefile | 1 + lfs.c | 8 +++- tests/test_relocations.sh | 88 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 2 deletions(-) create mode 100755 tests/test_relocations.sh diff --git a/Makefile b/Makefile index a22fc22d..95c26dac 100644 --- a/Makefile +++ b/Makefile @@ -55,6 +55,7 @@ test: \ test_attrs \ test_move \ test_orphan \ + test_relocations \ test_corrupt @rm test.c test_%: tests/test_%.sh diff --git a/lfs.c b/lfs.c index 95a3b6af..e71faff8 100644 --- a/lfs.c +++ b/lfs.c @@ -2103,8 +2103,6 @@ int lfs_dir_rewind(lfs_t *lfs, lfs_dir_t *dir) { return err; } - dir->m.pair[0] = dir->head[0]; - dir->m.pair[1] = dir->head[1]; dir->id = 0; dir->pos = 0; LFS_TRACE("lfs_dir_rewind -> %d", 0); @@ -3887,6 +3885,12 @@ static int lfs_fs_relocate(lfs_t *lfs, d->m.pair[0] = newpair[0]; d->m.pair[1] = newpair[1]; } + + if (d->type == LFS_TYPE_DIR && + lfs_pair_cmp(oldpair, ((lfs_dir_t*)d)->head) == 0) { + ((lfs_dir_t*)d)->head[0] = newpair[0]; + ((lfs_dir_t*)d)->head[1] = newpair[1]; + } } // find parent diff --git a/tests/test_relocations.sh b/tests/test_relocations.sh new file mode 100755 index 00000000..4a371eab --- /dev/null +++ b/tests/test_relocations.sh @@ -0,0 +1,88 @@ +#!/bin/bash +set -eu +export TEST_FILE=$0 +trap 'export TEST_LINE=$LINENO' DEBUG + +ITERATIONS=20 +COUNT=10 + +echo "=== Relocation tests ===" +rm -rf blocks +scripts/test.py << TEST + lfs_format(&lfs, &cfg) => 0; + // fill up filesystem so only ~16 blocks are left + lfs_mount(&lfs, &cfg) => 0; + lfs_file_open(&lfs, &file, "padding", LFS_O_CREAT | LFS_O_WRONLY) => 0; + memset(buffer, 0, 512); + while (LFS_BLOCK_COUNT - lfs_fs_size(&lfs) > 16) { + lfs_file_write(&lfs, &file, buffer, 512) => 512; + } + lfs_file_close(&lfs, &file) => 0; + // make a child dir to use in bounded space + lfs_mkdir(&lfs, "child") => 0; + lfs_unmount(&lfs) => 0; +TEST + +echo "--- Outdated head test ---" +scripts/test.py << TEST + lfs_mount(&lfs, &cfg) => 0; + for (int j = 0; j < $ITERATIONS; j++) { + for (int i = 0; i < $COUNT; i++) { + sprintf(path, "child/test%03d_loooooooooooooooooong_name", i); + lfs_file_open(&lfs, &file, path, LFS_O_CREAT | LFS_O_WRONLY) => 0; + lfs_file_close(&lfs, &file) => 0; + } + + lfs_dir_open(&lfs, &dir, "child") => 0; + lfs_dir_read(&lfs, &dir, &info) => 1; + lfs_dir_read(&lfs, &dir, &info) => 1; + for (int i = 0; i < $COUNT; i++) { + sprintf(path, "test%03d_loooooooooooooooooong_name", i); + lfs_dir_read(&lfs, &dir, &info) => 1; + strcmp(info.name, path) => 0; + info.size => 0; + + sprintf(path, "child/test%03d_loooooooooooooooooong_name", i); + lfs_file_open(&lfs, &file, path, LFS_O_WRONLY) => 0; + lfs_file_write(&lfs, &file, "hi", 2) => 2; + lfs_file_close(&lfs, &file) => 0; + } + lfs_dir_read(&lfs, &dir, &info) => 0; + + lfs_dir_rewind(&lfs, &dir) => 0; + lfs_dir_read(&lfs, &dir, &info) => 1; + lfs_dir_read(&lfs, &dir, &info) => 1; + for (int i = 0; i < $COUNT; i++) { + sprintf(path, "test%03d_loooooooooooooooooong_name", i); + lfs_dir_read(&lfs, &dir, &info) => 1; + strcmp(info.name, path) => 0; + info.size => 2; + + sprintf(path, "child/test%03d_loooooooooooooooooong_name", i); + lfs_file_open(&lfs, &file, path, LFS_O_WRONLY) => 0; + lfs_file_write(&lfs, &file, "hi", 2) => 2; + lfs_file_close(&lfs, &file) => 0; + } + lfs_dir_read(&lfs, &dir, &info) => 0; + + lfs_dir_rewind(&lfs, &dir) => 0; + lfs_dir_read(&lfs, &dir, &info) => 1; + lfs_dir_read(&lfs, &dir, &info) => 1; + for (int i = 0; i < $COUNT; i++) { + sprintf(path, "test%03d_loooooooooooooooooong_name", i); + lfs_dir_read(&lfs, &dir, &info) => 1; + strcmp(info.name, path) => 0; + info.size => 2; + } + lfs_dir_read(&lfs, &dir, &info) => 0; + lfs_dir_close(&lfs, &dir) => 0; + + for (int i = 0; i < $COUNT; i++) { + sprintf(path, "child/test%03d_loooooooooooooooooong_name", i); + lfs_remove(&lfs, path) => 0; + } + } + lfs_unmount(&lfs) => 0; +TEST + +scripts/results.py From aeff2a28cf871b08020d165d8e2dffa916f0ff53 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 26 Nov 2019 13:59:45 -0600 Subject: [PATCH 6/9] Stop wear-leveling during migration Stop proactively relocate blocks during migrations, this can cause a number of failure states such: clobbering the v1 superblock if we relocate root, and invalidating directory pointers if we relocate the head of a directory. On top of this, relocations increase the overall complexity of lfs_migration, which is already a delicate operation. --- lfs.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lfs.c b/lfs.c index e71faff8..a2c29dea 100644 --- a/lfs.c +++ b/lfs.c @@ -1503,9 +1503,13 @@ static int lfs_dir_compact(lfs_t *lfs, } } #ifdef LFS_MIGRATE - } else if (lfs_pair_cmp(dir->pair, lfs->root) == 0 && lfs->lfs1) { - // we can't relocate our root during migrations, as this would - // cause the superblock to get updated, which would clobber v1 + } else if (lfs->lfs1) { + // do not proactively relocate blocks during migrations, this + // can cause a number of failure states such: clobbering the + // v1 superblock if we relocate root, and invalidating directory + // pointers if we relocate the head of a directory. On top of + // this, relocations increase the overall complexity of + // lfs_migration, which is already a delicate operation. #endif } else { // we're writing too much, time to relocate From d6dc728c8752a7c314b40a988d2a3ed742eb7c84 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Thu, 14 Nov 2019 14:34:49 -0600 Subject: [PATCH 7/9] Fixed some issues in lfs_migrate - Bad size used for writing out softtail tag - Use of sizeof address instead of intended target --- lfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lfs.c b/lfs.c index a2c29dea..459085a5 100644 --- a/lfs.c +++ b/lfs.c @@ -4622,7 +4622,7 @@ int lfs_migrate(lfs_t *lfs, const struct lfs_config *cfg) { id, entry1.d.nlen), name}, {LFS_MKTAG( isdir ? LFS_TYPE_DIRSTRUCT : LFS_TYPE_CTZSTRUCT, - id, sizeof(&entry1.d.u)), &entry1.d.u})); + id, sizeof(entry1.d.u)), &entry1.d.u})); lfs1_entry_fromle32(&entry1.d); if (err) { goto cleanup; @@ -4645,7 +4645,7 @@ int lfs_migrate(lfs_t *lfs, const struct lfs_config *cfg) { lfs_pair_tole32(dir2.pair); err = lfs_dir_commit(lfs, &dir2, LFS_MKATTRS( - {LFS_MKTAG(LFS_TYPE_SOFTTAIL, 0x3ff, 0), + {LFS_MKTAG(LFS_TYPE_SOFTTAIL, 0x3ff, 8), dir1.d.tail})); lfs_pair_fromle32(dir2.pair); if (err) { From 0197b1810071d0d144a2350df9c0acee5393221e Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 27 Nov 2019 11:27:30 -0600 Subject: [PATCH 8/9] Fixed issue with superblock breaking lfs_dir_seek The superblock entry takes up id 0 in the root directory (not all entries are files, though currently the superblock is the only exception). Normally, reading a directory correctly skips the superblock and only reports non-superblock files. However, this doesn't work perfectly for lfs_dir_seek, which tries to be clever to not touch the disk. Fortunately, we can fix this by adding an offset for the superblock. This will only work while the superblock is the only non-file entry, otherwise we would need to touch the disk to properly seek in a directory (though we already touch the disk a bit to get dir-tails during seeks). Found by jhartika --- lfs.c | 14 ++++++--- tests/test_seek.sh | 74 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/lfs.c b/lfs.c index 9cfb295f..719d199c 100644 --- a/lfs.c +++ b/lfs.c @@ -2068,10 +2068,14 @@ int lfs_dir_seek(lfs_t *lfs, lfs_dir_t *dir, lfs_off_t off) { dir->pos = lfs_min(2, off); off -= dir->pos; - while (off != 0) { - dir->id = lfs_min(dir->m.count, off); - dir->pos += dir->id; - off -= dir->id; + // skip superblock entry + dir->id = (off > 0 && lfs_pair_cmp(dir->head, lfs->root) == 0); + + while (off > 0) { + int diff = lfs_min(dir->m.count - dir->id, off); + dir->id += diff; + dir->pos += diff; + off -= diff; if (dir->id == dir->m.count) { if (!dir->m.split) { @@ -2084,6 +2088,8 @@ int lfs_dir_seek(lfs_t *lfs, lfs_dir_t *dir, lfs_off_t off) { LFS_TRACE("lfs_dir_seek -> %d", err); return err; } + + dir->id = 0; } } diff --git a/tests/test_seek.sh b/tests/test_seek.sh index e5696d05..e136aa0d 100755 --- a/tests/test_seek.sh +++ b/tests/test_seek.sh @@ -428,4 +428,78 @@ scripts/test.py << TEST TEST done +echo "--- Root seek test ---" +./scripts/test.py << TEST + lfs_mount(&lfs, &cfg) => 0; + for (int i = 3; i < $MEDIUMSIZE; i++) { + sprintf(path, "hi%03d", i); + lfs_mkdir(&lfs, path) => 0; + } + + lfs_dir_open(&lfs, &dir, "/") => 0; + for (int i = 0; i < $MEDIUMSIZE; i++) { + if (i == 0) { + sprintf(path, "."); + } else if (i == 1) { + sprintf(path, ".."); + } else if (i == 2) { + sprintf(path, "hello"); + } else { + sprintf(path, "hi%03d", i); + } + lfs_dir_read(&lfs, &dir, &info) => 1; + strcmp(path, info.name) => 0; + } + lfs_dir_read(&lfs, &dir, &info) => 0; + lfs_dir_close(&lfs, &dir) => 0; + + for (int j = 0; j < $MEDIUMSIZE; j++) { + lfs_soff_t off = -1; + + lfs_dir_open(&lfs, &dir, "/") => 0; + for (int i = 0; i < $MEDIUMSIZE; i++) { + if (i == 0) { + sprintf(path, "."); + } else if (i == 1) { + sprintf(path, ".."); + } else if (i == 2) { + sprintf(path, "hello"); + } else { + sprintf(path, "hi%03d", i); + } + + if (i == j) { + off = lfs_dir_tell(&lfs, &dir); + off >= 0 => true; + } + + lfs_dir_read(&lfs, &dir, &info) => 1; + strcmp(path, info.name) => 0; + } + lfs_dir_read(&lfs, &dir, &info) => 0; + lfs_dir_close(&lfs, &dir) => 0; + + lfs_dir_open(&lfs, &dir, "/") => 0; + lfs_dir_seek(&lfs, &dir, off) => 0; + for (int i = j; i < $MEDIUMSIZE; i++) { + if (i == 0) { + sprintf(path, "."); + } else if (i == 1) { + sprintf(path, ".."); + } else if (i == 2) { + sprintf(path, "hello"); + } else { + sprintf(path, "hi%03d", i); + } + + lfs_dir_read(&lfs, &dir, &info) => 1; + strcmp(path, info.name) => 0; + } + lfs_dir_read(&lfs, &dir, &info) => 0; + lfs_dir_close(&lfs, &dir) => 0; + } + + lfs_unmount(&lfs) => 0; +TEST + scripts/results.py From ce2c01f098f4d2b9479de5a796c3bb531f1fe14c Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 26 Nov 2019 01:21:42 -0600 Subject: [PATCH 9/9] Fixed lfs_dir_fetchmatch not understanding overwritten tags Sometimes small, single line code change hides behind it a complicated story. This is one of those times. If you look at this diff, you may note that this is a case of lfs_dir_fetchmatch not correctly handling a tag that invalidates a callback used to search for some condition, in this case a search for a parent, which is invalidated by a later dir tag overwritting the previous dir pair. But how can this happen? Dir-pair-tags are only overwritten during relocations (when a block goes bad or exceeds the block_cycles config option for dynamic wear-leveling). Other dir operations create new directory entries. And the only lfs_dir_fetchmatch condition that relies on overwrites (as opposed to proper deletes) is when we need to find a directory's parent, an operation that only occurs during a _different_ relocation. And a false _positive_, can only happen if we don't have a parent. Which is really unlikely when we search for directory parents! This bug and minimal test case was found by Matthew Renzelmann. In a unfortunate series of events, first a file creation causes a directory split to occur. This creates a new, orphaned metadata-pair containing our new file. However, the revision count on this metadata-pair indicates the pair is due for relocation as a part of wear-leveling. Normally, this is fine, even though this metadata-pair has no parent, the lfs_dir_find should return ENOENT and continue without error. However, here we get hit by our fetchmatch bug. A previous, unrelated relocation overwrites a pair which just happens to contain the block allocated for a new metadata-pair. When we search for a parent, lfs_dir_fetchmatch incorrectly finds this old, outdated metadata pair and incorrectly tells our orphan it's found its parent. As you can imagine the orphan's dissapointment must be immense. So an unfortunately timed dir split triggers a relocation which incorrectly finds a previously written parent that has been outdated by another relocation. As a solution we can outdate our found tag if it is overwritten by an exact match during lfs_dir_fetchmatch. As a part of this I started adding a new set of tests: tests/test_relocations, for aggressive relocations tests. This is already by appended to by another PR. I suspect relocations is relatively under-tested and is becoming more important due to recent improvements in wear-leveling. --- .travis.yml | 1 + lfs.c | 6 +++++ tests/test_alloc.sh | 10 ++++++++ tests/test_relocations.sh | 51 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+) diff --git a/.travis.yml b/.travis.yml index 880c861f..42e59dc5 100644 --- a/.travis.yml +++ b/.travis.yml @@ -20,6 +20,7 @@ script: # run tests with a few different configurations - make test QUIET=1 CFLAGS+="-DLFS_READ_SIZE=1 -DLFS_CACHE_SIZE=4" - make test QUIET=1 CFLAGS+="-DLFS_READ_SIZE=512 -DLFS_CACHE_SIZE=512 -DLFS_BLOCK_CYCLES=16" + - make test QUIET=1 CFLAGS+="-DLFS_READ_SIZE=8 -DLFS_CACHE_SIZE=16 -DLFS_BLOCK_CYCLES=2" - make test QUIET=1 CFLAGS+="-DLFS_BLOCK_COUNT=1023 -DLFS_LOOKAHEAD_SIZE=256" - make clean test QUIET=1 CFLAGS+="-DLFS_INLINE_MAX=0" diff --git a/lfs.c b/lfs.c index 719d199c..4553a6ed 100644 --- a/lfs.c +++ b/lfs.c @@ -929,6 +929,11 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, if (res == LFS_CMP_EQ) { // found a match tempbesttag = tag; + } else if ((LFS_MKTAG(0x7ff, 0x3ff, 0) & tag) == + (LFS_MKTAG(0x7ff, 0x3ff, 0) & tempbesttag)) { + // found an identical tag, but contents didn't match + // this must mean that our besttag has been overwritten + tempbesttag = -1; } else if (res == LFS_CMP_GT && lfs_tag_id(tag) <= lfs_tag_id(tempbesttag)) { // found a greater match, keep track to keep things sorted @@ -1378,6 +1383,7 @@ static int lfs_dir_split(lfs_t *lfs, lfs_mdir_t *dir, const struct lfs_mattr *attrs, int attrcount, lfs_mdir_t *source, uint16_t split, uint16_t end) { // create tail directory + lfs_alloc_ack(lfs); lfs_mdir_t tail; int err = lfs_dir_alloc(lfs, &tail); if (err) { diff --git a/tests/test_alloc.sh b/tests/test_alloc.sh index d9f233ba..06698508 100755 --- a/tests/test_alloc.sh +++ b/tests/test_alloc.sh @@ -250,6 +250,14 @@ scripts/test.py << TEST lfs_unmount(&lfs) => 0; TEST +## Below, these tests depend _very_ heavily on the geometry of the +## block device being tested, they should be removed and replaced +## by generalized tests. For now we'll just skip if the geometry +## is customized. + +if [[ ! $MAKEFLAGS =~ "LFS_BLOCK_CYCLES" ]] +then + echo "--- Chained dir exhaustion test ---" scripts/test.py << TEST lfs_mount(&lfs, &cfg) => 0; @@ -481,4 +489,6 @@ scripts/test.py << TEST lfs_unmount(&lfs) => 0; TEST +fi + scripts/results.py diff --git a/tests/test_relocations.sh b/tests/test_relocations.sh index 4a371eab..5244e5ea 100755 --- a/tests/test_relocations.sh +++ b/tests/test_relocations.sh @@ -23,6 +23,57 @@ scripts/test.py << TEST lfs_unmount(&lfs) => 0; TEST +echo "--- Dangling split dir test ---" +scripts/test.py << TEST + lfs_mount(&lfs, &cfg) => 0; + for (int j = 0; j < $ITERATIONS; j++) { + for (int i = 0; i < $COUNT; i++) { + sprintf(path, "child/test%03d_loooooooooooooooooong_name", i); + lfs_file_open(&lfs, &file, path, LFS_O_CREAT | LFS_O_WRONLY) => 0; + lfs_file_close(&lfs, &file) => 0; + } + + lfs_dir_open(&lfs, &dir, "child") => 0; + lfs_dir_read(&lfs, &dir, &info) => 1; + lfs_dir_read(&lfs, &dir, &info) => 1; + for (int i = 0; i < $COUNT; i++) { + sprintf(path, "test%03d_loooooooooooooooooong_name", i); + lfs_dir_read(&lfs, &dir, &info) => 1; + strcmp(info.name, path) => 0; + } + lfs_dir_read(&lfs, &dir, &info) => 0; + lfs_dir_close(&lfs, &dir) => 0; + + if (j == $ITERATIONS-1) { + break; + } + + for (int i = 0; i < $COUNT; i++) { + sprintf(path, "child/test%03d_loooooooooooooooooong_name", i); + lfs_remove(&lfs, path) => 0; + } + } + lfs_unmount(&lfs) => 0; +TEST +scripts/test.py << TEST + lfs_mount(&lfs, &cfg) => 0; + lfs_dir_open(&lfs, &dir, "child") => 0; + lfs_dir_read(&lfs, &dir, &info) => 1; + lfs_dir_read(&lfs, &dir, &info) => 1; + for (int i = 0; i < $COUNT; i++) { + sprintf(path, "test%03d_loooooooooooooooooong_name", i); + lfs_dir_read(&lfs, &dir, &info) => 1; + strcmp(info.name, path) => 0; + } + lfs_dir_read(&lfs, &dir, &info) => 0; + lfs_dir_close(&lfs, &dir) => 0; + for (int i = 0; i < $COUNT; i++) { + sprintf(path, "child/test%03d_loooooooooooooooooong_name", i); + lfs_remove(&lfs, path) => 0; + } + lfs_unmount(&lfs) => 0; +TEST + echo "--- Outdated head test ---" scripts/test.py << TEST lfs_mount(&lfs, &cfg) => 0;