Fiddled with how opened mdirs are tracked and updated

The main intention here was to make the tracking of opened mdirs,
mostly opened lfsr_dir_t structs, simpler and more resilient to weird
corner cases. I'm not entirely sure this was successful.

The main changes:

- lfsr_dir_t now contains a full mdir for the dstart entry.

  This makes it so that dstarts are not a special case when it comes
  to mdir updates, though the fact that directories have 2 mdirs is
  still an awkward case on its own.

  I considered using two entries in the opened linked-list for this, but
  it wouldn't have worked out that well. Both entries need to update the
  directory position, so it would have required a third file type. We
  would also have needed to make sure removed mdirs mark both mdirs as
  removed, otherwise the position mdir would move around arbitrary into
  possibly erronous values.

  Instead the current solution treats the directory mdirs as a small
  array of 2 mdirs, which is as hacky as it is hacky, but does get the
  job done with little code duplication.

- Directory positions are updated a bit more intellegently.

  Instead of checking if in range before updating, which requires access
  to both mdirs and duplicate mid/rid comparison logic, position is
  updated without regard for the beginning of the directory, and
  un-updated if it was actually out of range of the directory.

  This means we only need to compare the mids/rids for each mdir once.

This changes make it so that lfsr_dir_rewind is much cheaper, and
doesn't even need to go to disk. Though I'm not sure it's worth the RAM
increase...

Expanding the lfsr_dir_t dstart entry to a full mdir does a lot for
making mdir updates more consistent, but increases the lfsr_dir_t size
from 52 bytes to 76 bytes (+46.2%).

lfs.c

@@ -5053,7 +5053,7 @@ static inline bool lfsr_mdir_isdropped(const lfsr_mdir_t *mdir) {
     return mdir->m.rbyd.trunk == 0;
 }
 
-// track "opened" mdirs that may need to by updated
+// track opened mdirs that may need to by updated
 static void lfsr_mdir_addopened(lfs_t *lfs,
         uint8_t type, lfsr_openedmdir_t *opened) {
     opened->next = lfs->opened[type];
@@ -6050,117 +6050,71 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir,
         for (lfsr_openedmdir_t *opened = lfs->opened[type];
                 opened;
                 opened = opened->next) {
-            // avoid double-updating our current mdir
+            // avoid double updating current mdir, avoid updating dropped mdirs
-            if (&opened->mdir == mdir) {
+            if (&opened->mdir == mdir || lfsr_mdir_isdropped(&opened->mdir)) {
                 continue;
             }
 
-            // first play out any attrs that change our rid
+            // kind of hacky, but this lets us iterate over both single
-            for (lfs_size_t i = 0; i < attr_count; i++) {
+            // mdirs and normal dirs which are pairs of mdirs
-                // TODO just remove dropped mdirs from this list?
+            for (uint8_t j = 0; j <= type; j++) {
-                // skip dropped mdirs
+                lfsr_mdir_t *opened_mdir = &(&opened->mdir)[j];
-                if (lfsr_mdir_isdropped(&opened->mdir)) {
-                    continue;
-                }
 
-                // TODO clean this up a bit
+                // first play out any attrs that change our rid
-                // adjust opened mdirs?
+                for (lfs_size_t i = 0; i < attr_count; i++) {
-                if (opened->mdir.mid == mdir->mid
+                    // TODO clean this up a bit?
-                        && opened->mdir.rid >= attrs[i].id) {
+                    // adjust opened mdirs?
-                    // removed?
+                    if (opened_mdir->mid == mdir->mid
-                    if (opened->mdir.rid + attrs[i].delta < attrs[i].id) {
+                            && opened_mdir->rid >= attrs[i].id) {
-                        // note we have different behavior for files and dirs
+                        // removed?
-                        // here:
+                        if (opened_mdir->rid + attrs[i].delta < attrs[i].id) {
-                        // - files => mark entry as removed
+                            // normal mdirs mark as dropped
-                        // - dirs => adjust rid/mid to point to next entry
+                            if (j == 0) {
-                        if (type == LFS_TYPE_DIR) {
+                                opened_mdir->m.rbyd.trunk = 0;
-                            opened->mdir.rid = attrs[i].id;
+                                goto next;
+                            }
+                            // for dir's second mdir (the position mdir), move
+                            // on to the next rid
+                            opened_mdir->rid = attrs[i].id;
                         } else {
-                            opened->mdir.mid = -1;
+                            opened_mdir->rid += attrs[i].delta;
-                            opened->mdir.rid = -1;
+                            // adjust dir position?
+                            if (type == LFS_TYPE_DIR && j == 0) {
+                                ((lfsr_dir_t*)opened)->pos -= attrs[i].delta;
+                            } else if (type == LFS_TYPE_DIR) {
+                                ((lfsr_dir_t*)opened)->pos += attrs[i].delta;
+                            }
                         }
-                    } else {
+                    } else if (opened_mdir->mid > mdir->mid) {
-                        opened->mdir.rid += attrs[i].delta;
                         // adjust dir position?
-                        if (type == LFS_TYPE_DIR
+                        if (type == LFS_TYPE_DIR && j == 0) {
-                                && ((((lfsr_dir_t*)opened)->dstart_mid
+                            ((lfsr_dir_t*)opened)->pos -= attrs[i].delta;
-                                        == mdir->mid
+                        } else if (type == LFS_TYPE_DIR) {
-                                        && ((lfsr_dir_t*)opened)->dstart_rid
-                                            < attrs[i].id)
-                                    || ((lfsr_dir_t*)opened)->dstart_mid
-                                        < mdir->mid)) {
                             ((lfsr_dir_t*)opened)->pos += attrs[i].delta;
                         }
                     }
-                } else if (opened->mdir.mid > mdir->mid) {
-                    // adjust dir position?
-                    if (type == LFS_TYPE_DIR
-                            && ((((lfsr_dir_t*)opened)->dstart_mid
-                                    == mdir->mid
-                                    && ((lfsr_dir_t*)opened)->dstart_rid
-                                        < attrs[i].id)
-                                || ((lfsr_dir_t*)opened)->dstart_mid
-                                    < mdir->mid)) {
-                        ((lfsr_dir_t*)opened)->pos += attrs[i].delta;
-                    }
                 }
 
-                // TODO combine with above?
+                // update any opened mdirs if we had a split or drop
-                // adjust opened dstarts?
+                if (opened_mdir->mid == mdir->mid) {
-                if (type == LFS_TYPE_DIR
+                    if (!lfsr_mdir_isdropped(&msibling_)
-                        && ((lfsr_dir_t*)opened)->dstart_mid == mdir->mid
+                            && opened_mdir->rid
-                        && ((lfsr_dir_t*)opened)->dstart_rid >= attrs[i].id) {
-                    // removed?
-                    if (((lfsr_dir_t*)opened)->dstart_rid
-                            + attrs[i].delta < attrs[i].id) {
-                        ((lfsr_dir_t*)opened)->dstart_mid = -1;
-                        ((lfsr_dir_t*)opened)->dstart_rid = -1;
-                    } else {
-                        ((lfsr_dir_t*)opened)->dstart_rid += attrs[i].delta;
-                    }
-                }
-            }
-
-            // update mid if we had a split or drop
-            if (opened->mdir.mid == mdir->mid
-                    && opened->mdir.m.rbyd.weight > 0) {
-                if (msibling_.m.rbyd.weight > 0
-                        && opened->mdir.rid
-                            >= (lfs_ssize_t)mdir_.m.rbyd.weight) {
-                    LFS_ASSERT(lfsr_btree_weight(&mtree_)
-                            != lfsr_mtree_weight(lfs));
-                    opened->mdir.mid = msibling_.mid;
-                    opened->mdir.rid -= mdir_.m.rbyd.weight;
-                    opened->mdir.m = msibling_.m;
-                } else {
-                    opened->mdir.mid = mdir_.mid;
-                    opened->mdir.m = mdir_.m;
-                }
-            } else if (opened->mdir.mid > mdir->mid) {
-                opened->mdir.mid += lfsr_btree_weight(&mtree_)
-                        - lfsr_mtree_weight(lfs);
-            }
-
-            // update dstarts if we had a split or drop
-            if (type == LFS_TYPE_DIR) {
-                if (((lfsr_dir_t*)opened)->dstart_mid == mdir->mid) {
-                    if (msibling_.m.rbyd.weight > 0
-                            && ((lfsr_dir_t*)opened)->dstart_rid
                                 >= (lfs_ssize_t)mdir_.m.rbyd.weight) {
                         LFS_ASSERT(lfsr_btree_weight(&mtree_)
                                 != lfsr_mtree_weight(lfs));
-                        ((lfsr_dir_t*)opened)->dstart_rid
+                        opened_mdir->mid = msibling_.mid;
-                                -= mdir_.m.rbyd.weight;
+                        opened_mdir->rid -= mdir_.m.rbyd.weight;
-                        ((lfsr_dir_t*)opened)->dstart_mid = msibling_.mid;
+                        opened_mdir->m = msibling_.m;
                     } else {
-                        ((lfsr_dir_t*)opened)->dstart_mid = mdir_.mid;
+                        opened_mdir->mid = mdir_.mid;
+                        opened_mdir->m = mdir_.m;
                     }
-                } else if (((lfsr_dir_t*)opened)->dstart_mid > mdir->mid) {
+                } else if (opened_mdir->mid > mdir->mid) {
-                    ((lfsr_dir_t*)opened)->dstart_mid
+                    opened_mdir->mid += lfsr_btree_weight(&mtree_)
-                            += lfsr_btree_weight(&mtree_)
+                            - lfsr_mtree_weight(lfs);
-                                - lfsr_mtree_weight(lfs);
                 }
             }
+    next:;
         }
     }
 
@@ -6168,9 +6122,9 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir,
     LFS_ASSERT(mdir->rid <= (lfs_ssize_t)mdir->m.rbyd.weight);
     if (mdir->mid < 0 && mdir->rid < 0) {
         mdir->m = mroot_.m;
-    } else if (mdir->rid >= (lfs_ssize_t)mdir_.m.rbyd.weight) {
+    } else if (!lfsr_mdir_isdropped(&msibling_)
-        // note removes can trigger this incorrectly, but we don't really
+            && mdir->rid >= (lfs_ssize_t)mdir_.m.rbyd.weight) {
-        // care, the rid was removed after all
+        LFS_ASSERT(lfsr_btree_weight(&mtree_) != lfsr_mtree_weight(lfs));
         mdir->mid = msibling_.mid;
         mdir->rid -= mdir_.m.rbyd.weight;
         mdir->m = msibling_.m;
@@ -7626,37 +7580,28 @@ int lfsr_dir_open(lfs_t *lfs, lfsr_dir_t *dir, const char *path) {
         }
     }
 
-    // reset pos
-    dir->pos = 0;
-
     // lookup our dstart in the mtree
     err = lfsr_mtree_dnamelookup(lfs, dir->did, NULL, 0,
-            &dir->m.mdir, NULL, NULL);
+            &dir->dstart_mdir, NULL, NULL);
     if (err) {
         LFS_ASSERT(err != LFS_ERR_NOENT);
         return err;
     }
 
-    // keep track of the mid/rid of our dstart
+    // let rewind initialize pos/mdir state
-    dir->dstart_mid = dir->m.mdir.mid;
+    err = lfsr_dir_rewind(lfs, dir);
-    dir->dstart_rid = dir->m.mdir.rid;
+    if (err) {
-
-    // eagerly look up the next entry
-    //
-    // this makes handling of corner cases with mixed removes/dir reads easier
-    err = lfsr_mtree_seek(lfs, &dir->m.mdir, 1);
-    if (err && err != LFS_ERR_NOENT) {
         return err;
     }
 
     // add to tracked mdirs
-    lfsr_mdir_addopened(lfs, LFS_TYPE_DIR, &dir->m);
+    lfsr_mdir_addopened(lfs, LFS_TYPE_DIR, (lfsr_openedmdir_t*)dir);
     return 0;
 }
 
 int lfsr_dir_close(lfs_t *lfs, lfsr_dir_t *dir) {
     // remove from tracked mdirs
-    lfsr_mdir_removeopened(lfs, LFS_TYPE_DIR, &dir->m);
+    lfsr_mdir_removeopened(lfs, LFS_TYPE_DIR, (lfsr_openedmdir_t*)dir);
     return 0;
 }
 
@@ -7677,7 +7622,7 @@ int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) {
     }
 
     // seek in case our mdir was dropped
-    int err = lfsr_mtree_seek(lfs, &dir->m.mdir, 0);
+    int err = lfsr_mtree_seek(lfs, &dir->pos_mdir, 0);
     if (err) {
         return err;
     }
@@ -7685,8 +7630,8 @@ int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) {
     // lookup our name tag
     lfsr_tag_t tag;
     lfsr_data_t data;
-    err = lfsr_mdir_lookup(lfs, &dir->m.mdir,
+    err = lfsr_mdir_lookup(lfs, &dir->pos_mdir,
-            dir->m.mdir.rid, LFSR_TAG_WIDENAME,
+            dir->pos_mdir.rid, LFSR_TAG_WIDENAME,
             &tag, &data);
     if (err) {
         return err;
@@ -7718,7 +7663,7 @@ int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) {
     // TODO get size once we actually have regular files
 
     // eagerly look up the next entry
-    err = lfsr_mtree_seek(lfs, &dir->m.mdir, 1);
+    err = lfsr_mtree_seek(lfs, &dir->pos_mdir, 1);
     if (err && err != LFS_ERR_NOENT) {
         return err;
     }
@@ -7739,7 +7684,7 @@ int lfsr_dir_seek(lfs_t *lfs, lfsr_dir_t *dir, lfs_off_t off) {
     //
     // note the -2 to adjust for dot entries
     if (off > 2) {
-        err = lfsr_mtree_seek(lfs, &dir->m.mdir, off - 2);
+        err = lfsr_mtree_seek(lfs, &dir->pos_mdir, off - 2);
         if (err && err != LFS_ERR_NOENT) {
             return err;
         }
@@ -7756,25 +7701,18 @@ lfs_soff_t lfsr_dir_tell(lfs_t *lfs, lfsr_dir_t *dir) {
 
 int lfsr_dir_rewind(lfs_t *lfs, lfsr_dir_t *dir) {
     // do nothing if removed
-    if (dir->dstart_rid == -1) {
+    if (lfsr_mdir_isdropped(&dir->dstart_mdir)) {
         return 0;
     }
 
     // reset pos
     dir->pos = 0;
 
-    // lookup our dstart in the mtree again
+    // copy dstart mdir and eagerly look up the next entry
-    int err = lfsr_mtree_lookup(lfs, dir->dstart_mid, dir->dstart_rid,
-            &dir->m.mdir);
-    if (err) {
-        LFS_ASSERT(err != LFS_ERR_NOENT);
-        return err;
-    }
-
-    // eagerly look up the next entry
     //
     // this makes handling of corner cases with mixed removes/dir reads easier
-    err = lfsr_mtree_seek(lfs, &dir->m.mdir, 1);
+    dir->pos_mdir = dir->dstart_mdir;
+    int err = lfsr_mtree_seek(lfs, &dir->pos_mdir, 1);
     if (err && err != LFS_ERR_NOENT) {
         return err;
     }

lfs.h

@@ -424,10 +424,14 @@ typedef struct lfs_dir {
 } lfs_dir_t;
 
 typedef struct lfsr_dir {
-    lfsr_openedmdir_t m;
+    // the order is very sensitive here!
+    // this overlaps with:
+    // - lfsr_openedmdir_t
+    // - lfsr_mdir_t[2]
+    struct lfsr_openedmdir *next;
+    lfsr_mdir_t dstart_mdir;
+    lfsr_mdir_t pos_mdir;
     lfs_size_t did;
-    lfs_ssize_t dstart_mid;
-    lfs_ssize_t dstart_rid;
     lfs_off_t pos;
 } lfsr_dir_t;
 

tests/t3_mtree.toml

@@ -2698,10 +2698,12 @@ code = '''
     assert(memcmp(buffer, &alphas[2 % 26], 1) == 0);
 
     // assert that our neighbors were updated correctly
+    assert(!lfsr_mdir_isdropped(&left_neighbor.mdir));
     assert(left_neighbor.mdir.mid == -1);
     assert(left_neighbor.mdir.rid == 0);
     assert(memcmp(&left_neighbor.mdir.m, &lfs.mroot.m,
             sizeof(lfs.mroot.m)) == 0);
+    assert(!lfsr_mdir_isdropped(&right_neighbor.mdir));
     assert(right_neighbor.mdir.mid == -1);
     assert(right_neighbor.mdir.rid == 2);
     assert(memcmp(&right_neighbor.mdir.m, &lfs.mroot.m,
@@ -2746,8 +2748,8 @@ code = '''
     assert(lfs.mroot.m.rbyd.weight == 1);
 
     // assert that our neighbors were updated correctly
-    assert(left_neighbor.mdir.mid == -1);
+    assert(lfsr_mdir_isdropped(&left_neighbor.mdir));
-    assert(left_neighbor.mdir.rid == -1);
+    assert(!lfsr_mdir_isdropped(&right_neighbor.mdir));
     assert(right_neighbor.mdir.mid == -1);
     assert(right_neighbor.mdir.rid == 0);
     assert(memcmp(&right_neighbor.mdir.m, &lfs.mroot.m,
@@ -2792,12 +2794,12 @@ code = '''
     assert(lfs.mroot.m.rbyd.weight == 1);
 
     // assert that our neighbors were updated correctly
+    assert(!lfsr_mdir_isdropped(&left_neighbor.mdir));
     assert(left_neighbor.mdir.mid == -1);
     assert(left_neighbor.mdir.rid == 0);
     assert(memcmp(&left_neighbor.mdir.m, &lfs.mroot.m,
             sizeof(lfs.mroot.m)) == 0);
-    assert(right_neighbor.mdir.mid == -1);
+    assert(lfsr_mdir_isdropped(&right_neighbor.mdir));
-    assert(right_neighbor.mdir.rid == -1);
 
     lfsr_mdir_removeopened(&lfs, LFS_TYPE_REG, &left_neighbor);
     lfsr_mdir_removeopened(&lfs, LFS_TYPE_REG, &right_neighbor);
@@ -2872,10 +2874,12 @@ code = '''
     assert(msibling.m.rbyd.weight == 1);
 
     // assert that our neighbors were updated correctly
+    assert(!lfsr_mdir_isdropped(&left_neighbor.mdir));
     assert(left_neighbor.mdir.mid == 0);
     assert(left_neighbor.mdir.rid == 0);
     assert(memcmp(&left_neighbor.mdir.m, &mdir.m,
             sizeof(mdir.m)) == 0);
+    assert(!lfsr_mdir_isdropped(&right_neighbor.mdir));
     assert(right_neighbor.mdir.mid == 1);
     assert(right_neighbor.mdir.rid == 0);
     assert(memcmp(&right_neighbor.mdir.m, &msibling.m,
@@ -2949,10 +2953,12 @@ code = '''
     assert(memcmp(buffer, &alphas[3 % 26], 1) == 0);
 
     // assert that our neighbors were updated correctly
+    assert(!lfsr_mdir_isdropped(&left_neighbor.mdir));
     assert(left_neighbor.mdir.mid == 0);
     assert(left_neighbor.mdir.rid == 0);
     assert(memcmp(&left_neighbor.mdir.m, &mdir.m,
             sizeof(mdir.m)) == 0);
+    assert(!lfsr_mdir_isdropped(&right_neighbor.mdir));
     assert(right_neighbor.mdir.mid == 1);
     assert(right_neighbor.mdir.rid == 1);
     assert(memcmp(&right_neighbor.mdir.m, &msibling.m,
@@ -3052,10 +3058,12 @@ code = '''
     assert(memcmp(buffer, &alphas[4 % 26], 1) == 0);
 
     // assert that our neighbors were updated correctly
+    assert(!lfsr_mdir_isdropped(&left_neighbor.mdir));
     assert(left_neighbor.mdir.mid == 0);
     assert(left_neighbor.mdir.rid == 0);
     assert(memcmp(&left_neighbor.mdir.m, &mdir.m,
             sizeof(mdir.m)) == 0);
+    assert(!lfsr_mdir_isdropped(&right_neighbor.mdir));
     assert(right_neighbor.mdir.mid == 1);
     assert(right_neighbor.mdir.rid == 1);
     assert(memcmp(&right_neighbor.mdir.m, &msibling.m,
@@ -3121,10 +3129,12 @@ code = '''
     assert(memcmp(buffer, &alphas[1 % 26], 1) == 0);
 
     // assert that our neighbors were updated correctly
+    assert(!lfsr_mdir_isdropped(&left_neighbor.mdir));
     assert(left_neighbor.mdir.mid == -1);
     assert(left_neighbor.mdir.rid == 0);
     assert(memcmp(&left_neighbor.mdir.m, &lfs.mroot.m,
             sizeof(lfs.mroot.m)) == 0);
+    assert(!lfsr_mdir_isdropped(&right_neighbor.mdir));
     assert(right_neighbor.mdir.mid == -1);
     assert(right_neighbor.mdir.rid == 1);
     assert(memcmp(&right_neighbor.mdir.m, &lfs.mroot.m,
@@ -3219,9 +3229,11 @@ code = '''
     assert(memcmp(buffer, &alphas[4 % 26], 1) == 0);
 
     // assert that our neighbors were updated correctly
+    assert(!lfsr_mdir_isdropped(&left_neighbor.mdir));
     assert(left_neighbor.mdir.mid == 0);
     assert(left_neighbor.mdir.rid == 0);
     assert(memcmp(&left_neighbor.mdir.m, &mdir.m, sizeof(mdir.m)) == 0);
+    assert(!lfsr_mdir_isdropped(&right_neighbor.mdir));
     assert(right_neighbor.mdir.mid == 0);
     assert(right_neighbor.mdir.rid == 2);
     assert(memcmp(&right_neighbor.mdir.m, &mdir.m, sizeof(mdir.m)) == 0);
@@ -3312,6 +3324,7 @@ code = '''
     assert(lfsr_mtree_weight(&lfs) == 4);
 
     // assert that our neighbors were updated correctly
+    assert(!lfsr_mdir_isdropped(&left_neighbor.mdir));
     assert(left_neighbor.mdir.mid == 0);
     assert(left_neighbor.mdir.rid == 0);
     lfsr_mtree_lookup(&lfs, 0, -1, &mdir) => 0;
@@ -3403,11 +3416,13 @@ code = '''
     assert(lfsr_mtree_weight(&lfs) == 2);
 
     // assert that our neighbors were updated correctly
+    assert(!lfsr_mdir_isdropped(&left_neighbor.mdir));
     assert(left_neighbor.mdir.mid == 0);
     assert(left_neighbor.mdir.rid == 0);
     lfsr_mtree_lookup(&lfs, 0, -1, &mdir) => 0;
     assert(memcmp(&left_neighbor.mdir.m, &mdir.m, sizeof(mdir.m)) == 0);
 
+    assert(!lfsr_mdir_isdropped(&right_neighbor.mdir));
     assert(right_neighbor.mdir.mid == 1);
     assert(right_neighbor.mdir.rid == 0);
     lfsr_mtree_lookup(&lfs, 1, -1, &mdir) => 0;