Github/littlefs
1
Fork 0
Code Actions Activity

Fixed a nasty bug in rbyd where shrinking the last id can leave bad alts

Browse Source 
This was particularly nasty to track down, the bad alts left in this way
are zero-weight, zero-tag alts that point out of the bounds of the rbyd.
This creates an immovable-object/unstoppable-force situation since the
alt that will never be followed should always be followed. This ended up
creating a confusing issue later since grows can follow this alt and
cause the alt state to fall apart.

The solution is to check for shrink leaves that drop to weight zero and
prune them. This has a side-effect of nicely handling over-sized
shrinks, though these shouldn't happen anyways and are being asserted
on.

Because I really, really don't want a regression, I've added a specific
test for this, though the minimal reproducible case is a bit complex.
The state of the rbyd is rather sensitive and it's not fully clear to me
what ultimately triggers the breakdown of the rbyd tree.

Also added a slightly better check for grow/shrink tags on altle leaves.
I don't know if this is strictly required but I know it keeps me sane.
This commit is contained in:
Christopher Haster
2023-03-11 02:06:02 -06:00
parent 89ab174f33
commit eb6b5332a0
2 changed files with 97 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+12 -8
View File
@@ -2515,13 +2515,13 @@ static int lfsr_rbyd_append(lfs_t *lfs, lfsr_rbyd_t *rbyd,
// if we diverged, merge the bounds
LFS_ASSERT(diverged < 4);
if (diverged == 2) {
// finished on upper path
// finished on lower path
tag_ = other_tag_;
id_ = other_id_;
branch = other_branch;
upper_id = other_upper_id;
} else if (diverged == 3) {
// finished on lower path
// finished on upper path
lower_id = other_lower_id;
}
@@ -2535,8 +2535,10 @@ static int lfsr_rbyd_append(lfs_t *lfs, lfsr_rbyd_t *rbyd,
// found an old removed tag, no split needed, just prune the
// removed tag
} else if ((id_ < id
|| (id_ == id && lfsr_tag_key(tag_) < lfsr_tag_key(tag)))) {
} else if (id_ < id
|| (id_ == id && lfsr_tag_key(tag_) < lfsr_tag_key(tag)
&& tag != LFSR_TAG_GROW
&& tag != LFSR_TAG_SHRINK)) {
// split less than
//
// note this is consistent for all appends and only happens when
@@ -2551,8 +2553,10 @@ static int lfsr_rbyd_append(lfs_t *lfs, lfsr_rbyd_t *rbyd,
} else if (tag == LFSR_TAG_SHRINK) {
// decrease weight when shrinking
alt = LFSR_TAG_ALT(B, GT, 0);
weight = upper_id - lower_id - 1 - lfsr_data_len(data);
if (upper_id - lower_id - 1 > (lfs_ssize_t)lfsr_data_len(data)) {
alt = LFSR_TAG_ALT(B, GT, 0);
weight = upper_id - lower_id - 1 - lfsr_data_len(data);
}
} else if (id_ > id
|| (id_ == id && lfsr_tag_key(tag_) > lfsr_tag_key(tag))) {
@@ -3360,11 +3364,11 @@ static int lfsr_btree_commit(lfs_t *lfs,
// note grow/shrink with 0 is treated as a noop in rbyd
if (rbyd->weight >= pweight) {
scratch_attrs[1] = *LFSR_ATTR(
GROW, pid, NULL, rbyd->weight-pweight,
GROW, pid-(pweight-1), NULL, rbyd->weight-pweight,
NULL);
} else {
scratch_attrs[1] = *LFSR_ATTR(
SHRINK, pid, NULL, pweight-rbyd->weight,
SHRINK, pid-(pweight-1), NULL, pweight-rbyd->weight,
NULL);
}
tests/test_rbyd.toml
+85 -2
View File
@@ -8930,6 +8930,89 @@ code = '''
}
'''
# this test is to catch the mistake of letting deletes unconditionally
# append an "altgt 0x0 w0"
[cases.test_rbyd_delete_end]
in = 'lfs.c'
code = '''
lfs_t lfs;
lfs_init(&lfs, cfg) => 0;
lfsr_rbyd_t init_rbyd = {
.block = 0,
.rev = 1,
.off = 0,
.crc = 0,
.trunk = 0,
.weight = 0,
.erased = true,
};
lfsr_rbyd_t rbyd;
uint8_t buffer[4];
rbyd = init_rbyd;
lfs_bd_erase(&lfs, rbyd.block) => 0;
// create three ids
lfsr_rbyd_commit(&lfs, &rbyd,
LFSR_ATTR(GROW, 0, NULL, 1,
LFSR_ATTR(MKREG, 0, "\xaa\xaa\xaa\xaa", 4,
LFSR_ATTR(UATTR(1), 0, "\xaa\xaa", 2,
LFSR_ATTR(GROW, 1, NULL, 1,
LFSR_ATTR(MKREG, 1, "\xbb\xbb\xbb\xbb", 4,
LFSR_ATTR(UATTR(1), 1, "\xbb\xbb", 2,
LFSR_ATTR(GROW, 2, NULL, 1,
LFSR_ATTR(MKREG, 2, "\xcc\xcc\xcc\xcc", 4,
NULL))))))))) => 0;
// delete the last two
lfsr_rbyd_commit(&lfs, &rbyd,
LFSR_ATTR(SHRINK, 1, NULL, 2, NULL)) => 0;
// create some new ids, if unconditional altgts are used this
// will end up losing tags
lfsr_rbyd_commit(&lfs, &rbyd,
LFSR_ATTR(GROW, 1, NULL, 1,
LFSR_ATTR(MKREG, 1, "\xdd\xdd\xdd\xdd", 4,
LFSR_ATTR(UATTR(1), 1, "\xdd\xdd", 2,
LFSR_ATTR(GROW, 2, NULL, 1,
LFSR_ATTR(MKREG, 2, "\xee\xee\xee\xee", 4,
LFSR_ATTR(UATTR(1), 2, "\xee\xee", 2,
NULL))))))) => 0;
assert(rbyd.weight == 3);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_MKREG, 0, buffer, 4) => 4;
assert(memcmp(buffer, "\xaa\xaa\xaa\xaa", 4) == 0);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_UATTR(1), 0, buffer, 4) => 2;
assert(memcmp(buffer, "\xaa\xaa", 2) == 0);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_MKREG, 1, buffer, 4) => 4;
assert(memcmp(buffer, "\xdd\xdd\xdd\xdd", 4) == 0);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_UATTR(1), 1, buffer, 4) => 2;
assert(memcmp(buffer, "\xdd\xdd", 2) == 0);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_MKREG, 2, buffer, 4) => 4;
assert(memcmp(buffer, "\xee\xee\xee\xee", 4) == 0);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_UATTR(1), 2, buffer, 4) => 2;
assert(memcmp(buffer, "\xee\xee", 2) == 0);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_MKREG, 3, buffer, 4)
=> LFS_ERR_NOENT;
lfsr_rbyd_fetch(&lfs, &rbyd, rbyd.block, cfg->block_size, NULL) => 0;
assert(rbyd.weight == 3);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_MKREG, 0, buffer, 4) => 4;
assert(memcmp(buffer, "\xaa\xaa\xaa\xaa", 4) == 0);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_UATTR(1), 0, buffer, 4) => 2;
assert(memcmp(buffer, "\xaa\xaa", 2) == 0);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_MKREG, 1, buffer, 4) => 4;
assert(memcmp(buffer, "\xdd\xdd\xdd\xdd", 4) == 0);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_UATTR(1), 1, buffer, 4) => 2;
assert(memcmp(buffer, "\xdd\xdd", 2) == 0);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_MKREG, 2, buffer, 4) => 4;
assert(memcmp(buffer, "\xee\xee\xee\xee", 4) == 0);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_UATTR(1), 2, buffer, 4) => 2;
assert(memcmp(buffer, "\xee\xee", 2) == 0);
lfsr_rbyd_get(&lfs, &rbyd, LFSR_TAG_MKREG, 3, buffer, 4)
=> LFS_ERR_NOENT;
'''
# Test rbyd weights
[cases.test_rbyd_grow]
@@ -11498,7 +11581,7 @@ code = '''
lfsr_rbyd_commit(&lfs, &rbyd,
LFSR_ATTR(UATTR(u), id, &alpha[i % 26], 1,
NULL)) => 0;
} else if (op == 3) {
// update our sim
sim[id*(M+1) + u+1] = '\0';
@@ -11630,7 +11713,7 @@ code = '''
lfsr_rbyd_commit(&lfs, &rbyd,
LFSR_ATTR(UATTR(u), id, &alpha[i % 26], 1,
NULL)) => 0;
} else if (op == 3) {
// update our rbyd
lfsr_rbyd_commit(&lfs, &rbyd,