Skip to content

Commit

Permalink
Cleaned up a few additional commit corner cases
Browse files Browse the repository at this point in the history 
- General cleanup from integration, including cleaning up some older
  commit code
- Partial-prog tests do not make sense when prog_size == block_size
  (there can't be partial-progs!)
- Fixed signed-comparison issue in modified filebd
  • Loading branch information
@geky
geky committed Dec 17, 2022
1 parent 52dd830 commit 91ad673
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 115 deletions.
211 changes: 96 additions & 115 deletions lfs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1160,25 +1160,21 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
// reset crc
crc = 0xffffffff;

// check if the next page is erased
//
// this may look inefficient, but it's surprisingly efficient
// since cache_size is probably > prog_size, so the data will
// always remain in cache for the next iteration
if (lfs_tag_chunk(tag) == 3) {
// check if the next page is erased
//
// this may look inefficient, but since cache_size is
// probably > prog_size, the data will always remain in
// cache for the next iteration

// first we get a tag-worth of bits, this is so we can
// tweak our current tag to force future writes to be
// different than the erased state
lfs_tag_t etag;
err = lfs_bd_read(lfs,
NULL, &lfs->rcache, lfs->cfg->block_size,
dir->pair[0], dir->off, &etag, sizeof(etag));
if (err) {
// TODO can we stop duplicating this error condition?
if (err == LFS_ERR_CORRUPT) {
dir->erased = false;
break;
}
if (err && err != LFS_ERR_CORRUPT) {
return err;
}

Expand All @@ -1192,97 +1188,94 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
err = lfs_bd_crc(lfs,
NULL, &lfs->rcache, lfs->cfg->block_size,
dir->pair[0], dir->off, estate.size, &tcrc);
if (err) {
if (err == LFS_ERR_CORRUPT) {
dir->erased = false;
break;
}
if (err && err != LFS_ERR_CORRUPT) {
return err;
}

if (tcrc == estate.crc) {
dir->erased = true;
break;
}
} else {
} else if (lfs_tag_chunk(tag) == 2) {
// end of block commit
// TODO handle backwards compat?
dir->erased = false;
break;
}

continue;
}

// crc the entry first, hopefully leaving it in the cache
err = lfs_bd_crc(lfs,
NULL, &lfs->rcache, lfs->cfg->block_size,
dir->pair[0], off+sizeof(tag),
lfs_tag_dsize(tag)-sizeof(tag), &crc);
if (err) {
if (err == LFS_ERR_CORRUPT) {
dir->erased = false;
break;
} else {
// for backwards compatibility we fall through here on
// unrecognized tags, leaving it up to the CRC to reject
// bad commits
}
return err;
}

// directory modification tags?
if (lfs_tag_type1(tag) == LFS_TYPE_NAME) {
// increase count of files if necessary
if (lfs_tag_id(tag) >= tempcount) {
tempcount = lfs_tag_id(tag) + 1;
}
} else if (lfs_tag_type1(tag) == LFS_TYPE_SPLICE) {
tempcount += lfs_tag_splice(tag);

if (tag == (LFS_MKTAG(LFS_TYPE_DELETE, 0, 0) |
(LFS_MKTAG(0, 0x3ff, 0) & tempbesttag))) {
tempbesttag |= 0x80000000;
} else if (tempbesttag != -1 &&
lfs_tag_id(tag) <= lfs_tag_id(tempbesttag)) {
tempbesttag += LFS_MKTAG(0, lfs_tag_splice(tag), 0);
}
} else if (lfs_tag_type1(tag) == LFS_TYPE_TAIL) {
tempsplit = (lfs_tag_chunk(tag) & 1);

err = lfs_bd_read(lfs,
} else {
// crc the entry first, hopefully leaving it in the cache
err = lfs_bd_crc(lfs,
NULL, &lfs->rcache, lfs->cfg->block_size,
dir->pair[0], off+sizeof(tag), &temptail, 8);
dir->pair[0], off+sizeof(tag),
lfs_tag_dsize(tag)-sizeof(tag), &crc);
if (err) {
if (err == LFS_ERR_CORRUPT) {
dir->erased = false;
break;
}
return err;
}
lfs_pair_fromle32(temptail);
}

// found a match for our fetcher?
if ((fmask & tag) == (fmask & ftag)) {
int res = cb(data, tag, &(struct lfs_diskoff){
dir->pair[0], off+sizeof(tag)});
if (res < 0) {
if (res == LFS_ERR_CORRUPT) {
dir->erased = false;
break;
// directory modification tags?
if (lfs_tag_type1(tag) == LFS_TYPE_NAME) {
// increase count of files if necessary
if (lfs_tag_id(tag) >= tempcount) {
tempcount = lfs_tag_id(tag) + 1;
}
return res;
} else if (lfs_tag_type1(tag) == LFS_TYPE_SPLICE) {
tempcount += lfs_tag_splice(tag);

if (tag == (LFS_MKTAG(LFS_TYPE_DELETE, 0, 0) |
(LFS_MKTAG(0, 0x3ff, 0) & tempbesttag))) {
tempbesttag |= 0x80000000;
} else if (tempbesttag != -1 &&
lfs_tag_id(tag) <= lfs_tag_id(tempbesttag)) {
tempbesttag += LFS_MKTAG(0, lfs_tag_splice(tag), 0);
}
} else if (lfs_tag_type1(tag) == LFS_TYPE_TAIL) {
tempsplit = (lfs_tag_chunk(tag) & 1);

err = lfs_bd_read(lfs,
NULL, &lfs->rcache, lfs->cfg->block_size,
dir->pair[0], off+sizeof(tag), &temptail, 8);
if (err) {
if (err == LFS_ERR_CORRUPT) {
dir->erased = false;
break;
}
}
lfs_pair_fromle32(temptail);
}

if (res == LFS_CMP_EQ) {
// found a match
tempbesttag = tag;
} else if ((LFS_MKTAG(0x7ff, 0x3ff, 0) & tag) ==
(LFS_MKTAG(0x7ff, 0x3ff, 0) & tempbesttag)) {
// found an identical tag, but contents didn't match
// this must mean that our besttag has been overwritten
tempbesttag = -1;
} else if (res == LFS_CMP_GT &&
lfs_tag_id(tag) <= lfs_tag_id(tempbesttag)) {
// found a greater match, keep track to keep things sorted
tempbesttag = tag | 0x80000000;
// found a match for our fetcher?
if ((fmask & tag) == (fmask & ftag)) {
int res = cb(data, tag, &(struct lfs_diskoff){
dir->pair[0], off+sizeof(tag)});
if (res < 0) {
if (res == LFS_ERR_CORRUPT) {
dir->erased = false;
break;
}
return res;
}

if (res == LFS_CMP_EQ) {
// found a match
tempbesttag = tag;
} else if ((LFS_MKTAG(0x7ff, 0x3ff, 0) & tag) ==
(LFS_MKTAG(0x7ff, 0x3ff, 0) & tempbesttag)) {
// found an identical tag, but contents didn't match
// this must mean that our besttag has been overwritten
tempbesttag = -1;
} else if (res == LFS_CMP_GT &&
lfs_tag_id(tag) <= lfs_tag_id(tempbesttag)) {
// found a greater match, keep track to keep
// things sorted
tempbesttag = tag | 0x80000000;
}
}
}
}
Expand Down Expand Up @@ -1640,7 +1633,6 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
int err = lfs_bd_read(lfs,
NULL, &lfs->rcache, esize,
commit->block, noff, &etag, sizeof(etag));
// TODO handle erased-as-corrupt correctly?
if (err && err != LFS_ERR_CORRUPT) {
return err;
}
Expand All @@ -1650,7 +1642,6 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
err = lfs_bd_crc(lfs,
NULL, &lfs->rcache, esize,
commit->block, noff, esize, &ecrc);
// TODO handle erased-as-corrupt correctly?
if (err && err != LFS_ERR_CORRUPT) {
return err;
}
Expand Down Expand Up @@ -1696,44 +1687,34 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
}

// successful commit, check checksums to make sure
//
// note that we don't need to check padding commits, worst
// case if they are corrupted we would have had to compact anyways
lfs_off_t off = commit->begin;
lfs_off_t noff = off1;
while (off < end) {
// TODO restructure to use lfs_bd_crc?
uint32_t crc = 0xffffffff;
for (lfs_off_t i = off; i < noff+sizeof(uint32_t); i++) {
// check against written crc, may catch blocks that
// become readonly and match our commit size exactly
if (i == off1 && crc != crc1) {
return LFS_ERR_CORRUPT;
}

uint8_t dat;
err = lfs_bd_read(lfs,
NULL, &lfs->rcache, noff+sizeof(uint32_t)-i,
commit->block, i, &dat, 1);
if (err) {
return err;
}
uint32_t crc = 0xffffffff;
err = lfs_bd_crc(lfs,
NULL, &lfs->rcache, off1+sizeof(uint32_t),
commit->block, off, off1-off, &crc);
if (err) {
return err;
}

crc = lfs_crc(crc, &dat, 1);
}
// check against known crc for non-padding commits
if (crc != crc1) {
return LFS_ERR_CORRUPT;
}

// detected write error?
if (crc != 0) {
return LFS_ERR_CORRUPT;
}
// make sure to check crc in case we happened to pick
// up an unrelated crc (frozen block?)
err = lfs_bd_crc(lfs,
NULL, &lfs->rcache, sizeof(uint32_t),
commit->block, off1, sizeof(uint32_t), &crc);
if (err) {
return err;
}

// skip padding, note that these always contain estate
off = noff - 3*sizeof(uint32_t);
off = lfs_min(end - off, 0x3fe) + off;
if (off < end) {
off = lfs_min(off, end - 2*sizeof(uint32_t));
}
noff = off + sizeof(uint32_t);
if (noff <= lfs->cfg->block_size - esize) {
noff += 2*sizeof(uint32_t);
}
if (crc != 0) {
return LFS_ERR_CORRUPT;
}

return 0;
Expand Down
1 change: 1 addition & 0 deletions tests/test_powerloss.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ code = '''

# partial prog, may not be byte in order!
[cases.test_powerloss_partial_prog]
if = "PROG_SIZE < BLOCK_SIZE"
defines.BYTE_OFF = ["0", "PROG_SIZE-1", "PROG_SIZE/2"]
defines.BYTE_VALUE = [0x33, 0xcc]
in = "lfs.c"
Expand Down

0 comments on commit 91ad673

Please sign in to comment.