Github/littlefs
1
Fork 0
Code Actions Activity

Found another bug in mdir tracking, dropping for an extra lookup

Browse Source 
Long story short, lfsr_mkdir needs to atomically create two somewhat
arbitrary mid entries: the bookmark and the dir entry. We can't, so we
fake it with our grm:

1. Create bookmark mid, set grm to delete bookmark mid
2. Create dir mid, zeroing grm

This works well enough, with the _small_ (read not small) caveat that
creating the bookmark mid can indirectly change the dir mid in
surprising ways.

The original plan was to take advantage of our opened-mdir tracking to
track an on-stack mdir pointing to where the dir mid should go. This
gets tricky, because, by definition of not being created yet, the dir
mid doesn't actually have an mid we can track. But this shouldn't be any
issue if we track the mid _after_ where we should insert right?

Wrong.

The issue is mdir splits. Which are especially nefarious because the
vast majority of the time mdirs don't split. And the split has to happen
_exactly_ between the tracked mid and where we would have inserted. Rare,
but possible. Which is why this went undetected for so long.

Consider this example of creating dir c:

  .---------------.
  |bmk|reg|reg|reg|
  | a | b | d | e |
  '---------------'
            ^
            '-- track reg d (to insert dir c)

First insert the did, but oh no! a split occured!

        .-------.
        |mdr|mdr|
        |   | d |
        '-|---|-'
      .---'   '---.
      v           v
  .-------.   .-------.
  |bmk|reg|   |reg|reg|
  | a | b |   | d | e |
  '-------'   '-------'
                ^
                '-- track reg d (to insert dir c)

Now insert the dir c before reg d:

        .-------.
        |mdr|mdr|
        |   | d |<--------------------.
        '-|---|-'                     |
      .---'   '-----.        ???????????????????
      v             v         dir c unreachable?
  .-------.   .-----------.  ???????????????????
  |bmk|reg|   |dir|reg|reg|           |
  | a | b |   | c | d | e |           |
  '-------'   '-----------'           |
                ^                     |
                '---------------------'

Problem, dir c became unreasonable.

Some options:

1. I _think_ things work if you track the mid _before_ where we want to
   insert, iff we track the dir mid.

   1. Our parent directory always at least contains a bookmark mid, so
      it's not possible for our bookmark to be inserted between the mid
      _before_ our dir mid and our dir mid.

      This is possible if we track the bookmark mid, and would break
      things.

   2. mdir splits use the first name in the right mdir, so even if we
      split on the mid immediately after the mid _before_ our dir mid,
      inserting the dir mid should not make it unreachable.

   But, as experience has shown, this whole tracking thing is very
   fragile.

2. Create an orphan file and track that, replacing it atomically after
   we've created our bookmark.

   This works, but trades read overhead for write overhead. Even though
   it's O(log^2 n) reads vs O(1) writes, write overhead always takes
   priority and should be minimized. Flash is destructive after all.

3. Rip our the on-stack mdir tracking and just do an additional name
   lookup after the bookmark is created.

mdir tracking, while clever, has created enough difficult to find bugs
and enough headache that I think its time is up. This commit implements
option 3.

Besides, it's not like lfsr_mkdir is really a performance sensitive
function anyways...

Code changes:

            code          stack
  before:  34036           2944
  after:   34048 (+0.0%)   2944 (+0.0%)
This commit is contained in:
Christopher Haster
2024-01-18 23:16:16 -06:00
parent 1db215309b
commit 76ccefcc84
1 changed files with 27 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+27 -37
View File
@@ -8450,10 +8450,9 @@ int lfsr_mkdir(lfs_t *lfs, const char *path) {
// Check if we have a collision. If we do, search for the next
// available did
lfsr_opened_t bookmark;
while (true) {
err = lfsr_mtree_namelookup(lfs, did_, NULL, 0,
&bookmark.mdir, NULL, NULL);
&mdir, NULL, NULL);
if (err) {
if (err == LFS_ERR_NOENT) {
break;
@@ -8466,26 +8465,34 @@ int lfsr_mkdir(lfs_t *lfs, const char *path) {
}
// found a good did, now to commit to the mtree
//
// A problem: we need to create both:
// 1. the metadata entry
// 2. the bookmark entry
//
// To do this atomically, we first create the bookmark entry with a grm
// to delete-self in case of powerloss, then create the metadata entry
// while atomically cancelling the grm.
// A problem: we need to create both 1. the metadata entry and 2. the
// bookmark entry.
//
// To do this atomically, we first create the metadata entry with a grm
// to delete-self in case of powerloss, then create the bookmark while
// atomically cancelling the grm.
//
// These commits can change the relative mids of each other, so we track
// the bookmark mdir as an "open file" temporarily.
//
// Note! The metadata/bookmark order is important! Attempting to create
// the bookmark first risks inserting the bookmark before the metadata
// entry, which breaks things.
//
bookmark.type = 0;
lfsr_addopened(lfs, &bookmark);
// commit our bookmark and a grm to self-remove in case of powerloss
err = lfsr_mdir_commit(lfs, &mdir, LFSR_ATTRS(
LFSR_ATTR(mdir.mid, BOOKMARK, +1, LEB128(did_)),
LFSR_ATTR(-1, GRM, 0, GRM(&((lfsr_grm_t){{mdir.mid, -1}})))));
if (err) {
return err;
}
// commit our new directory into our parent, creating a grm to self-remove
// in case of powerloss
// committing our bookmark may have changed the mid of our metadata entry,
// we need to look it up again, we can at least avoid the full path walk
err = lfsr_mtree_namelookup(lfs, did, name, name_size,
&mdir, NULL, NULL);
if (err && err != LFS_ERR_NOENT) {
return err;
}
LFS_ASSERT((exists) ? !err : err == LFS_ERR_NOENT);
// commit our new directory into our parent, zeroing the grm in the
// process
err = lfsr_mdir_commit(lfs, &mdir, LFSR_ATTRS(
LFSR_ATTR(mdir.mid + ((exists) ? 1 : 0),
DIR, +1, CAT(
@@ -8496,29 +8503,12 @@ int lfsr_mkdir(lfs_t *lfs, const char *path) {
(exists)
? LFSR_ATTR(mdir.mid, RM, -1, NULL())
: LFSR_ATTR_NOOP(),
LFSR_ATTR(-1, GRM, 0, GRM(&((lfsr_grm_t){{
mdir.mid,
-1}})))));
if (err) {
goto failed_with_bookmark;
}
lfsr_removeopened(lfs, &bookmark);
// commit our bookmark and zero the grm, the bookmark tag is an empty
// entry that marks our did as allocated
err = lfsr_mdir_commit(lfs, &bookmark.mdir, LFSR_ATTRS(
LFSR_ATTR(bookmark.mdir.mid, BOOKMARK, +1, LEB128(did_)),
LFSR_ATTR(-1, GRM, 0, GRM(&((lfsr_grm_t){{-1, -1}})))));
if (err) {
return err;
}
return 0;
failed_with_bookmark:
lfsr_removeopened(lfs, &bookmark);
return err;
}
int lfsr_remove(lfs_t *lfs, const char *path) {