From f2a6f45eeff23e37ca43e87b7d2e795c2f20c306 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 27 Feb 2024 12:58:07 -0600 Subject: [PATCH 1/4] Added out-of-order write testing to emubd Some forms of storage, mainly anything with an FTL, eMMC, SD, etc, do not guarantee a strict write order for writes to different blocks. It would be good to test that this doesn't break littlefs. This adds LFS_EMUBD_POWERLOSS_OOO to lfs_emubd, which tells lfs_emubd to try to break any order-dependent code on powerloss. The behavior right now is a bit simple, but does result in test breakage: 1. Save the state of the block on first write (erase really) after sync/init. 2. On powerloss, revert the first write to its original state. This might be a bit confusing when debugging, since the block will appear to time-travel, but doing anything fancier would make emubd quite a bit more complicated. You could also get a bit fancier with which/how many blocks to revert, but this should at least be sufficient to make sure bd sync calls are in the right place. --- bd/lfs_emubd.c | 79 ++++++++++++++++++++++++++++++++---- bd/lfs_emubd.h | 15 ++++--- tests/test_dirs.toml | 8 ++++ tests/test_files.toml | 8 ++++ tests/test_interspersed.toml | 4 ++ tests/test_move.toml | 8 ++++ tests/test_seek.toml | 4 ++ tests/test_superblocks.toml | 8 ++++ tests/test_truncate.toml | 4 ++ 9 files changed, 125 insertions(+), 13 deletions(-) diff --git a/bd/lfs_emubd.c b/bd/lfs_emubd.c index 9d88d053..2d2c168b 100644 --- a/bd/lfs_emubd.c +++ b/bd/lfs_emubd.c @@ -129,6 +129,8 @@ int lfs_emubd_create(const struct lfs_config *cfg, bd->proged = 0; bd->erased = 0; bd->power_cycles = bd->cfg->power_cycles; + bd->ooo_block = -1; + bd->ooo_data = NULL; bd->disk = NULL; if (bd->cfg->disk_path) { @@ -195,6 +197,7 @@ int lfs_emubd_destroy(const struct lfs_config *cfg) { free(bd->blocks); // clean up other resources + lfs_emubd_decblock(bd->ooo_data); if (bd->disk) { bd->disk->rc -= 1; if (bd->disk->rc == 0) { @@ -209,6 +212,47 @@ int lfs_emubd_destroy(const struct lfs_config *cfg) { } +// powerloss hook +static int lfs_emubd_powerloss(const struct lfs_config *cfg) { + lfs_emubd_t *bd = cfg->context; + + // emulate out-of-order writes? + if (bd->cfg->powerloss_behavior == LFS_EMUBD_POWERLOSS_OOO + && bd->ooo_block != -1) { + // since writes between syncs are allowed to be out-of-order, it + // shouldn't hurt to restore the first write on powerloss, right? + lfs_emubd_decblock(bd->blocks[bd->ooo_block]); + bd->blocks[bd->ooo_block] = bd->ooo_data; + + // mirror to disk file? + if (bd->disk && (bd->ooo_data || bd->cfg->erase_value != -1)) { + off_t res1 = lseek(bd->disk->fd, + (off_t)bd->ooo_block*bd->cfg->erase_size, + SEEK_SET); + if (res1 < 0) { + return -errno; + } + + ssize_t res2 = write(bd->disk->fd, + (bd->ooo_data) + ? bd->ooo_data->data + : bd->disk->scratch, + bd->cfg->erase_size); + if (res2 < 0) { + return -errno; + } + } + + bd->ooo_block = -1; + bd->ooo_data = NULL; + } + + // simulate power loss + bd->cfg->powerloss_cb(bd->cfg->powerloss_data); + + return 0; +} + // block device API @@ -344,8 +388,11 @@ int lfs_emubd_prog(const struct lfs_config *cfg, lfs_block_t block, if (bd->power_cycles > 0) { bd->power_cycles -= 1; if (bd->power_cycles == 0) { - // simulate power loss - bd->cfg->powerloss_cb(bd->cfg->powerloss_data); + int err = lfs_emubd_powerloss(cfg); + if (err) { + LFS_EMUBD_TRACE("lfs_emubd_prog -> %d", err); + return err; + } } } @@ -361,10 +408,17 @@ int lfs_emubd_erase(const struct lfs_config *cfg, lfs_block_t block) { // check if erase is valid LFS_ASSERT(block < bd->cfg->erase_count); + // emulate out-of-order writes? save first write + if (bd->cfg->powerloss_behavior == LFS_EMUBD_POWERLOSS_OOO + && bd->ooo_block == -1) { + bd->ooo_block = block; + bd->ooo_data = lfs_emubd_incblock(bd->blocks[block]); + } + // get the block lfs_emubd_block_t *b = lfs_emubd_mutblock(cfg, &bd->blocks[block]); if (!b) { - LFS_EMUBD_TRACE("lfs_emubd_prog -> %d", LFS_ERR_NOMEM); + LFS_EMUBD_TRACE("lfs_emubd_erase -> %d", LFS_ERR_NOMEM); return LFS_ERR_NOMEM; } @@ -430,8 +484,11 @@ int lfs_emubd_erase(const struct lfs_config *cfg, lfs_block_t block) { if (bd->power_cycles > 0) { bd->power_cycles -= 1; if (bd->power_cycles == 0) { - // simulate power loss - bd->cfg->powerloss_cb(bd->cfg->powerloss_data); + int err = lfs_emubd_powerloss(cfg); + if (err) { + LFS_EMUBD_TRACE("lfs_emubd_erase -> %d", err); + return err; + } } } @@ -441,14 +498,20 @@ int lfs_emubd_erase(const struct lfs_config *cfg, lfs_block_t block) { int lfs_emubd_sync(const struct lfs_config *cfg) { LFS_EMUBD_TRACE("lfs_emubd_sync(%p)", (void*)cfg); + lfs_emubd_t *bd = cfg->context; - // do nothing - (void)cfg; + // emulate out-of-order writes? reset first write, writes + // cannot be out-of-order across sync + if (bd->cfg->powerloss_behavior == LFS_EMUBD_POWERLOSS_OOO) { + bd->ooo_block = -1; + bd->ooo_data = NULL; + } LFS_EMUBD_TRACE("lfs_emubd_sync -> %d", 0); return 0; } + /// Additional extended API for driving test features /// static int lfs_emubd_crc_(const struct lfs_config *cfg, @@ -633,6 +696,8 @@ int lfs_emubd_copy(const struct lfs_config *cfg, lfs_emubd_t *copy) { copy->proged = bd->proged; copy->erased = bd->erased; copy->power_cycles = bd->power_cycles; + copy->ooo_block = bd->ooo_block; + copy->ooo_data = lfs_emubd_incblock(bd->ooo_data); copy->disk = bd->disk; if (copy->disk) { copy->disk->rc += 1; diff --git a/bd/lfs_emubd.h b/bd/lfs_emubd.h index 9049649f..90600083 100644 --- a/bd/lfs_emubd.h +++ b/bd/lfs_emubd.h @@ -36,17 +36,18 @@ extern "C" // Not that read-noop is not allowed. Read _must_ return a consistent (but // may be arbitrary) value on every read. typedef enum lfs_emubd_badblock_behavior { - LFS_EMUBD_BADBLOCK_PROGERROR, - LFS_EMUBD_BADBLOCK_ERASEERROR, - LFS_EMUBD_BADBLOCK_READERROR, - LFS_EMUBD_BADBLOCK_PROGNOOP, - LFS_EMUBD_BADBLOCK_ERASENOOP, + LFS_EMUBD_BADBLOCK_PROGERROR = 0, // Error on prog + LFS_EMUBD_BADBLOCK_ERASEERROR = 1, // Error on erase + LFS_EMUBD_BADBLOCK_READERROR = 2, // Error on read + LFS_EMUBD_BADBLOCK_PROGNOOP = 3, // Prog does nothing silently + LFS_EMUBD_BADBLOCK_ERASENOOP = 4, // Erase does nothing silently } lfs_emubd_badblock_behavior_t; // Mode determining how power-loss behaves during testing. For now this // only supports a noop behavior, leaving the data on-disk untouched. typedef enum lfs_emubd_powerloss_behavior { - LFS_EMUBD_POWERLOSS_NOOP, + LFS_EMUBD_POWERLOSS_NOOP = 0, // Progs are atomic + LFS_EMUBD_POWERLOSS_OOO = 1, // Blocks are written out-of-order } lfs_emubd_powerloss_behavior_t; // Type for measuring read/program/erase operations @@ -152,6 +153,8 @@ typedef struct lfs_emubd { lfs_emubd_io_t proged; lfs_emubd_io_t erased; lfs_emubd_powercycles_t power_cycles; + lfs_ssize_t ooo_block; + lfs_emubd_block_t *ooo_data; lfs_emubd_disk_t *disk; const struct lfs_emubd_config *cfg; diff --git a/tests/test_dirs.toml b/tests/test_dirs.toml index 181dd6ad..cb1f2e94 100644 --- a/tests/test_dirs.toml +++ b/tests/test_dirs.toml @@ -181,6 +181,10 @@ code = ''' defines.N = [5, 11] if = 'BLOCK_COUNT >= 4*N' reentrant = true +defines.POWERLOSS_BEHAVIOR = [ + 'LFS_EMUBD_POWERLOSS_NOOP', + 'LFS_EMUBD_POWERLOSS_OOO', +] code = ''' lfs_t lfs; int err = lfs_mount(&lfs, cfg); @@ -439,6 +443,10 @@ code = ''' defines.N = [5, 25] if = 'N < BLOCK_COUNT/2' reentrant = true +defines.POWERLOSS_BEHAVIOR = [ + 'LFS_EMUBD_POWERLOSS_NOOP', + 'LFS_EMUBD_POWERLOSS_OOO', +] code = ''' lfs_t lfs; int err = lfs_mount(&lfs, cfg); diff --git a/tests/test_files.toml b/tests/test_files.toml index 1c86cd8d..1ef41d47 100644 --- a/tests/test_files.toml +++ b/tests/test_files.toml @@ -310,6 +310,10 @@ defines.SIZE = [32, 0, 7, 2049] defines.CHUNKSIZE = [31, 16, 65] defines.INLINE_MAX = [0, -1, 8] reentrant = true +defines.POWERLOSS_BEHAVIOR = [ + 'LFS_EMUBD_POWERLOSS_NOOP', + 'LFS_EMUBD_POWERLOSS_OOO', +] code = ''' lfs_t lfs; int err = lfs_mount(&lfs, cfg); @@ -500,6 +504,10 @@ code = ''' [cases.test_files_many_power_loss] defines.N = 300 reentrant = true +defines.POWERLOSS_BEHAVIOR = [ + 'LFS_EMUBD_POWERLOSS_NOOP', + 'LFS_EMUBD_POWERLOSS_OOO', +] code = ''' lfs_t lfs; int err = lfs_mount(&lfs, cfg); diff --git a/tests/test_interspersed.toml b/tests/test_interspersed.toml index d7143f61..8c834010 100644 --- a/tests/test_interspersed.toml +++ b/tests/test_interspersed.toml @@ -195,6 +195,10 @@ code = ''' defines.SIZE = [10, 100] defines.FILES = [4, 10, 26] reentrant = true +defines.POWERLOSS_BEHAVIOR = [ + 'LFS_EMUBD_POWERLOSS_NOOP', + 'LFS_EMUBD_POWERLOSS_OOO', +] code = ''' lfs_t lfs; lfs_file_t files[FILES]; diff --git a/tests/test_move.toml b/tests/test_move.toml index 0537f486..75643908 100644 --- a/tests/test_move.toml +++ b/tests/test_move.toml @@ -357,6 +357,10 @@ code = ''' [cases.test_move_reentrant_file] reentrant = true +defines.POWERLOSS_BEHAVIOR = [ + 'LFS_EMUBD_POWERLOSS_NOOP', + 'LFS_EMUBD_POWERLOSS_OOO', +] code = ''' lfs_t lfs; int err = lfs_mount(&lfs, cfg); @@ -839,6 +843,10 @@ code = ''' [cases.test_reentrant_dir] reentrant = true +defines.POWERLOSS_BEHAVIOR = [ + 'LFS_EMUBD_POWERLOSS_NOOP', + 'LFS_EMUBD_POWERLOSS_OOO', +] code = ''' lfs_t lfs; int err = lfs_mount(&lfs, cfg); diff --git a/tests/test_seek.toml b/tests/test_seek.toml index b976057b..9b3768d7 100644 --- a/tests/test_seek.toml +++ b/tests/test_seek.toml @@ -329,6 +329,10 @@ code = ''' # must be power-of-2 for quadratic probing to be exhaustive defines.COUNT = [4, 64, 128] reentrant = true +defines.POWERLOSS_BEHAVIOR = [ + 'LFS_EMUBD_POWERLOSS_NOOP', + 'LFS_EMUBD_POWERLOSS_OOO', +] code = ''' lfs_t lfs; int err = lfs_mount(&lfs, cfg); diff --git a/tests/test_superblocks.toml b/tests/test_superblocks.toml index 6d4a3e4f..a7c2aff3 100644 --- a/tests/test_superblocks.toml +++ b/tests/test_superblocks.toml @@ -32,6 +32,10 @@ code = ''' # reentrant format [cases.test_superblocks_reentrant_format] reentrant = true +defines.POWERLOSS_BEHAVIOR = [ + 'LFS_EMUBD_POWERLOSS_NOOP', + 'LFS_EMUBD_POWERLOSS_OOO', +] code = ''' lfs_t lfs; int err = lfs_mount(&lfs, cfg); @@ -174,6 +178,10 @@ code = ''' defines.BLOCK_CYCLES = [2, 1] defines.N = 24 reentrant = true +defines.POWERLOSS_BEHAVIOR = [ + 'LFS_EMUBD_POWERLOSS_NOOP', + 'LFS_EMUBD_POWERLOSS_OOO', +] code = ''' lfs_t lfs; int err = lfs_mount(&lfs, cfg); diff --git a/tests/test_truncate.toml b/tests/test_truncate.toml index 2f10e952..8114a118 100644 --- a/tests/test_truncate.toml +++ b/tests/test_truncate.toml @@ -231,6 +231,10 @@ defines.SMALLSIZE = [4, 512] defines.MEDIUMSIZE = [0, 3, 4, 5, 31, 32, 33, 511, 512, 513, 1023, 1024, 1025] defines.LARGESIZE = 2048 reentrant = true +defines.POWERLOSS_BEHAVIOR = [ + 'LFS_EMUBD_POWERLOSS_NOOP', + 'LFS_EMUBD_POWERLOSS_OOO', +] code = ''' lfs_t lfs; int err = lfs_mount(&lfs, cfg); From 635218594971434028cff62cbbd484b8b2ab7fae Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 27 Feb 2024 14:00:10 -0600 Subject: [PATCH 2/4] Fixed sync issue where data writes could appear before metadata writes Long story short we aren't calling sync correctly in littlefs. This fixes that. Some forms of storage, mainly anything with an FTL, eMMC, SD, etc, do not guarantee a strict write order for writes to different blocks. In theory this is what bd sync is for, to tell the bd when it is important for the writes to be ordered. Currently, littlefs calls bd sync after committing metadata. This is useful as it ensures that user code can rely on lfs_file_sync for ordering external side-effects. But this is insufficient for handling storage with out-of-order writes. Consider the simple case of a file with one data block: 1. lfs_file_write(blablabla) => writes data into a new data block 2. lfs_file_sync() => commits metadata to point to the new data block But with out-of-order writes, the bd is free to reorder things such that the metadata is updated _before_ the data is written. If we lose power, that would be bad. The solution to this is to call bd sync twice: Once before we commit the metadata to tell the bd that these writes must be ordered, and once after we commit the metadata to allow ordering with user code. As a small optimization, we only call bd sync if the current file is not inlined and has actually been modified (LFS_F_DIRTY). It's possible for inlined files to be interleaved with writes to other files. Found by MFaehling and alex31 --- lfs.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lfs.c b/lfs.c index d5885ca0..6e49851d 100644 --- a/lfs.c +++ b/lfs.c @@ -3401,6 +3401,15 @@ static int lfs_file_sync_(lfs_t *lfs, lfs_file_t *file) { if ((file->flags & LFS_F_DIRTY) && !lfs_pair_isnull(file->m.pair)) { + // before we commit metadata, we need sync the disk to make sure + // data writes don't complete after metadata writes + if (!(file->flags & LFS_F_INLINE)) { + err = lfs_bd_sync(lfs, &lfs->pcache, &lfs->rcache, false); + if (err) { + return err; + } + } + // update dir entry uint16_t type; const void *buffer; From fc2aa3350c1161dcb224733de636947cace0d1f5 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 27 Feb 2024 21:14:59 -0600 Subject: [PATCH 3/4] Fixed issue with exhaustive + out-of-order powerloss testing Unlike the heuristic based testing, exhaustive powerloss testing effectively forks the current test and runs both the interrupted and uninterrupted test states to completion. But emubd wasn't expecting bd->cfg->powerloss_cb to return. The fix here is to keep track to both the old+new out-of-order block states and unrevert them if bd->cfg->powerloss_cb returns. This may leak the temporary copy, but powerloss testing is already inherently leaky. --- bd/lfs_emubd.c | 44 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/bd/lfs_emubd.c b/bd/lfs_emubd.c index 2d2c168b..3f6183c9 100644 --- a/bd/lfs_emubd.c +++ b/bd/lfs_emubd.c @@ -217,15 +217,18 @@ static int lfs_emubd_powerloss(const struct lfs_config *cfg) { lfs_emubd_t *bd = cfg->context; // emulate out-of-order writes? + lfs_emubd_block_t *ooo_data = NULL; if (bd->cfg->powerloss_behavior == LFS_EMUBD_POWERLOSS_OOO && bd->ooo_block != -1) { // since writes between syncs are allowed to be out-of-order, it // shouldn't hurt to restore the first write on powerloss, right? - lfs_emubd_decblock(bd->blocks[bd->ooo_block]); - bd->blocks[bd->ooo_block] = bd->ooo_data; + ooo_data = bd->blocks[bd->ooo_block]; + bd->blocks[bd->ooo_block] = lfs_emubd_incblock(bd->ooo_data); // mirror to disk file? - if (bd->disk && (bd->ooo_data || bd->cfg->erase_value != -1)) { + if (bd->disk + && (bd->blocks[bd->ooo_block] + || bd->cfg->erase_value != -1)) { off_t res1 = lseek(bd->disk->fd, (off_t)bd->ooo_block*bd->cfg->erase_size, SEEK_SET); @@ -234,22 +237,47 @@ static int lfs_emubd_powerloss(const struct lfs_config *cfg) { } ssize_t res2 = write(bd->disk->fd, - (bd->ooo_data) - ? bd->ooo_data->data + (bd->blocks[bd->ooo_block]) + ? bd->blocks[bd->ooo_block]->data : bd->disk->scratch, bd->cfg->erase_size); if (res2 < 0) { return -errno; } } - - bd->ooo_block = -1; - bd->ooo_data = NULL; } // simulate power loss bd->cfg->powerloss_cb(bd->cfg->powerloss_data); + // if we continue, undo out-of-order write emulation + if (bd->cfg->powerloss_behavior == LFS_EMUBD_POWERLOSS_OOO + && bd->ooo_block != -1) { + lfs_emubd_decblock(bd->blocks[bd->ooo_block]); + bd->blocks[bd->ooo_block] = ooo_data; + + // mirror to disk file? + if (bd->disk + && (bd->blocks[bd->ooo_block] + || bd->cfg->erase_value != -1)) { + off_t res1 = lseek(bd->disk->fd, + (off_t)bd->ooo_block*bd->cfg->erase_size, + SEEK_SET); + if (res1 < 0) { + return -errno; + } + + ssize_t res2 = write(bd->disk->fd, + (bd->blocks[bd->ooo_block]) + ? bd->blocks[bd->ooo_block]->data + : bd->disk->scratch, + bd->cfg->erase_size); + if (res2 < 0) { + return -errno; + } + } + } + return 0; } From 7873d811a09abb4fec57945ccf2a30442cea9dfd Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 27 Feb 2024 21:39:34 -0600 Subject: [PATCH 4/4] Fixed memory leak in emubd's out-of-order write emulation We need to decrement the saved block state on sync, when we reset out-of-order emulation. Otherwise we leak blocks out the wazoo. --- bd/lfs_emubd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/bd/lfs_emubd.c b/bd/lfs_emubd.c index 3f6183c9..a734bc2c 100644 --- a/bd/lfs_emubd.c +++ b/bd/lfs_emubd.c @@ -531,6 +531,7 @@ int lfs_emubd_sync(const struct lfs_config *cfg) { // emulate out-of-order writes? reset first write, writes // cannot be out-of-order across sync if (bd->cfg->powerloss_behavior == LFS_EMUBD_POWERLOSS_OOO) { + lfs_emubd_decblock(bd->ooo_data); bd->ooo_block = -1; bd->ooo_data = NULL; }