Github/littlefs
1
Fork 0
Code Actions Activity

Reverted the second mdir in lfsr_dir_t, simplified dir updates

Browse Source 
This reverts the big hack of treating the lfsr_dir_t as an mdir array in
lfsr_mdir_commit in an effort to deduplicate the bookmark/pos mid
updates.

It worked, but lets be honest, it was a big hack and probably not very
maintainable. It made other opened-mdir updates, such as propagation of
unerases more complex, and is made the code a bit unreadable.

We also don't really need a full mdir for the dir's bookmark, since
rewinds really aren't that common, and a single mtree lookup in that
case gets the job done. Removing the bookmark mdir (though we still need
the bookmark mid to adjust the dir pos correctly) saves 24 bytes from
every lfsr_dir_t.

It would be nice to deduplicate some of the mid logic here, but that's
been difficult because of mid-related side-effects, such as updating the
mdir's pos. There may be room for improvement here.

---

This looks pretty bad, with the additional loop over the attr-list to
update just the dir's bookmark, but it's really not that bad when
compiled, and probably worth the code readability:

            code          stack          structs
  before:  20958           1744              864
  after:   21052 (+0.4%)   1744 (+0.0%)      840 (-2.9%)
This commit is contained in:
Christopher Haster
2023-09-10 12:58:00 -05:00
parent cced7d66ef
commit ba571cf83f
2 changed files with 122 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+120 -81
View File
@@ -4582,13 +4582,13 @@ static int lfsr_data_readmblocks(lfs_t *lfs, lfsr_data_t *data,
// 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, static void lfsr_mdir_addopened(lfs_t *lfs,
uint8_t type, lfsr_openedmdir_t *opened) { unsigned type, lfsr_openedmdir_t *opened) {
opened->next = lfs->opened[type]; opened->next = lfs->opened[type];
lfs->opened[type] = opened; lfs->opened[type] = opened;
} }
static void lfsr_mdir_removeopened(lfs_t *lfs, static void lfsr_mdir_removeopened(lfs_t *lfs,
uint8_t type, lfsr_openedmdir_t *opened) { unsigned type, lfsr_openedmdir_t *opened) {
for (lfsr_openedmdir_t **p = &lfs->opened[type]; *p; p = &(*p)->next) { for (lfsr_openedmdir_t **p = &lfs->opened[type]; *p; p = &(*p)->next) {
if (*p == opened) { if (*p == opened) {
*p = (*p)->next; *p = (*p)->next;
@@ -4598,7 +4598,7 @@ static void lfsr_mdir_removeopened(lfs_t *lfs,
} }
static bool lfsr_mdir_isopened(lfs_t *lfs, static bool lfsr_mdir_isopened(lfs_t *lfs,
uint8_t type, const lfsr_openedmdir_t *opened) { unsigned type, const lfsr_openedmdir_t *opened) {
for (lfsr_openedmdir_t *p = lfs->opened[type]; p; p = p->next) { for (lfsr_openedmdir_t *p = lfs->opened[type]; p; p = p->next) {
if (p == opened) { if (p == opened) {
return true; return true;
@@ -4812,6 +4812,9 @@ static int lfsr_mtree_parent(lfs_t *lfs, const lfs_block_t blocks[static 2],
} }
static int lfsr_mtree_seek(lfs_t *lfs, lfsr_mdir_t *mdir, lfs_off_t off) { static int lfsr_mtree_seek(lfs_t *lfs, lfsr_mdir_t *mdir, lfs_off_t off) {
// upper layers should handle removed mdirs
LFS_ASSERT(mdir->mid >= 0);
while (true) { while (true) {
// calculate new mid, be careful to avoid rid overflow // calculate new mid, be careful to avoid rid overflow
lfs_size_t bid = mdir->mid & lfsr_mbidmask(lfs); lfs_size_t bid = mdir->mid & lfsr_mbidmask(lfs);
@@ -5143,6 +5146,7 @@ compact:;
return 0; return 0;
} }
// high-level mdir commit // high-level mdir commit
// //
// this is also responsible for updating any opened mdirs, lfs_t, gstate, etc // this is also responsible for updating any opened mdirs, lfs_t, gstate, etc
@@ -5186,19 +5190,13 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir,
if (mdir->mid == -1 || lfsr_mtree_isinlined(lfs)) { if (mdir->mid == -1 || lfsr_mtree_isinlined(lfs)) {
lfsr_mdir_unerase(&lfs->mroot); lfsr_mdir_unerase(&lfs->mroot);
} }
for (uint8_t type = 0; type < 2; type++) { for (unsigned type = 0; type < 2; type++) {
for (lfsr_openedmdir_t *opened = lfs->opened[type]; for (lfsr_openedmdir_t *opened = lfs->opened[type];
opened; opened;
opened = opened->next) { opened = opened->next) {
// TODO this is now a liability if ((opened->mdir.mid & lfsr_mbidmask(lfs))
// kind of hacky, but this lets us iterate over both single == (lfs_smax32(mdir->mid, 0) & lfsr_mbidmask(lfs))) {
// mdirs and normal dirs which are pairs of mdirs lfsr_mdir_unerase(&opened->mdir);
for (uint8_t j = 0; j <= type; j++) {
lfsr_mdir_t *opened_mdir = &(&opened->mdir)[j];
if ((opened_mdir->mid & lfsr_mbidmask(lfs))
== (lfs_smax32(mdir->mid, 0) & lfsr_mbidmask(lfs))) {
lfsr_mdir_unerase(opened_mdir);
}
} }
} }
} }
@@ -5649,81 +5647,113 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir,
// keep track of the exact encoding on-disk // keep track of the exact encoding on-disk
int err = lfsr_grm_todisk(lfs, &lfs->grm, lfs->pgrm); int err = lfsr_grm_todisk(lfs, &lfs->grm, lfs->pgrm);
if (err) { if (err) {
LFS_ASSERT(!err);
return err; return err;
} }
} }
} }
// update any opened mdirs // update any opened mdirs
for (uint8_t type = 0; type < 2; type++) { for (unsigned type = 0; type < 2; type++) {
for (lfsr_openedmdir_t *opened = lfs->opened[type]; for (lfsr_openedmdir_t *opened = lfs->opened[type];
opened; opened;
opened = opened->next) { opened = opened->next) {
// avoid double updating current mdir, avoid updating dropped mdirs // avoid double updating current mdir, and avoid updating
// dropped mdirs
if (&opened->mdir == mdir || opened->mdir.mid == -1) { if (&opened->mdir == mdir || opened->mdir.mid == -1) {
continue; continue;
} }
// kind of hacky, but this lets us iterate over both single // first play out any attrs that change our rid
// mdirs and normal dirs which are pairs of mdirs for (lfs_size_t i = 0; i < attr_count; i++) {
for (uint8_t j = 0; j <= type; j++) { // adjust opened mdirs?
lfsr_mdir_t *opened_mdir = &(&opened->mdir)[j]; if ((opened->mdir.mid & lfsr_mbidmask(lfs))
LFS_ASSERT(opened_mdir->mid >= 0); == (lfs_smax32(mdir->mid, 0) & lfsr_mbidmask(lfs))
&& opened->mdir.mid >= attrs[i].rid) {
// first play out any attrs that change our rid // removed?
for (lfs_size_t i = 0; i < attr_count; i++) { if (opened->mdir.mid < attrs[i].rid - attrs[i].delta) {
// TODO clean this up a bit? // for dir's second mdir (the position mdir), move
// adjust opened mdirs? // on to the next rid
if ((opened_mdir->mid & lfsr_mbidmask(lfs)) if (type == LFS_TYPE_DIR) {
== (lfs_smax32(mdir->mid, 0) opened->mdir.mid = attrs[i].rid;
& lfsr_mbidmask(lfs)) // for normal mdirs mark as dropped
&& (opened_mdir->mid >= attrs[i].rid)) {
// removed?
if (opened_mdir->mid < attrs[i].rid - attrs[i].delta) {
// normal mdirs mark as dropped
if (j == 0) {
opened_mdir->mid = -1;
opened_mdir->u.m.trunk = 0;
goto next;
}
// for dir's second mdir (the position mdir), move
// on to the next rid
opened_mdir->mid = attrs[i].rid;
} else { } else {
opened_mdir->mid += attrs[i].delta; opened->mdir.mid = -1;
// adjust dir position? goto next;
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 if (opened_mdir->mid > mdir->mid) { } else {
opened->mdir.mid += attrs[i].delta;
// adjust dir position? // adjust dir position?
if (type == LFS_TYPE_DIR && j == 0) { if (type == LFS_TYPE_DIR) {
((lfsr_dir_t*)opened)->pos -= attrs[i].delta;
} else if (type == LFS_TYPE_DIR) {
((lfsr_dir_t*)opened)->pos += attrs[i].delta; ((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)->pos += attrs[i].delta;
}
}
}
// update any opened mdirs if we had a split or drop
if ((opened->mdir.mid & lfsr_mbidmask(lfs))
== (lfs_smax32(mdir->mid, 0) & lfsr_mbidmask(lfs))) {
if (msibling_.u.m.weight > 0
&& (opened->mdir.mid & lfsr_mridmask(lfs))
>= (lfs_ssize_t)mdir_.u.m.weight) {
LFS_ASSERT(lfsr_btree_weight(&mtree_)
!= lfsr_mtree_weight(lfs));
opened->mdir.mid += lfsr_mweight(lfs)
- mdir_.u.m.weight;
opened->mdir.u.m = msibling_.u.m;
} else {
opened->mdir.u.m = mdir_.u.m;
}
} else if (opened->mdir.mid > mdir->mid) {
opened->mdir.mid += lfsr_btree_weight(&mtree_)
- lfsr_mtree_weight(lfs);
}
// some extra work is needed for opened dirs
if (type == LFS_TYPE_DIR) {
lfsr_dir_t *dir = (lfsr_dir_t*)opened;
for (lfs_size_t i = 0; i < attr_count; i++) {
// TODO clean this up a bit?
// adjust opened mdirs?
if ((dir->bookmark & lfsr_mbidmask(lfs))
== (lfs_smax32(mdir->mid, 0)
& lfsr_mbidmask(lfs))
&& dir->bookmark >= attrs[i].rid) {
// removed?
if (dir->bookmark < attrs[i].rid - attrs[i].delta) {
// mark dir as dropped
dir->m.mdir.mid = -1;
dir->bookmark = -1;
goto next;
} else {
dir->bookmark += attrs[i].delta;
// adjust dir position?
dir->pos -= attrs[i].delta;
}
} else if (dir->bookmark > mdir->mid) {
// adjust dir position?
dir->pos -= attrs[i].delta;
}
} }
// update any opened mdirs if we had a split or drop if ((dir->bookmark & lfsr_mbidmask(lfs))
if ((opened_mdir->mid & lfsr_mbidmask(lfs))
== (lfs_smax32(mdir->mid, 0) & lfsr_mbidmask(lfs))) { == (lfs_smax32(mdir->mid, 0) & lfsr_mbidmask(lfs))) {
if (msibling_.u.m.weight > 0 if (msibling_.u.m.weight > 0
&& (opened_mdir->mid & lfsr_mridmask(lfs)) && (dir->bookmark & lfsr_mridmask(lfs))
>= (lfs_ssize_t)mdir_.u.m.weight) { >= (lfs_ssize_t)mdir_.u.m.weight) {
LFS_ASSERT(lfsr_btree_weight(&mtree_) LFS_ASSERT(lfsr_btree_weight(&mtree_)
!= lfsr_mtree_weight(lfs)); != lfsr_mtree_weight(lfs));
opened_mdir->mid += lfsr_mweight(lfs) dir->bookmark += lfsr_mweight(lfs)
- mdir_.u.m.weight; - mdir_.u.m.weight;
opened_mdir->u.m = msibling_.u.m;
} else {
opened_mdir->u.m = mdir_.u.m;
} }
} else if (opened_mdir->mid > mdir->mid) { } else if (dir->bookmark > mdir->mid) {
opened_mdir->mid += lfsr_btree_weight(&mtree_) dir->bookmark += lfsr_btree_weight(&mtree_)
- lfsr_mtree_weight(lfs); - lfsr_mtree_weight(lfs);
} }
} }
@@ -7201,7 +7231,7 @@ int lfsr_dir_open(lfs_t *lfs, lfsr_dir_t *dir, const char *path) {
// are we a directory? // are we a directory?
if (tag != LFSR_TAG_DIR) { if (tag != LFSR_TAG_DIR) {
return LFS_ERR_NOENT; return LFS_ERR_NOTDIR;
} }
// read our did from the mdir, unless we're root // read our did from the mdir, unless we're root
@@ -7221,15 +7251,8 @@ int lfsr_dir_open(lfs_t *lfs, lfsr_dir_t *dir, const char *path) {
} }
} }
// lookup our bookmark in the mtree // let rewind initialize the pos/bookmark state
err = lfsr_mtree_namelookup(lfs, dir->did, NULL, 0, dir->bookmark = 0;
&dir->bookmark_mdir, NULL, NULL);
if (err) {
LFS_ASSERT(err != LFS_ERR_NOENT);
return err;
}
// let rewind initialize pos/mdir state
err = lfsr_dir_rewind(lfs, dir); err = lfsr_dir_rewind(lfs, dir);
if (err) { if (err) {
return err; return err;
@@ -7247,7 +7270,10 @@ int lfsr_dir_close(lfs_t *lfs, lfsr_dir_t *dir) {
} }
int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) { int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) {
memset(info, 0, sizeof(struct lfs_info)); // was our dir removed?
if (dir->bookmark == -1) {
return LFS_ERR_NOENT;
}
// handle dots specially // handle dots specially
if (dir->pos == 0) { if (dir->pos == 0) {
@@ -7263,7 +7289,7 @@ int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) {
} }
// seek in case our mdir was dropped // seek in case our mdir was dropped
int err = lfsr_mtree_seek(lfs, &dir->pos_mdir, 0); int err = lfsr_mtree_seek(lfs, &dir->m.mdir, 0);
if (err) { if (err) {
return err; return err;
} }
@@ -7271,8 +7297,8 @@ int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) {
// lookup our name tag // lookup our name tag
lfsr_tag_t tag; lfsr_tag_t tag;
lfsr_data_t data; lfsr_data_t data;
err = lfsr_mdir_lookup(lfs, &dir->pos_mdir, err = lfsr_mdir_lookup(lfs, &dir->m.mdir,
dir->pos_mdir.mid, LFSR_TAG_WIDE(NAME), dir->m.mdir.mid, LFSR_TAG_WIDE(NAME),
&tag, &data); &tag, &data);
if (err) { if (err) {
return err; return err;
@@ -7305,7 +7331,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 // TODO get size once we actually have regular files
// eagerly look up the next entry // eagerly look up the next entry
err = lfsr_mtree_seek(lfs, &dir->pos_mdir, 1); err = lfsr_mtree_seek(lfs, &dir->m.mdir, 1);
if (err && err != LFS_ERR_NOENT) { if (err && err != LFS_ERR_NOENT) {
return err; return err;
} }
@@ -7315,6 +7341,11 @@ int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) {
} }
int lfsr_dir_seek(lfs_t *lfs, lfsr_dir_t *dir, lfs_off_t off) { int lfsr_dir_seek(lfs_t *lfs, lfsr_dir_t *dir, lfs_off_t off) {
// do nothing if removed
if (dir->bookmark == -1) {
return 0;
}
// first rewind // first rewind
int err = lfsr_dir_rewind(lfs, dir); int err = lfsr_dir_rewind(lfs, dir);
if (err) { if (err) {
@@ -7326,7 +7357,7 @@ int lfsr_dir_seek(lfs_t *lfs, lfsr_dir_t *dir, lfs_off_t off) {
// //
// note the -2 to adjust for dot entries // note the -2 to adjust for dot entries
if (off > 2) { if (off > 2) {
err = lfsr_mtree_seek(lfs, &dir->pos_mdir, off - 2); err = lfsr_mtree_seek(lfs, &dir->m.mdir, off - 2);
if (err && err != LFS_ERR_NOENT) { if (err && err != LFS_ERR_NOENT) {
return err; return err;
} }
@@ -7343,18 +7374,26 @@ lfs_soff_t lfsr_dir_tell(lfs_t *lfs, lfsr_dir_t *dir) {
int lfsr_dir_rewind(lfs_t *lfs, lfsr_dir_t *dir) { int lfsr_dir_rewind(lfs_t *lfs, lfsr_dir_t *dir) {
// do nothing if removed // do nothing if removed
if (dir->bookmark_mdir.mid == -1) { if (dir->bookmark == -1) {
return 0; return 0;
} }
// reset pos // lookup our bookmark in the mtree
int err = lfsr_mtree_namelookup(lfs, dir->did, NULL, 0,
&dir->m.mdir, NULL, NULL);
if (err) {
LFS_ASSERT(err != LFS_ERR_NOENT);
return err;
}
// keep track of bookmark so we can adjust pos correctly
dir->bookmark = dir->m.mdir.mid;
dir->pos = 0; dir->pos = 0;
// copy bookmark mdir and eagerly look up the next entry // eagerly lookup the next entry
// //
// this makes handling of corner cases with mixed removes/dir reads easier // this makes handling of corner cases with mixed removes/dir reads easier
dir->pos_mdir = dir->bookmark_mdir; err = lfsr_mtree_seek(lfs, &dir->m.mdir, 1);
int err = lfsr_mtree_seek(lfs, &dir->pos_mdir, 1);
if (err && err != LFS_ERR_NOENT) { if (err && err != LFS_ERR_NOENT) {
return err; return err;
} }
lfs.h
+2 -7
View File
@@ -428,14 +428,9 @@ typedef struct lfs_dir {
} lfs_dir_t; } lfs_dir_t;
typedef struct lfsr_dir { typedef struct lfsr_dir {
// the order is very sensitive here! lfsr_openedmdir_t m;
// this overlaps with:
// - lfsr_openedmdir_t
// - lfsr_mdir_t[2]
struct lfsr_openedmdir *next;
lfsr_mdir_t bookmark_mdir;
lfsr_mdir_t pos_mdir;
lfs_size_t did; lfs_size_t did;
lfs_ssize_t bookmark;
lfs_off_t pos; lfs_off_t pos;
} lfsr_dir_t; } lfsr_dir_t;