t: Dropped LFS_T_EXCL/LFS_I_DIRTY
The tests highlighted that the LFS_I_DIRTY flag in lfsr_tinfo approach
is insufficient. Consider what happens if our filesystem is mutated
while traversing the last mdir:
1. Traversal traverses last mdir, populate blocks, return first block
2. Filesystem mutated, maybe mdir was compacted, clobbers traversal and
sets LFS_I_DIRTY
3. Traversal return LFS_ERR_NOENT immediately, last block never
returned (and out of date), LFS_I_DIRTY never returned
Not only do we miss the LFS_I_DIRTY flag, but we completely miss the
last block in the mdir pair without any warning.
This is _not_ a problem for the actual lookahead buffer, since we still
internally check the LFS_I_DIRTY flag before marking it as complete, but
it is an issue for any external logic that depends on the traversal
being complete...
---
We could revert to LFS_T_EXCL, but, to be honest, I just really don't
know a good name for this flag...
LFS_T_EXCL is a bad name because it conflicts with LFS_O_EXCL. These
flags have very different behaviors, which risks confusing users, and
risks potential name conflicts down the line if we ever want
LFS_T_EXCL-esque semantics for open dirs/files (not unreasonable, though
quite fancy).
My current best contender is LFS_T_WATCH, but while scratching my head
on this, I starting to wonder why we're even providing LFS_T_EXCL in the
first place...
We err on the side of forcing users to implement filesystem-external
features themselves when possible elsewhere, and LFS_T_EXCL technically
_can_ be implemented entirely outside of the filesystem. Though to be
fair it is quite annoying/tedious.
It's not like there's any equivalent feature for dir/file reads anyways.
And a background thread calling lfsr_traversal_read with LFS_T_LOOKAHEAD
will still _eventually_ make progress, even if it takes a bit longer.
Don't get me wrong, I understand it is significantly easier to implement
this inside the filesystem than outside. But it's also easier to
implement this later than right now. And if we implement this later,
hopefully we'll have a better idea what exactly will be useful for
users.
---
Removing LFS_T_EXCL/LFS_I_DIRTY has no real impact on code cost. We were
really just exposing internal logic that we need for lookahead
correctness anyways:
code stack
before: 35224 2680
after: 35220 (-0.0%) 2680 (+0.0%)
This commit is contained in:
@@ -5968,7 +5968,7 @@ static bool lfsr_omdir_ismidopen(lfs_t *lfs, lfsr_smid_t mid) {
|
||||
static void lfsr_fs_mkdirty(lfs_t *lfs) {
|
||||
for (lfsr_omdir_t *o = lfs->omdirs; o; o = o->next) {
|
||||
if (o->type == LFS_TYPE_TRAVERSAL) {
|
||||
o->flags |= LFS_I_DIRTY;
|
||||
o->flags |= LFS_F_DIRTY;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -8261,8 +8261,8 @@ static inline bool lfsr_t_isckdata(uint32_t flags) {
|
||||
return flags & (LFS_T_CKDATA ^ LFS_T_CKMETA);
|
||||
}
|
||||
|
||||
static inline bool lfsr_i_isdirty(uint32_t flags) {
|
||||
return flags & LFS_I_DIRTY;
|
||||
static inline bool lfsr_f_isdirty(uint32_t flags) {
|
||||
return flags & LFS_F_DIRTY;
|
||||
}
|
||||
|
||||
|
||||
@@ -8679,7 +8679,7 @@ static int lfsr_mtree_gc(lfs_t *lfs, lfsr_traversal_t *t,
|
||||
}
|
||||
|
||||
// mark as dirty
|
||||
t->o.o.flags |= LFS_I_DIRTY;
|
||||
t->o.o.flags |= LFS_F_DIRTY;
|
||||
}
|
||||
|
||||
// compacting btree nodes?
|
||||
@@ -8757,7 +8757,7 @@ static int lfsr_mtree_gc(lfs_t *lfs, lfsr_traversal_t *t,
|
||||
t->u.bt.rid = t->u.bt.bid;
|
||||
|
||||
// mark as dirty
|
||||
t->o.o.flags |= LFS_I_DIRTY;
|
||||
t->o.o.flags |= LFS_F_DIRTY;
|
||||
}
|
||||
|
||||
if (tag_) {
|
||||
@@ -12827,7 +12827,7 @@ int lfsr_traversal_open(lfs_t *lfs, lfsr_traversal_t *t, uint32_t flags) {
|
||||
LFS_ASSERT(!lfsr_t_ismtreeonly(flags) || !lfsr_t_islookahead(flags));
|
||||
LFS_ASSERT(!lfsr_t_ismtreeonly(flags) || !lfsr_t_isckdata(flags));
|
||||
// these flags are internal and shouldn't be provided by the user
|
||||
LFS_ASSERT(!lfsr_i_isdirty(flags));
|
||||
LFS_ASSERT(!lfsr_f_isdirty(flags));
|
||||
|
||||
// some flags mutate the filesystem
|
||||
if (lfsr_t_ismkconsistent(flags)
|
||||
@@ -12870,7 +12870,6 @@ int lfsr_traversal_read(lfs_t *lfs, lfsr_traversal_t *t,
|
||||
// some redund blocks left over?
|
||||
if (t->blocks[0] != -1) {
|
||||
// write our traversal info
|
||||
tinfo->flags = t->o.o.flags & LFS_I_DIRTY;
|
||||
tinfo->btype = lfsr_t_btype(t->o.o.flags);
|
||||
tinfo->block = t->blocks[0];
|
||||
|
||||
@@ -12918,7 +12917,7 @@ int lfsr_traversal_read(lfs_t *lfs, lfsr_traversal_t *t,
|
||||
done:;
|
||||
// was a lookahead scan successful?
|
||||
if (lfsr_t_islookahead(t->o.o.flags)
|
||||
&& !lfsr_i_isdirty(t->o.o.flags)) {
|
||||
&& !lfsr_f_isdirty(t->o.o.flags)) {
|
||||
lfs_alloc_markfree(lfs);
|
||||
}
|
||||
|
||||
@@ -12961,7 +12960,7 @@ static void lfsr_traversal_clobber(lfs_t *lfs, lfsr_traversal_t *t) {
|
||||
static int lfsr_traversal_rewind_(lfs_t *lfs, lfsr_traversal_t *t) {
|
||||
(void)lfs;
|
||||
// reset traversal
|
||||
t->o.o.flags &= ~LFS_I_DIRTY;
|
||||
t->o.o.flags &= ~LFS_F_DIRTY;
|
||||
t->o.o.state = LFSR_TSTATE_MROOTANCHOR;
|
||||
t->o.o.mdir.mid = -1;
|
||||
t->o.o.mdir.rbyd.weight = 0;
|
||||
|
||||
@@ -177,11 +177,9 @@ enum lfs_traversal_flags {
|
||||
// TODO
|
||||
// LFS_T_REPAIRMETA = 0x0400, // Repair metadata blocks
|
||||
// LFS_T_REPAIRDATA = 0x0c00, // Repair metadata + data blocks
|
||||
};
|
||||
|
||||
enum lfs_tinfo_flags {
|
||||
// traversal info flags
|
||||
LFS_I_DIRTY = 0x1000, // Filesystem has been modified
|
||||
// internally used flags
|
||||
LFS_F_DIRTY = 0x1000, // Filesystem has been modified
|
||||
};
|
||||
|
||||
|
||||
@@ -385,9 +383,6 @@ struct lfs_fsinfo {
|
||||
|
||||
// Traversal info structure
|
||||
struct lfs_tinfo {
|
||||
// Traversal flags
|
||||
uint16_t flags;
|
||||
|
||||
// Type of the block
|
||||
uint8_t btype;
|
||||
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
Reference in New Issue
Block a user