Merge pull request #948 from littlefs-project/fix-sync-ordering

Fix sync issue where data writes could appear before metadata writes

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,75 @@ 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?
+    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?
+        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->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;
+            }
+        }
+    }
+
+    // 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;
+}
+
 
 // block device API
 
@@ -344,8 +416,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 +436,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 +512,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 +526,21 @@ 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) {
+        lfs_emubd_decblock(bd->ooo_data);
+        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 +725,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;

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;

lfs.c

@@ -3404,6 +3404,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;

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);

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);

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];

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);

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);

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);

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);