Fixed mdir-relocate-pcache corruption, test_ck_spam_* bitflips
Ckprogs does not suffer from rollback issues! I was too quick to assume
this was the case in test_ck_spam_* (I blame ckfetches), but it just
turned out that the more aggressive bit flip tests found an actual bug!
The bug in question is caused by bit-errors being introduced in multiple
blocks during mdir relocation.
When relocating, we make the false assumption that if
lfsr_mdir_compact__ returns success, the intermediary compaction has
successfully been written to disk. But this is not true until we
write the rest of the commit and flush the pcache. If the remaining
commit fails due to a bit-error, the pcache can end up corrupt and the
intermediary compaction lost.
But why do we care about the intermediary compaction at all after
corruption? Why do we keep updating the mdir every attempted relocation?
We already mark all relevant mdirs as unerased (eoff=-1) in the
top-level lfsr_mdir_commit, so as far as I can tell the only reason for
updating the mdir on error is to propagate mdir.rbyd.weight=0 when the
mdir is empty (LFS_ERR_NOENT).
But this is a bit stupid. Relying on mdir state across function
boundaries on error is incredibly fragile. If instead we consider the
mdir clobbered on any error and move all the implicit mdir.rbyd.weight=0
stuff up into lfsr_mdir_commit, this whole category of problems goes
away.
So yeah, that's what we do now:
- lfsr_mdir_commit__ failed => mdir clobbered
- lfsr_mdir_compact__ failed => mdir clobbered
- lfsr_mdir_commit_ failed => mdir preserved, marked unerased
- lfsr_mdir_commit failed => mdir preserved, marked unerased
---
Curiously, all of these changes ended up with a net-zero cost:
code stack
before: 36432 2672
after: 36432 (+0.0%) 2672 (+0.0%)
This commit is contained in:
@@ -7743,9 +7743,8 @@ static int lfsr_mdir_commit__(lfs_t *lfs, lfsr_mdir_t *mdir,
|
||||
if (rbyd_.weight == 0
|
||||
// unless we are an mroot
|
||||
&& !(mdir->mid == -1 || lfsr_mdir_cmp(mdir, &lfs->mroot) == 0)) {
|
||||
// mark weight as zero, but note! we can no longer read from this mdir
|
||||
// as our pcache may be clobbered
|
||||
mdir->rbyd.weight = 0;
|
||||
// note we can no longer read from this mdir as our pcache may
|
||||
// be clobbered
|
||||
return LFS_ERR_NOENT;
|
||||
}
|
||||
|
||||
@@ -8143,14 +8142,11 @@ relocate:;
|
||||
return err;
|
||||
}
|
||||
|
||||
// update mdir, we need to propagate mdir changes if commit fails
|
||||
*mdir = mdir_;
|
||||
|
||||
// now try to commit again
|
||||
//
|
||||
// upper layers should make sure this can't fail by limiting the
|
||||
// maximum commit size
|
||||
err = lfsr_mdir_commit__(lfs, mdir, start_rid, end_rid,
|
||||
err = lfsr_mdir_commit__(lfs, &mdir_, start_rid, end_rid,
|
||||
mid, attrs, attr_count);
|
||||
if (err) {
|
||||
LFS_ASSERT(err != LFS_ERR_RANGE);
|
||||
@@ -8161,6 +8157,8 @@ relocate:;
|
||||
return err;
|
||||
}
|
||||
|
||||
// update mdir
|
||||
*mdir = mdir_;
|
||||
return 0;
|
||||
}
|
||||
|
||||
@@ -8349,6 +8347,10 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir,
|
||||
}
|
||||
goto failed;
|
||||
}
|
||||
// empty? set weight to zero
|
||||
if (err == LFS_ERR_NOENT) {
|
||||
mdir_[i^left].rbyd.weight = 0;
|
||||
}
|
||||
}
|
||||
|
||||
// adjust our sibling's mid after committing attrs
|
||||
@@ -8467,6 +8469,8 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir,
|
||||
"0x{%"PRIx32",%"PRIx32"}",
|
||||
mdir->mid >> lfs->mdir_bits,
|
||||
mdir->rbyd.blocks[0], mdir->rbyd.blocks[1]);
|
||||
// set weight to zero
|
||||
mdir_[0].rbyd.weight = 0;
|
||||
|
||||
// consume gstate so we don't lose any info
|
||||
err = lfsr_fs_consumegdelta(lfs, mdir);
|
||||
|
||||
+5
-15
@@ -1493,9 +1493,7 @@ code = '''
|
||||
defines.METHOD = [0]
|
||||
defines.PERIOD = 10
|
||||
defines.PROTECTED_MROOTANCHOR = [false, true]
|
||||
# TODO rollback is even a problem for ckprogs, change this
|
||||
# to PROGFLIP when rollback protection is in place
|
||||
defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGNOOP'
|
||||
defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGFLIP'
|
||||
defines.CKPROGS = 'METHOD == 0'
|
||||
defines.CKREADS = 'METHOD == 1'
|
||||
defines.CKFETCHES = 'METHOD == 2'
|
||||
@@ -1736,9 +1734,7 @@ corrupt_mounted:;
|
||||
defines.METHOD = [0]
|
||||
defines.PERIOD = 10
|
||||
defines.PROTECTED_MROOTANCHOR = [false, true]
|
||||
# TODO rollback is even a problem for ckprogs, change this
|
||||
# to PROGFLIP when rollback protection is in place
|
||||
defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGNOOP'
|
||||
defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGFLIP'
|
||||
defines.CKPROGS = 'METHOD == 0'
|
||||
defines.CKREADS = 'METHOD == 1'
|
||||
defines.CKFETCHES = 'METHOD == 2'
|
||||
@@ -2051,9 +2047,7 @@ corrupt_mounted:;
|
||||
defines.METHOD = [0]
|
||||
defines.PERIOD = 10
|
||||
defines.PROTECTED_MROOTANCHOR = [false, true]
|
||||
# TODO rollback is even a problem for ckprogs, change this
|
||||
# to PROGFLIP when rollback protection is in place
|
||||
defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGNOOP'
|
||||
defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGFLIP'
|
||||
defines.CKPROGS = 'METHOD == 0'
|
||||
defines.CKREADS = 'METHOD == 1'
|
||||
defines.CKFETCHES = 'METHOD == 2'
|
||||
@@ -2281,9 +2275,7 @@ defines.BADBLOCK = -1
|
||||
defines.METHOD = [0]
|
||||
defines.PERIOD = 10
|
||||
defines.PROTECTED_MROOTANCHOR = [false, true]
|
||||
# TODO rollback is even a problem for ckprogs, change this
|
||||
# to PROGFLIP when rollback protection is in place
|
||||
defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGNOOP'
|
||||
defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGFLIP'
|
||||
defines.CKPROGS = 'METHOD == 0'
|
||||
defines.CKREADS = 'METHOD == 1'
|
||||
defines.CKFETCHES = 'METHOD == 2'
|
||||
@@ -2717,9 +2709,7 @@ corrupt_mounted:;
|
||||
defines.METHOD = [0]
|
||||
defines.PERIOD = 10
|
||||
defines.PROTECTED_MROOTANCHOR = [false, true]
|
||||
# TODO rollback is even a problem for ckprogs, change this
|
||||
# to PROGFLIP when rollback protection is in place
|
||||
defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGNOOP'
|
||||
defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGFLIP'
|
||||
defines.CKPROGS = 'METHOD == 0'
|
||||
defines.CKREADS = 'METHOD == 1'
|
||||
defines.CKFETCHES = 'METHOD == 2'
|
||||
|
||||
Reference in New Issue
Block a user