From 6bb404315405ba6f5217c061c0e3fb2a0d19099b Mon Sep 17 00:00:00 2001 From: Themba Dube Date: Thu, 24 Dec 2020 14:05:46 -0500 Subject: [PATCH 1/4] Skip flushing file if lfs_file_rawseek() doesn't change position --- lfs.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/lfs.c b/lfs.c index d7439fe..c59d3d2 100644 --- a/lfs.c +++ b/lfs.c @@ -3048,14 +3048,6 @@ relocate: static lfs_soff_t lfs_file_rawseek(lfs_t *lfs, lfs_file_t *file, lfs_soff_t off, int whence) { -#ifndef LFS_READONLY - // write out everything beforehand, may be noop if rdonly - int err = lfs_file_flush(lfs, file); - if (err) { - return err; - } -#endif - // find new pos lfs_off_t npos = file->pos; if (whence == LFS_SEEK_SET) { @@ -3071,6 +3063,19 @@ static lfs_soff_t lfs_file_rawseek(lfs_t *lfs, lfs_file_t *file, return LFS_ERR_INVAL; } + if (file->pos == npos) { + // noop - position has not changed + return npos; + } + +#ifndef LFS_READONLY + // write out everything beforehand, may be noop if rdonly + int err = lfs_file_flush(lfs, file); + if (err) { + return err; + } +#endif + // update pos file->pos = npos; return npos; From 3216b07c3bce220115ea8c5c8b3eb1e452bf6de0 Mon Sep 17 00:00:00 2001 From: Themba Dube Date: Wed, 6 Jan 2021 11:20:41 -0500 Subject: [PATCH 2/4] Use lfs_file_rawsize to calculate LFS_SEEK_END position --- lfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lfs.c b/lfs.c index c59d3d2..281f138 100644 --- a/lfs.c +++ b/lfs.c @@ -3055,7 +3055,7 @@ static lfs_soff_t lfs_file_rawseek(lfs_t *lfs, lfs_file_t *file, } else if (whence == LFS_SEEK_CUR) { npos = file->pos + off; } else if (whence == LFS_SEEK_END) { - npos = file->ctz.size + off; + npos = lfs_file_rawsize(lfs, file) + off; } if (npos > lfs->file_max) { From 745d98cde08850cd0322daed86e5584a891095eb Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 11 Jan 2021 00:01:05 -0600 Subject: [PATCH 3/4] Fixed lfs_file_truncate issue where internal state may not be flushed This was caused by the new lfs_file_rawseek optimization that can skip flushing when calculated file->pos is unchanged combined with an implicit expectation in lfs_file_truncate that lfs_file_rawseek unconditionally sets file->pos. Because of this assumption, lfs_file_truncate could leave file->pos in an outdated state while changing the internal file metadata. Humorously, this was always gauranteed to trigger the skip in lfs_file_rawseek when we try to restore the file->pos, leaving the file->cache used to do the CTZ skip-list lookup in a potentially bad state. The easiest fix is to just update file->pos correctly. Note we don't want to explicitly flush since we can leverage the same noop optimization if we truncate to the file position. Which I've added a test for. --- lfs.c | 3 +++ tests/test_truncate.toml | 45 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/lfs.c b/lfs.c index 281f138..f54aa04 100644 --- a/lfs.c +++ b/lfs.c @@ -3106,6 +3106,9 @@ static int lfs_file_rawtruncate(lfs_t *lfs, lfs_file_t *file, lfs_off_t size) { return err; } + // need to set pos/block/off consistently so seeking back to + // the old position does not get confused + file->pos = size; file->ctz.head = file->block; file->ctz.size = size; file->flags |= LFS_F_DIRTY | LFS_F_READING; diff --git a/tests/test_truncate.toml b/tests/test_truncate.toml index c11285b..850d7aa 100644 --- a/tests/test_truncate.toml +++ b/tests/test_truncate.toml @@ -392,3 +392,48 @@ code = ''' lfs_unmount(&lfs) => 0; ''' + +[[case]] # noop truncate +define.MEDIUMSIZE = [32, 2048] +code = ''' + lfs_format(&lfs, &cfg) => 0; + lfs_mount(&lfs, &cfg) => 0; + lfs_file_open(&lfs, &file, "baldynoop", + LFS_O_RDWR | LFS_O_CREAT) => 0; + + strcpy((char*)buffer, "hair"); + size = strlen((char*)buffer); + for (lfs_off_t j = 0; j < MEDIUMSIZE; j += size) { + lfs_file_write(&lfs, &file, buffer, size) => size; + + // this truncate should do nothing + lfs_file_truncate(&lfs, &file, j+size) => 0; + } + lfs_file_size(&lfs, &file) => MEDIUMSIZE; + + lfs_file_seek(&lfs, &file, 0, LFS_SEEK_SET) => 0; + // should do nothing again + lfs_file_truncate(&lfs, &file, MEDIUMSIZE) => 0; + lfs_file_size(&lfs, &file) => MEDIUMSIZE; + + for (lfs_off_t j = 0; j < MEDIUMSIZE; j += size) { + lfs_file_read(&lfs, &file, buffer, size) => size; + memcmp(buffer, "hair", size) => 0; + } + lfs_file_read(&lfs, &file, buffer, size) => 0; + + lfs_file_close(&lfs, &file) => 0; + lfs_unmount(&lfs) => 0; + + // still there after reboot? + lfs_mount(&lfs, &cfg) => 0; + lfs_file_open(&lfs, &file, "baldynoop", LFS_O_RDWR) => 0; + lfs_file_size(&lfs, &file) => MEDIUMSIZE; + for (lfs_off_t j = 0; j < MEDIUMSIZE; j += size) { + lfs_file_read(&lfs, &file, buffer, size) => size; + memcmp(buffer, "hair", size) => 0; + } + lfs_file_read(&lfs, &file, buffer, size) => 0; + lfs_file_close(&lfs, &file) => 0; + lfs_unmount(&lfs) => 0; +''' From 47d6b2fcf34c231c491ae4a81dda2b3787f1156b Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 11 Jan 2021 00:13:18 -0600 Subject: [PATCH 4/4] Removed unnecessary truncate condition thanks to new seek optimization --- lfs.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lfs.c b/lfs.c index f54aa04..7b7d5b6 100644 --- a/lfs.c +++ b/lfs.c @@ -3114,16 +3114,14 @@ static int lfs_file_rawtruncate(lfs_t *lfs, lfs_file_t *file, lfs_off_t size) { file->flags |= LFS_F_DIRTY | LFS_F_READING; } else if (size > oldsize) { // flush+seek if not already at end - if (file->pos != oldsize) { - lfs_soff_t res = lfs_file_rawseek(lfs, file, 0, LFS_SEEK_END); - if (res < 0) { - return (int)res; - } + lfs_soff_t res = lfs_file_rawseek(lfs, file, 0, LFS_SEEK_END); + if (res < 0) { + return (int)res; } // fill with zeros while (file->pos < size) { - lfs_ssize_t res = lfs_file_rawwrite(lfs, file, &(uint8_t){0}, 1); + res = lfs_file_rawwrite(lfs, file, &(uint8_t){0}, 1); if (res < 0) { return (int)res; }