Fixed move handling when caught in a relocate

This was a surprisingly tricky issue. One of the subtle requirements for
the new move handling to work is that the block containing the move does
not change until the move is resolved. Initially, this seemed easy to
implement, given that a move is always immediately followed by its
resolution.

However, the extra metadata-pair operations needed to maintain integrity
present a challenge. At any commit, a directory block may end up moved
as a side effect of relocation due to a bad block.

The fix here is to move the move resolution directly into the commit
logic. This means that any commit to a block containing a move will be
implicitly resolved, leaving the later attempt at move resolution as a
noop.

This fix required quite a bit of restructuring, but as a nice
side-effect some of the complexity around moves actually went away.
Additionally, the new move handling is surprisingly powerful at
combining moves with nearby commits. And we now get same-metadata-pair
renames for free! A win for procrasination on that minor feature.

lfs.c

@@ -430,7 +430,7 @@ static inline bool lfs_pairsync(
     (((uint32_t)(type) << 22) | ((uint32_t)(id) << 12) | (uint32_t)(size))
 
 #define LFS_MKATTR(type, id, buffer, size, next) \
-    &(lfs_mattr_t){(next), LFS_MKTAG(type, id, size), (buffer)}
+    &(const lfs_mattr_t){(next), LFS_MKTAG(type, id, size), (buffer)}
 
 static inline bool lfs_tagisvalid(uint32_t tag) {
     return !(tag & 0x80000000);
@@ -485,7 +485,7 @@ struct lfs_diskoff {
 };
 
 static int32_t lfs_commitget(lfs_t *lfs, lfs_block_t block, lfs_off_t off,
-        uint32_t tag, uint32_t getmask, uint32_t gettag, int32_t difftag,
+        uint32_t tag, uint32_t getmask, uint32_t gettag, int32_t getdiff,
         void *buffer, bool stopatcommit) {
     // iterate over dir block backwards (for faster lookups)
     while (off > sizeof(tag)) {
@@ -495,10 +495,10 @@ static int32_t lfs_commitget(lfs_t *lfs, lfs_block_t block, lfs_off_t off,
         if (lfs_tagtype(tag) == LFS_TYPE_CRC && stopatcommit) {
             break;
         } else if (lfs_tagtype(tag) == LFS_TYPE_DELETE) {
-            if (lfs_tagid(tag) <= lfs_tagid(gettag + difftag)) {
+            if (lfs_tagid(tag) <= lfs_tagid(gettag + getdiff)) {
-                difftag += LFS_MKTAG(0, 1, 0);
+                getdiff += LFS_MKTAG(0, 1, 0);
             }
-        } else if ((tag & getmask) == ((gettag + difftag) & getmask)) {
+        } else if ((tag & getmask) == ((gettag + getdiff) & getmask)) {
             if (buffer) {
                 lfs_size_t diff = lfs_min(
                         lfs_tagsize(gettag), lfs_tagsize(tag));
@@ -512,7 +512,7 @@ static int32_t lfs_commitget(lfs_t *lfs, lfs_block_t block, lfs_off_t off,
                         lfs_tagsize(gettag) - diff);
             }
 
-            return tag - difftag;
+            return tag - getdiff;
         }
 
         uint32_t ntag;
@@ -926,14 +926,67 @@ relocate:
     return 0;
 }
 
-static int lfs_dir_commit(lfs_t *lfs,
+static int lfs_dir_commit(lfs_t *lfs, lfs_mdir_t *dir,
-        lfs_mdir_t *dir, const lfs_mattr_t *attrs) {
+        const lfs_mattr_t *attrs) {
-    while (true) {
+    bool canceling = (lfs_paircmp(dir->pair, lfs->globals.move.pair) == 0);
-        if (!dir->erased) {
+    lfs_mattr_t cancel;
-            // not erased, must compact
+    if (canceling) {
-            goto compact;
+        // Wait, we have the move? Just cancel this out here
+        // We need to, or else the move can become outdated
+        lfs->diff.move.pair[0] ^= 0xffffffff ^ lfs->globals.move.pair[0];
+        lfs->diff.move.pair[1] ^= 0xffffffff ^ lfs->globals.move.pair[1];
+        lfs->diff.move.id      ^= 0x3ff      ^ lfs->globals.move.id;
+
+        cancel.tag = LFS_MKTAG(LFS_TYPE_DELETE, lfs->globals.move.id, 0);
+        cancel.next = attrs;
+        attrs = &cancel;
+    }
+
+    // calculate new directory size
+    uint32_t deletetag = 0xffffffff;
+    for (const lfs_mattr_t *a = attrs; a; a = a->next) {
+        if (lfs_tagid(a->tag) < 0x3ff && lfs_tagid(a->tag) >= dir->count) {
+            dir->count = lfs_tagid(a->tag)+1;
         }
 
+        if (lfs_tagtype(a->tag) == LFS_TYPE_DELETE) {
+            LFS_ASSERT(dir->count > 0);
+            dir->count -= 1;
+            deletetag = a->tag;
+
+            if (dir->count == 0) {
+                // should we actually drop the directory block?
+                lfs_mdir_t pdir;
+                int err = lfs_pred(lfs, dir->pair, &pdir);
+                if (err && err != LFS_ERR_NOENT) {
+                    return err;
+                }
+
+                if (err != LFS_ERR_NOENT && pdir.split) {
+                    // steal tail and global state
+                    pdir.split = dir->split;
+                    pdir.tail[0] = dir->tail[0];
+                    pdir.tail[1] = dir->tail[1];
+                    lfs_globalsxor(&lfs->diff, &dir->locals);
+                    return lfs_dir_commit(lfs, &pdir,
+                            LFS_MKATTR(LFS_TYPE_TAIL + pdir.split, 0x3ff,
+                                pdir.tail, sizeof(pdir.tail),
+                            NULL));
+                }
+            }
+        }
+    }
+
+    if (!dir->erased) {
+compact:
+        // fall back to compaction
+        lfs->pcache.block = 0xffffffff;
+        int err = lfs_dir_compact(lfs, dir, attrs, dir, 0, dir->count);
+        if (err) {
+            return err;
+        }
+    } else {
+        // try to commit
         struct lfs_commit commit = {
             .block = dir->pair[0],
             .off = dir->off,
@@ -945,7 +998,20 @@ static int lfs_dir_commit(lfs_t *lfs,
         };
 
         for (const lfs_mattr_t *a = attrs; a; a = a->next) {
-            int err = lfs_commitattr(lfs, &commit, a->tag, a->buffer);
+            if (lfs_tagtype(a->tag) != LFS_TYPE_DELETE) {
+                int err = lfs_commitattr(lfs, &commit, a->tag, a->buffer);
+                if (err) {
+                    if (err == LFS_ERR_NOSPC || err == LFS_ERR_CORRUPT) {
+                        goto compact;
+                    }
+                    return err;
+                }
+            }
+        }
+
+        if (lfs_tagisvalid(deletetag)) {
+            // special case for deletes, since order matters
+            int err = lfs_commitattr(lfs, &commit, deletetag, NULL);
             if (err) {
                 if (err == LFS_ERR_NOSPC || err == LFS_ERR_CORRUPT) {
                     goto compact;
@@ -976,15 +1042,13 @@ static int lfs_dir_commit(lfs_t *lfs,
         // successful commit, update globals
         lfs_globalsxor(&dir->locals, &lfs->diff);
         lfs->diff = (lfs_globals_t){0};
-        break;
+    }
 
-compact:
+    // update globals that are affected
-        lfs->pcache.block = 0xffffffff;
+    if (canceling) {
-        err = lfs_dir_compact(lfs, dir, attrs, dir, 0, dir->count);
+        lfs->globals.move.pair[0] = 0xffffffff;
-        if (err) {
+        lfs->globals.move.pair[1] = 0xffffffff;
-            return err;
+        lfs->globals.move.id = 0x3ff;
-        }
-        break;
     }
 
     // update any directories that are affected
@@ -992,10 +1056,24 @@ compact:
     for (lfs_dir_t *d = lfs->dirs; d; d = d->next) {
         if (lfs_paircmp(d->m.pair, dir->pair) == 0) {
             d->m = *dir;
+            if (d->id > lfs_tagid(deletetag)) {
+                d->id -= 1;
+                d->pos -= 1;
+            }
+        }
+    }
+
+    for (lfs_file_t *f = lfs->files; f; f = f->next) {
+        if (lfs_paircmp(f->pair, dir->pair) == 0) {
+            if (f->id == lfs_tagid(deletetag)) {
+                f->pair[0] = 0xffffffff;
+                f->pair[1] = 0xffffffff;
+            } else if (f->id > lfs_tagid(deletetag)) {
+                f->id -= 1;
+            }
         }
     }
 
-    // TODO what if we relocated the block containing the move?
     return 0;
 }
 
@@ -1114,6 +1192,7 @@ static int32_t lfs_dir_find(lfs_t *lfs,
                         return err;
                     }
                 } else if (lfs_tagtype(tag) == LFS_TYPE_DELETE) {
+                    LFS_ASSERT(tempdir.count > 0);
                     tempdir.count -= 1;
 
                     if (lfs_tagid(tag) == lfs_tagid(tempfoundtag)) {
@@ -1174,77 +1253,15 @@ static int lfs_dir_fetch(lfs_t *lfs,
 
 static int32_t lfs_dir_get(lfs_t *lfs, lfs_mdir_t *dir,
         uint32_t getmask, uint32_t gettag, void *buffer) {
-    int32_t difftag = 0;
+    int32_t getdiff = 0;
     if (lfs_paircmp(dir->pair, lfs->globals.move.pair) == 0 &&
             lfs_tagid(gettag) <= lfs->globals.move.id) {
         // synthetic moves
-        difftag = LFS_MKTAG(0, 1, 0);
+        getdiff = LFS_MKTAG(0, 1, 0);
     }
 
     return lfs_commitget(lfs, dir->pair[0], dir->off, dir->etag,
-            getmask, gettag, difftag, buffer, false);
+            getmask, gettag, getdiff, buffer, false);
-}
-
-static int lfs_dir_append(lfs_t *lfs, lfs_mdir_t *dir, uint16_t *id) {
-    *id = dir->count;
-    dir->count += 1;
-    return 0;
-}
-
-static int lfs_dir_delete(lfs_t *lfs, lfs_mdir_t *dir, uint16_t id) {
-    dir->count -= 1;
-
-    // check if we should drop the directory block
-    if (dir->count == 0) {
-        lfs_mdir_t pdir;
-        int err = lfs_pred(lfs, dir->pair, &pdir);
-        if (err && err != LFS_ERR_NOENT) {
-            return err;
-        }
-
-        if (err != LFS_ERR_NOENT && pdir.split) {
-            // steal tail and global state
-            pdir.split = dir->split;
-            pdir.tail[0] = dir->tail[0];
-            pdir.tail[1] = dir->tail[1];
-            lfs_globalsxor(&lfs->diff, &dir->locals);
-
-            return lfs_dir_commit(lfs, &pdir,
-                    LFS_MKATTR(LFS_TYPE_TAIL + pdir.split, 0x3ff,
-                        pdir.tail, sizeof(pdir.tail),
-                    NULL));
-        }
-    }
-
-    int err = lfs_dir_commit(lfs, dir,
-            LFS_MKATTR(LFS_TYPE_DELETE, id, NULL, 0,
-            NULL));
-    if (err) {
-        return err;
-    }
-
-    // shift over any dirs/files that are affected
-    for (lfs_dir_t *d = lfs->dirs; d; d = d->next) {
-        if (lfs_paircmp(d->m.pair, dir->pair) == 0) {
-            if (d->id > id) {
-                d->id -= 1;
-                d->pos -= 1;
-            }
-        }
-    }
-
-    for (lfs_file_t *f = lfs->files; f; f = f->next) {
-        if (lfs_paircmp(f->pair, dir->pair) == 0) {
-            if (f->id == id) {
-                f->pair[0] = 0xffffffff;
-                f->pair[1] = 0xffffffff;
-            } else if (f->id > id) {
-                f->id -= 1;
-            }
-        }
-    }
-
-    return 0;
 }
 
 static int32_t lfs_dir_lookup(lfs_t *lfs, lfs_mdir_t *dir, const char **path) {
@@ -1411,12 +1428,7 @@ int lfs_mkdir(lfs_t *lfs, const char *path) {
     }
 
     // get next slot and commit
-    uint16_t id;
+    uint16_t id = cwd.count;
-    err = lfs_dir_append(lfs, &cwd, &id);
-    if (err) {
-        return err;
-    }
-
     cwd.tail[0] = dir.pair[0];
     cwd.tail[1] = dir.pair[1];
     err = lfs_dir_commit(lfs, &cwd,
@@ -1802,15 +1814,10 @@ int lfs_file_open(lfs_t *lfs, lfs_file_t *file,
         }
 
         // get next slot and create entry to remember name
-        uint16_t id;
-        int err = lfs_dir_append(lfs, &cwd, &id);
-        if (err) {
-            return err;
-        }
-
         // TODO do we need to make file registered to list to catch updates from this commit? ie if id/cwd change
         // TODO don't use inline struct? just leave it out?
-        err = lfs_dir_commit(lfs, &cwd,
+        uint16_t id = cwd.count;
+        int err = lfs_dir_commit(lfs, &cwd,
                 LFS_MKATTR(LFS_TYPE_REG, id, path, nlen,
                 LFS_MKATTR(LFS_TYPE_INLINESTRUCT, id, NULL, 0,
                 NULL)));
@@ -2056,7 +2063,7 @@ int lfs_file_sync(lfs_t *lfs, lfs_file_t *file) {
         if (!(file->flags & LFS_F_INLINE)) {
             int err = lfs_dir_commit(lfs, &cwd,
                     LFS_MKATTR(LFS_TYPE_CTZSTRUCT, file->id,
-                            &file->ctz.head, 2*sizeof(uint32_t),
+                        &file->ctz.head, sizeof(file->ctz),
                     file->attrs));
             if (err) {
                 return err;
@@ -2505,7 +2512,9 @@ int lfs_remove(lfs_t *lfs, const char *path) {
     }
 
     // delete the entry
-    err = lfs_dir_delete(lfs, &cwd, lfs_tagid(tag));
+    err = lfs_dir_commit(lfs, &cwd,
+            LFS_MKATTR(LFS_TYPE_DELETE, lfs_tagid(tag), NULL, 0,
+            NULL));
     if (err) {
         return err;
     }
@@ -2516,8 +2525,11 @@ int lfs_remove(lfs_t *lfs, const char *path) {
             return err;
         }
 
+        // steal state
+        // TODO test for global state stealing?
         cwd.tail[0] = dir.tail[0];
         cwd.tail[1] = dir.tail[1];
+        lfs_globalsxor(&lfs->diff, &dir.locals);
         err = lfs_dir_commit(lfs, &cwd,
                 LFS_MKATTR(LFS_TYPE_SOFTTAIL, 0x3ff,
                     cwd.tail, sizeof(cwd.tail),
@@ -2591,10 +2603,7 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) {
         }
 
         // get next id
-        int err = lfs_dir_append(lfs, &newcwd, &newid);
+        newid = newcwd.count;
-        if (err) {
-            return err;
-        }
     }
 
     // create move to fix later
@@ -2614,10 +2623,13 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) {
         return err;
     }
 
-    // clean up after ourselves
+    // let commit clean up after move (if we're different! otherwise move
-    err = lfs_fixmove(lfs);
+    // logic already fixed it for us)
-    if (err) {
+    if (lfs_paircmp(oldcwd.pair, newcwd.pair) != 0) {
-        return err;
+        err = lfs_dir_commit(lfs, &oldcwd, NULL);
+        if (err) {
+            return err;
+        }
     }
 
     if (prevtag != LFS_ERR_NOENT && lfs_tagtype(prevtag) == LFS_TYPE_DIR) {
@@ -2626,12 +2638,11 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) {
             return err;
         }
 
-        // steal global state
+        // steal state
         // TODO test for global state stealing?
-        lfs_globalsxor(&lfs->diff, &prevdir.locals);
-
         newcwd.tail[0] = prevdir.tail[0];
         newcwd.tail[1] = prevdir.tail[1];
+        lfs_globalsxor(&lfs->diff, &prevdir.locals);
         err = lfs_dir_commit(lfs, &newcwd,
                 LFS_MKATTR(LFS_TYPE_SOFTTAIL, 0x3ff,
                     newcwd.tail, sizeof(newcwd.tail),
@@ -2859,7 +2870,6 @@ int lfs_format(lfs_t *lfs, const struct lfs_config *cfg) {
         .name_size   = lfs->name_size,
     };
 
-    dir.count += 1;
     err = lfs_dir_commit(lfs, &dir,
             LFS_MKATTR(LFS_TYPE_SUPERBLOCK, 0, &superblock, sizeof(superblock),
             LFS_MKATTR(LFS_TYPE_DIRSTRUCT, 0, lfs->root, sizeof(lfs->root),
@@ -3250,35 +3260,6 @@ int lfs_scan(lfs_t *lfs) {
     return 0;
 }
 
-int lfs_fixmove(lfs_t *lfs) {
-    LFS_DEBUG("Fixing move %d %d %d", // TODO move to just deorphan
-            lfs->globals.move.pair[0],
-            lfs->globals.move.pair[1],
-            lfs->globals.move.id);
-
-    // mark global state to clear move entry
-    lfs->diff.move.pair[0] = 0xffffffff ^ lfs->globals.move.pair[0];
-    lfs->diff.move.pair[1] = 0xffffffff ^ lfs->globals.move.pair[1];
-    lfs->diff.move.id      = 0x3ff      ^ lfs->globals.move.id;
-
-    // fetch and delete the moved entry
-    lfs_mdir_t movedir;
-    int err = lfs_dir_fetch(lfs, &movedir, lfs->globals.move.pair);
-    if (err) {
-        return err;
-    }
-
-    err = lfs_dir_delete(lfs, &movedir, lfs->globals.move.id);
-    if (err) {
-        return err;
-    }
-
-    lfs->globals.move.pair[0] = 0xffffffff;
-    lfs->globals.move.pair[1] = 0xffffffff;
-    lfs->globals.move.id      = 0x3ff;
-    return 0;
-}
-
 int lfs_deorphan(lfs_t *lfs) {
     lfs->deorphaned = true;
     if (lfs_pairisnull(lfs->root)) { // TODO rm me?
@@ -3287,7 +3268,20 @@ int lfs_deorphan(lfs_t *lfs) {
 
     // Fix bad moves
     if (!lfs_pairisnull(lfs->globals.move.pair)) {
-        int err = lfs_fixmove(lfs);
+        LFS_DEBUG("Fixing move %d %d %d", // TODO move to just deorphan?
+                lfs->globals.move.pair[0],
+                lfs->globals.move.pair[1],
+                lfs->globals.move.id);
+
+        // fetch and delete the moved entry
+        lfs_mdir_t movedir;
+        int err = lfs_dir_fetch(lfs, &movedir, lfs->globals.move.pair);
+        if (err) {
+            return err;
+        }
+
+        // rely on cancel logic inside commit
+        err = lfs_dir_commit(lfs, &movedir, NULL);
         if (err) {
             return err;
         }