Reverted removal of 1-bit counter threaded through tags

Initially I thought the fcrc would be sufficient for all of the
end-of-commit context, since indicating that there is a new commit is a
simple as invalidating the fcrc. But it turns out there are cases that
make this impossible.

The surprising, and actually common, case, is that of an fcrc that
will end up containing a full commit. This is common as soon as the
prog_size is big, as small commits are padded to the prog_size at
minimum.

  .------------------. \
  |     metadata     | |
  |                  | |
  |                  | +-.
  |------------------| | |
  |   foward CRC ------------.
  |------------------| / |   |
  |   commit CRC    -----'   |
  |------------------|       |
  |     padding      |       |
  |                  |       |
  |------------------| \   \ |
  |     metadata     | |   | |
  |                  | +-. | |
  |                  | | | +-'
  |------------------| / | |
  |   commit CRC --------' |
  |------------------|     |
  |                  |     /
  '------------------'

When the commit + crc is all contained in the fcrc, something silly
happens with the math behind crcs. Everything in the commit gets
canceled out:

  crc(m) = m(x) x^|P|-1 mod P(x)

  m ++ crc(m) = m(x) x^|P|-1 + (m(x) x^|P|-1 mod P(x))

  crc(m ++ crc(m)) = (m(x) x^|P|-1 + (m(x) x^|P|-1 mod P(x))) x^|P|-1 mod P(x)

  crc(m ++ crc(m)) = (m(x) x^|P|-1 + m(x) x^|P|-1) x^|P|-1 mod P(x)

  crc(m ++ crc(m)) = 0 * x^|P|-1 mod P(x)

This is the reason the crc of a message + naive crc is zero. Even with an
initializer/bit-fiddling, the crc of the whole commit ends up as some
constant.

So no manipulation of the commit can change the fcrc...

But even if this did work, or we changed this scheme to use two
different checksums, it would still require calculating the fcrc of
the whole commit to know if we need to tweak the first bit to invalidate
the unlikely-but-problematic case where we happen to match the fcrc. This
would add a large amount of complexity to the commit code.

It's much simpler and cheaper to keep the 1-bit counter in the tag, even
if it adds another moving part to the system.
This commit is contained in:
Christopher Haster
2022-12-10 13:16:52 -06:00
parent 2f26966710
commit d1b254da2c
3 changed files with 98 additions and 110 deletions

168
lfs.c
View File

@@ -1078,6 +1078,8 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
bool tempsplit = false; bool tempsplit = false;
lfs_stag_t tempbesttag = besttag; lfs_stag_t tempbesttag = besttag;
// assume not erased until proven otherwise
bool maybeerased = false;
bool hasfcrc = false; bool hasfcrc = false;
struct lfs_fcrc fcrc; struct lfs_fcrc fcrc;
@@ -1095,24 +1097,26 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
if (err) { if (err) {
if (err == LFS_ERR_CORRUPT) { if (err == LFS_ERR_CORRUPT) {
// can't continue? // can't continue?
dir->erased = false;
break; break;
} }
return err; return err;
} }
crc = lfs_crc(crc, &tag, sizeof(tag)); crc = lfs_crc(crc, &tag, sizeof(tag));
tag = (lfs_frombe32(tag) ^ ptag) & 0x7fffffff; tag = lfs_frombe32(tag) ^ ptag;
// next commit not yet programmed?
if (!lfs_tag_isvalid(tag)) {
maybeerased = true;
break;
// out of range? // out of range?
if (off + lfs_tag_dsize(tag) > lfs->cfg->block_size) { } else if (off + lfs_tag_dsize(tag) > lfs->cfg->block_size) {
dir->erased = false;
break; break;
} }
ptag = tag; ptag = tag;
if (lfs_tag_type2(tag) == LFS_TYPE_CRC) { if (lfs_tag_type2(tag) == LFS_TYPE_CCRC) {
// check the crc attr // check the crc attr
uint32_t dcrc; uint32_t dcrc;
err = lfs_bd_read(lfs, err = lfs_bd_read(lfs,
@@ -1120,7 +1124,6 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
dir->pair[0], off+sizeof(tag), &dcrc, sizeof(dcrc)); dir->pair[0], off+sizeof(tag), &dcrc, sizeof(dcrc));
if (err) { if (err) {
if (err == LFS_ERR_CORRUPT) { if (err == LFS_ERR_CORRUPT) {
dir->erased = false;
break; break;
} }
return err; return err;
@@ -1128,10 +1131,12 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
dcrc = lfs_fromle32(dcrc); dcrc = lfs_fromle32(dcrc);
if (crc != dcrc) { if (crc != dcrc) {
dir->erased = false;
break; break;
} }
// reset the next bit if we need to
ptag ^= (lfs_tag_t)(lfs_tag_chunk(tag) & 1U) << 31;
// toss our crc into the filesystem seed for // toss our crc into the filesystem seed for
// pseudorandom numbers, note we use another crc here // pseudorandom numbers, note we use another crc here
// as a collection function because it is sufficiently // as a collection function because it is sufficiently
@@ -1147,50 +1152,14 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
dir->tail[1] = temptail[1]; dir->tail[1] = temptail[1];
dir->split = tempsplit; dir->split = tempsplit;
// check for an fcrc matching the next prog's erased state, if
// this failed most likely a previous prog was interrupted, we
// need a new erase
if (hasfcrc) {
// this may look inefficient, but since cache_size is
// probably > prog_size, the data will always remain in
// cache for the next iteration
// first read the leading byte, this always contains a bit
// we can perturb to avoid writes that don't change the fcrc
uint8_t eperturb;
err = lfs_bd_read(lfs,
NULL, &lfs->rcache, lfs->cfg->block_size,
dir->pair[0], dir->off, &eperturb, 1);
if (err && err != LFS_ERR_CORRUPT) {
return err;
}
// perturb valid bit?
dir->etag |= (0x80 & ~eperturb) << 24;
// crc the full prog_size, don't bother avoiding a reread
// of the eperturb, it should still be in our cache
uint32_t ecrc = 0xffffffff;
err = lfs_bd_crc(lfs,
NULL, &lfs->rcache, lfs->cfg->block_size,
dir->pair[0], dir->off, fcrc.size, &ecrc);
if (err && err != LFS_ERR_CORRUPT) {
return err;
}
// found beginning of erased part?
if (ecrc == fcrc.crc) {
dir->erased = true;
break;
}
}
// reset crc // reset crc
crc = 0xffffffff; crc = 0xffffffff;
hasfcrc = false;
continue; continue;
} }
// fcrc is only valid when last tag was a crc
hasfcrc = false;
// crc the entry first, hopefully leaving it in the cache // crc the entry first, hopefully leaving it in the cache
err = lfs_bd_crc(lfs, err = lfs_bd_crc(lfs,
NULL, &lfs->rcache, lfs->cfg->block_size, NULL, &lfs->rcache, lfs->cfg->block_size,
@@ -1198,7 +1167,6 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
lfs_tag_dsize(tag)-sizeof(tag), &crc); lfs_tag_dsize(tag)-sizeof(tag), &crc);
if (err) { if (err) {
if (err == LFS_ERR_CORRUPT) { if (err == LFS_ERR_CORRUPT) {
dir->erased = false;
break; break;
} }
return err; return err;
@@ -1228,7 +1196,6 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
dir->pair[0], off+sizeof(tag), &temptail, 8); dir->pair[0], off+sizeof(tag), &temptail, 8);
if (err) { if (err) {
if (err == LFS_ERR_CORRUPT) { if (err == LFS_ERR_CORRUPT) {
dir->erased = false;
break; break;
} }
return err; return err;
@@ -1241,7 +1208,6 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
&fcrc, sizeof(fcrc)); &fcrc, sizeof(fcrc));
if (err) { if (err) {
if (err == LFS_ERR_CORRUPT) { if (err == LFS_ERR_CORRUPT) {
dir->erased = false;
break; break;
} }
} }
@@ -1256,7 +1222,6 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
dir->pair[0], off+sizeof(tag)}); dir->pair[0], off+sizeof(tag)});
if (res < 0) { if (res < 0) {
if (res == LFS_ERR_CORRUPT) { if (res == LFS_ERR_CORRUPT) {
dir->erased = false;
break; break;
} }
return res; return res;
@@ -1278,35 +1243,54 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
} }
} }
// consider what we have good enough // found no valid commits?
if (dir->off > 0) { if (dir->off == 0) {
// synthetic move // try the other block?
if (lfs_gstate_hasmovehere(&lfs->gdisk, dir->pair)) { lfs_pair_swap(dir->pair);
if (lfs_tag_id(lfs->gdisk.tag) == lfs_tag_id(besttag)) { dir->rev = revs[(r+1)%2];
besttag |= 0x80000000; continue;
} else if (besttag != -1 && }
lfs_tag_id(lfs->gdisk.tag) < lfs_tag_id(besttag)) {
besttag -= LFS_MKTAG(0, 1, 0); // did we end on a valid commit? we may have an erased block
} dir->erased = false;
if (maybeerased && hasfcrc && dir->off % lfs->cfg->prog_size == 0) {
// check for an fcrc matching the next prog's erased state, if
// this failed most likely a previous prog was interrupted, we
// need a new erase
uint32_t fcrc_ = 0xffffffff;
int err = lfs_bd_crc(lfs,
NULL, &lfs->rcache, lfs->cfg->block_size,
dir->pair[0], dir->off, fcrc.size, &fcrc_);
if (err && err != LFS_ERR_CORRUPT) {
return err;
} }
// found tag? or found best id? // found beginning of erased part?
if (id) { dir->erased = (fcrc_ == fcrc.crc);
*id = lfs_min(lfs_tag_id(besttag), dir->count); }
}
if (lfs_tag_isvalid(besttag)) { // synthetic move
return besttag; if (lfs_gstate_hasmovehere(&lfs->gdisk, dir->pair)) {
} else if (lfs_tag_id(besttag) < dir->count) { if (lfs_tag_id(lfs->gdisk.tag) == lfs_tag_id(besttag)) {
return LFS_ERR_NOENT; besttag |= 0x80000000;
} else { } else if (besttag != -1 &&
return 0; lfs_tag_id(lfs->gdisk.tag) < lfs_tag_id(besttag)) {
besttag -= LFS_MKTAG(0, 1, 0);
} }
} }
// failed, try the other block? // found tag? or found best id?
lfs_pair_swap(dir->pair); if (id) {
dir->rev = revs[(r+1)%2]; *id = lfs_min(lfs_tag_id(besttag), dir->count);
}
if (lfs_tag_isvalid(besttag)) {
return besttag;
} else if (lfs_tag_id(besttag) < dir->count) {
return LFS_ERR_NOENT;
} else {
return 0;
}
} }
LFS_ERROR("Corrupted dir pair at {0x%"PRIx32", 0x%"PRIx32"}", LFS_ERROR("Corrupted dir pair at {0x%"PRIx32", 0x%"PRIx32"}",
@@ -1598,8 +1582,9 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
// padding is not crced, which lets fetches skip padding but // padding is not crced, which lets fetches skip padding but
// makes committing a bit more complicated // makes committing a bit more complicated
while (commit->off < end) { while (commit->off < end) {
lfs_off_t noff = lfs_min(end - (commit->off+sizeof(lfs_tag_t)), 0x3fe) lfs_off_t noff = (
+ (commit->off+sizeof(lfs_tag_t)); lfs_min(end - (commit->off+sizeof(lfs_tag_t)), 0x3fe)
+ (commit->off+sizeof(lfs_tag_t)));
// too large for crc tag? need padding commits // too large for crc tag? need padding commits
if (noff < end) { if (noff < end) {
noff = lfs_min(noff, end - 5*sizeof(uint32_t)); noff = lfs_min(noff, end - 5*sizeof(uint32_t));
@@ -1607,7 +1592,7 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
// space for fcrc? // space for fcrc?
uint8_t eperturb = -1; uint8_t eperturb = -1;
if (noff < lfs->cfg->block_size) { if (noff >= end && noff <= lfs->cfg->block_size - lfs->cfg->prog_size) {
// first read the leading byte, this always contains a bit // first read the leading byte, this always contains a bit
// we can perturb to avoid writes that don't change the fcrc // we can perturb to avoid writes that don't change the fcrc
int err = lfs_bd_read(lfs, int err = lfs_bd_read(lfs,
@@ -1619,15 +1604,9 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
// find the expected fcrc, don't bother avoiding a reread // find the expected fcrc, don't bother avoiding a reread
// of the eperturb, it should still be in our cache // of the eperturb, it should still be in our cache
struct lfs_fcrc fcrc = { struct lfs_fcrc fcrc = {.size=lfs->cfg->prog_size, .crc=0xffffffff};
// if our commit is a padding commit, we only care about
// invalidating outdated commits if there is a partial write,
// so we fcrc the minimum amount (1 byte)
.size=(noff < end ? 1 : lfs->cfg->prog_size),
.crc=0xffffffff,
};
err = lfs_bd_crc(lfs, err = lfs_bd_crc(lfs,
NULL, &lfs->rcache, fcrc.size, NULL, &lfs->rcache, lfs->cfg->prog_size,
commit->block, noff, fcrc.size, &fcrc.crc); commit->block, noff, fcrc.size, &fcrc.crc);
if (err && err != LFS_ERR_CORRUPT) { if (err && err != LFS_ERR_CORRUPT) {
return err; return err;
@@ -1647,7 +1626,8 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
lfs_tag_t tag; lfs_tag_t tag;
uint32_t crc; uint32_t crc;
} ccrc; } ccrc;
lfs_tag_t ntag = LFS_MKTAG(LFS_TYPE_CCRC, 0x3ff, lfs_tag_t ntag = LFS_MKTAG(
LFS_TYPE_CCRC + (((uint8_t)~eperturb) >> 7), 0x3ff,
noff - (commit->off+sizeof(lfs_tag_t))); noff - (commit->off+sizeof(lfs_tag_t)));
ccrc.tag = lfs_tobe32(ntag ^ commit->ptag); ccrc.tag = lfs_tobe32(ntag ^ commit->ptag);
commit->crc = lfs_crc(commit->crc, &ccrc.tag, sizeof(lfs_tag_t)); commit->crc = lfs_crc(commit->crc, &ccrc.tag, sizeof(lfs_tag_t));
@@ -1668,15 +1648,19 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
commit->off = noff; commit->off = noff;
// perturb valid bit? // perturb valid bit?
commit->ptag = ntag | ((0x80 & ~eperturb) << 24); commit->ptag = ntag ^ ((0x80 & ~eperturb) << 24);
// reset crc for next commit // reset crc for next commit
commit->crc = 0xffffffff; commit->crc = 0xffffffff;
}
// flush buffers // manually flush here since we don't prog the padding, this confuses
int err = lfs_bd_sync(lfs, &lfs->pcache, &lfs->rcache, false); // the caching layer
if (err) { if (noff >= end || noff >= lfs->pcache.off + lfs->cfg->cache_size) {
return err; // flush buffers
int err = lfs_bd_sync(lfs, &lfs->pcache, &lfs->rcache, false);
if (err) {
return err;
}
}
} }
// successful commit, check checksums to make sure // successful commit, check checksums to make sure
@@ -1685,7 +1669,7 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
// case if they are corrupted we would have had to compact anyways // case if they are corrupted we would have had to compact anyways
lfs_off_t off = commit->begin; lfs_off_t off = commit->begin;
uint32_t crc = 0xffffffff; uint32_t crc = 0xffffffff;
err = lfs_bd_crc(lfs, int err = lfs_bd_crc(lfs,
NULL, &lfs->rcache, off1+sizeof(uint32_t), NULL, &lfs->rcache, off1+sizeof(uint32_t),
commit->block, off, off1-off, &crc); commit->block, off, off1-off, &crc);
if (err) { if (err) {

2
lfs.h
View File

@@ -112,7 +112,7 @@ enum lfs_type {
LFS_TYPE_SOFTTAIL = 0x600, LFS_TYPE_SOFTTAIL = 0x600,
LFS_TYPE_HARDTAIL = 0x601, LFS_TYPE_HARDTAIL = 0x601,
LFS_TYPE_MOVESTATE = 0x7ff, LFS_TYPE_MOVESTATE = 0x7ff,
LFS_TYPE_CCRC = 0x502, LFS_TYPE_CCRC = 0x500,
LFS_TYPE_FCRC = 0x5ff, LFS_TYPE_FCRC = 0x5ff,
// internal chip sources // internal chip sources

View File

@@ -23,7 +23,8 @@ TAG_TYPES = {
'hardtail': (0x7ff, 0x601), 'hardtail': (0x7ff, 0x601),
'gstate': (0x700, 0x700), 'gstate': (0x700, 0x700),
'movestate': (0x7ff, 0x7ff), 'movestate': (0x7ff, 0x7ff),
'crc': (0x780, 0x500), 'crc': (0x700, 0x500),
'ccrc': (0x780, 0x500),
'fcrc': (0x7ff, 0x5ff), 'fcrc': (0x7ff, 0x5ff),
} }
@@ -121,12 +122,13 @@ class Tag:
ntag = Tag(self.type, nid, self.size) ntag = Tag(self.type, nid, self.size)
if hasattr(self, 'off'): ntag.off = self.off if hasattr(self, 'off'): ntag.off = self.off
if hasattr(self, 'data'): ntag.data = self.data if hasattr(self, 'data'): ntag.data = self.data
if hasattr(self, 'crc'): ntag.crc = self.crc if hasattr(self, 'ccrc'): ntag.crc = self.crc
if hasattr(self, 'erased'): ntag.erased = self.erased if hasattr(self, 'erased'): ntag.erased = self.erased
return ntag return ntag
def typerepr(self): def typerepr(self):
if (self.is_('crc') and getattr(self, 'crc', 0xffffffff) != 0xffffffff): if (self.is_('ccrc')
and getattr(self, 'ccrc', 0xffffffff) != 0xffffffff):
crc_status = ' (bad)' crc_status = ' (bad)'
elif self.is_('fcrc') and getattr(self, 'erased', False): elif self.is_('fcrc') and getattr(self, 'erased', False):
crc_status = ' (era)' crc_status = ' (era)'
@@ -187,8 +189,8 @@ class MetadataPair:
self.rev, = struct.unpack('<I', block[0:4]) self.rev, = struct.unpack('<I', block[0:4])
crc = binascii.crc32(block[0:4]) crc = binascii.crc32(block[0:4])
etag = None fcrctag = None
estate = None fcrcdata = None
# parse tags # parse tags
corrupt = False corrupt = False
@@ -202,7 +204,7 @@ class MetadataPair:
tag = Tag((int(tag) ^ ntag) & 0x7fffffff) tag = Tag((int(tag) ^ ntag) & 0x7fffffff)
tag.off = off + 4 tag.off = off + 4
tag.data = block[off+4:off+tag.dsize] tag.data = block[off+4:off+tag.dsize]
if tag.is_('crc'): if tag.is_('ccrc'):
crc = binascii.crc32(block[off:off+2*4], crc) crc = binascii.crc32(block[off:off+2*4], crc)
else: else:
crc = binascii.crc32(block[off:off+tag.dsize], crc) crc = binascii.crc32(block[off:off+tag.dsize], crc)
@@ -212,26 +214,28 @@ class MetadataPair:
self.all_.append(tag) self.all_.append(tag)
if tag.is_('fcrc') and len(tag.data) == 8: if tag.is_('fcrc') and len(tag.data) == 8:
etag = tag fcrctag = tag
estate = struct.unpack('<II', tag.data) fcrcdata = struct.unpack('<II', tag.data)
elif tag.is_('crc'): elif tag.is_('ccrc'):
# is valid commit? # is valid commit?
if crc != 0xffffffff: if crc != 0xffffffff:
corrupt = True corrupt = True
if not corrupt: if not corrupt:
self.log = self.all_.copy() self.log = self.all_.copy()
# end of commit? # end of commit?
if estate: if fcrcdata:
esize, ecrc = estate fcrcsize, fcrc = fcrcdata
dcrc = 0xffffffff ^ binascii.crc32(block[off:off+esize]) fcrc_ = 0xffffffff ^ binascii.crc32(
if ecrc == dcrc: block[off:off+fcrcsize])
etag.erased = True if fcrc_ == fcrc:
fcrctag.erased = True
corrupt = True corrupt = True
# reset tag parsing # reset tag parsing
crc = 0 crc = 0
etag = None tag = Tag(int(tag) ^ ((tag.type & 1) << 31))
estate = None fcrctag = None
fcrcdata = None
# find active ids # find active ids
self.ids = list(it.takewhile( self.ids = list(it.takewhile(
@@ -241,7 +245,7 @@ class MetadataPair:
# find most recent tags # find most recent tags
self.tags = [] self.tags = []
for tag in self.log: for tag in self.log:
if tag.is_('crc') or tag.is_('fcrc') or tag.is_('splice'): if tag.is_('crc') or tag.is_('splice'):
continue continue
elif tag.id == 0x3ff: elif tag.id == 0x3ff:
if tag in self and self[tag] is tag: if tag in self and self[tag] is tag: