From 52dd83096be5826c9e03f33fe45b279bab3820ea Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sat, 5 Dec 2020 14:29:52 -0600 Subject: [PATCH 1/6] Initial implementation of forward-looking erase-state CRCs This change is necessary to handle out-of-order writes found by pjsg's fuzzing work. The problem is that it is possible for (non-NOR) block devices to write pages in any order, or to even write random data in the case of a power-loss. This breaks littlefs's use of the first bit in a page to indicate the erase-state. pjsg notes this behavior is documented in the W25Q here: https://community.cypress.com/docs/DOC-10507 --- The basic idea here is to CRC the next page, and use this "erase-state CRC" to check if the next page is erased and ready to accept programs. .------------------. \ commit | metadata | | | | +---. | | | | |------------------| | | | erase-state CRC -----. | |------------------| | | | | commit CRC ---|-|-' |------------------| / | | padding | | padding (doesn't need CRC) | | | |------------------| \ | next prog | erased? | +-' | | | | | v | / | | | | '------------------' This is made a bit annoying since littlefs doesn't actually store the page (prog_size) in the superblock, since it doesn't need to know the size for any other operation. We can work around this by storing both the CRC and size of the next page when necessary. Another interesting note is that we don't need to any bit tweaking information, since we read the next page every time we would need to know how to clobber the erase-state CRC. And since we only read prog_size, this works really well with our caching, since the caches must be a multiple of prog_size. This also brings back the internal lfs_bd_crc function, in which we can use some optimizations added to lfs_bd_cmp. Needs some cleanup but the idea is passing most relevant tests. --- lfs.c | 253 ++++++++++++++++++++++++++++++-------- scripts/readmdir.py | 51 ++++++-- tests/test_powerloss.toml | 181 +++++++++++++++++++++++++++ 3 files changed, 419 insertions(+), 66 deletions(-) create mode 100644 tests/test_powerloss.toml diff --git a/lfs.c b/lfs.c index 48a10e8e..abd5e649 100644 --- a/lfs.c +++ b/lfs.c @@ -133,16 +133,15 @@ static int lfs_bd_cmp(lfs_t *lfs, for (lfs_off_t i = 0; i < size; i += diff) { uint8_t dat[8]; - diff = lfs_min(size-i, sizeof(dat)); - int res = lfs_bd_read(lfs, + int err = lfs_bd_read(lfs, pcache, rcache, hint-i, block, off+i, &dat, diff); - if (res) { - return res; + if (err) { + return err; } - res = memcmp(dat, data + i, diff); + int res = memcmp(dat, data + i, diff); if (res) { return res < 0 ? LFS_CMP_LT : LFS_CMP_GT; } @@ -151,6 +150,27 @@ static int lfs_bd_cmp(lfs_t *lfs, return LFS_CMP_EQ; } +static int lfs_bd_crc(lfs_t *lfs, + const lfs_cache_t *pcache, lfs_cache_t *rcache, lfs_size_t hint, + lfs_block_t block, lfs_off_t off, lfs_size_t size, uint32_t *crc) { + lfs_size_t diff = 0; + + for (lfs_off_t i = 0; i < size; i += diff) { + uint8_t dat[8]; + diff = lfs_min(size-i, sizeof(dat)); + int err = lfs_bd_read(lfs, + pcache, rcache, hint-i, + block, off+i, &dat, diff); + if (err) { + return err; + } + + *crc = lfs_crc(*crc, &dat, diff); + } + + return 0; +} + #ifndef LFS_READONLY static int lfs_bd_flush(lfs_t *lfs, lfs_cache_t *pcache, lfs_cache_t *rcache, bool validate) { @@ -413,6 +433,22 @@ static inline void lfs_gstate_tole32(lfs_gstate_t *a) { } #endif +// operations on estate in CRC tags +struct lfs_estate { + lfs_size_t size; + uint32_t crc; +}; + +static void lfs_estate_fromle32(struct lfs_estate *estate) { + estate->size = lfs_fromle32(estate->size); + estate->crc = lfs_fromle32(estate->crc); +} + +static void lfs_estate_tole32(struct lfs_estate *estate) { + estate->size = lfs_tole32(estate->size); + estate->crc = lfs_tole32(estate->crc); +} + // other endianness operations static void lfs_ctz_fromle32(struct lfs_ctz *ctz) { ctz->head = lfs_fromle32(ctz->head); @@ -1056,14 +1092,10 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, } crc = lfs_crc(crc, &tag, sizeof(tag)); - tag = lfs_frombe32(tag) ^ ptag; + tag = (lfs_frombe32(tag) ^ ptag) & 0x7fffffff; - // next commit not yet programmed or we're not in valid range - if (!lfs_tag_isvalid(tag)) { - dir->erased = (lfs_tag_type1(ptag) == LFS_TYPE_CRC && - dir->off % lfs->cfg->prog_size == 0); - break; - } else if (off + lfs_tag_dsize(tag) > lfs->cfg->block_size) { + // out of range? + if (off + lfs_tag_dsize(tag) > lfs->cfg->block_size) { dir->erased = false; break; } @@ -1071,11 +1103,31 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, ptag = tag; if (lfs_tag_type1(tag) == LFS_TYPE_CRC) { + lfs_off_t noff = off + sizeof(tag); + struct lfs_estate estate; + + if (lfs_tag_chunk(tag) == 3) { + err = lfs_bd_read(lfs, + NULL, &lfs->rcache, lfs->cfg->block_size, + dir->pair[0], noff, &estate, sizeof(estate)); + if (err) { + if (err == LFS_ERR_CORRUPT) { + dir->erased = false; + break; + } + return err; + } + + crc = lfs_crc(crc, &estate, sizeof(estate)); + lfs_estate_fromle32(&estate); + noff += sizeof(estate); + } + // check the crc attr uint32_t dcrc; err = lfs_bd_read(lfs, NULL, &lfs->rcache, lfs->cfg->block_size, - dir->pair[0], off+sizeof(tag), &dcrc, sizeof(dcrc)); + dir->pair[0], noff, &dcrc, sizeof(dcrc)); if (err) { if (err == LFS_ERR_CORRUPT) { dir->erased = false; @@ -1090,9 +1142,6 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, 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 // pseudorandom numbers, note we use another crc here // as a collection function because it is sufficiently @@ -1110,24 +1159,72 @@ 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) { + // 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; + } + return err; + } + + // perturb valid bit? + dir->etag |= 0x80000000 & ~lfs_frombe32(etag); + + // crc the rest the full prog_size, including etag in case + // the actual esize is < tag size (though this shouldn't + // happen normally) + uint32_t tcrc = 0xffffffff; + 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; + } + return err; + } + + if (tcrc == estate.crc) { + dir->erased = true; + break; + } + } else { + // end of block commit + // TODO handle backwards compat? + dir->erased = false; + break; + } + continue; } // crc the entry first, hopefully leaving it in the cache - for (lfs_off_t j = sizeof(tag); j < lfs_tag_dsize(tag); j++) { - uint8_t dat; - err = lfs_bd_read(lfs, - NULL, &lfs->rcache, lfs->cfg->block_size, - dir->pair[0], off+j, &dat, 1); - if (err) { - if (err == LFS_ERR_CORRUPT) { - dir->erased = false; - break; - } - return err; + 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; } - - crc = lfs_crc(crc, &dat, 1); + return err; } // directory modification tags? @@ -1494,9 +1591,18 @@ static int lfs_dir_commitattr(lfs_t *lfs, struct lfs_commit *commit, #ifndef LFS_READONLY static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { // align to program units - const lfs_off_t end = lfs_alignup(commit->off + 2*sizeof(uint32_t), + // + // this gets a bit complex as we have two types of crcs: + // - 4-word crc with estate to check following prog (middle of block) + // - 2-word crc with no following prog (end of block) + const lfs_off_t end = lfs_alignup( + lfs_min(commit->off + 4*sizeof(uint32_t), lfs->cfg->block_size), lfs->cfg->prog_size); + // clamp erase size to tag size, this gives us the full tag as potential + // to intentionally invalidate erase CRCs + const lfs_size_t esize = lfs_max(lfs->cfg->prog_size, sizeof(lfs_tag_t)); + lfs_off_t off1 = 0; uint32_t crc1 = 0; @@ -1510,40 +1616,77 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { noff = lfs_min(noff, end - 2*sizeof(uint32_t)); } - // read erased state from next program unit - lfs_tag_t tag = 0xffffffff; - int err = lfs_bd_read(lfs, - NULL, &lfs->rcache, sizeof(tag), - commit->block, noff, &tag, sizeof(tag)); - if (err && err != LFS_ERR_CORRUPT) { - return err; + // build crc tag + lfs_tag_t etag = 0; + lfs_tag_t tag = LFS_MKTAG(LFS_TYPE_CRC + 2, 0x3ff, noff - off); + lfs_size_t size = 2*sizeof(uint32_t); + struct { + lfs_tag_t tag; + union { + struct { + uint32_t crc; + } crc2; + struct { + struct lfs_estate estate; + uint32_t crc; + } crc3; + } u; + } data; + + if (noff <= lfs->cfg->block_size - esize) { + // 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 + 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; + } + + // find expected erased state + uint32_t ecrc = 0xffffffff; + 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; + } + + data.u.crc3.estate.size = esize; + data.u.crc3.estate.crc = ecrc; + lfs_estate_tole32(&data.u.crc3.estate); + + // indicate we include estate + tag |= LFS_MKTAG(1, 0, 0); + size += sizeof(struct lfs_estate); } - // build crc tag - bool reset = ~lfs_frombe32(tag) >> 31; - tag = LFS_MKTAG(LFS_TYPE_CRC + reset, 0x3ff, noff - off); + data.tag = lfs_tobe32(tag ^ commit->ptag); + commit->crc = lfs_crc(commit->crc, &data, size-sizeof(uint32_t)); + ((uint32_t*)&data)[size/sizeof(uint32_t) - 1] = lfs_tole32(commit->crc); - // write out crc - uint32_t footer[2]; - footer[0] = lfs_tobe32(tag ^ commit->ptag); - commit->crc = lfs_crc(commit->crc, &footer[0], sizeof(footer[0])); - footer[1] = lfs_tole32(commit->crc); - err = lfs_bd_prog(lfs, + int err = lfs_bd_prog(lfs, &lfs->pcache, &lfs->rcache, false, - commit->block, commit->off, &footer, sizeof(footer)); + commit->block, commit->off, &data, size); if (err) { return err; } // keep track of non-padding checksum to verify if (off1 == 0) { - off1 = commit->off + sizeof(uint32_t); + //off1 = commit->off + sizeof(uint32_t); + off1 = commit->off + size-sizeof(uint32_t); crc1 = commit->crc; } commit->off += sizeof(tag)+lfs_tag_size(tag); - commit->ptag = tag ^ ((lfs_tag_t)reset << 31); - commit->crc = 0xffffffff; // reset crc for next "commit" + // perturb valid bit? + commit->ptag = tag | (0x80000000 & ~lfs_frombe32(etag)); + // reset crc for next commit + commit->crc = 0xffffffff; } // flush buffers @@ -1556,6 +1699,7 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { 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 @@ -1564,7 +1708,6 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { return LFS_ERR_CORRUPT; } - // leave it up to caching to make this efficient uint8_t dat; err = lfs_bd_read(lfs, NULL, &lfs->rcache, noff+sizeof(uint32_t)-i, @@ -1581,12 +1724,16 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { return LFS_ERR_CORRUPT; } - // skip padding - off = lfs_min(end - noff, 0x3fe) + noff; + // 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); + } } return 0; diff --git a/scripts/readmdir.py b/scripts/readmdir.py index b6c3dcca..cf6d9d88 100755 --- a/scripts/readmdir.py +++ b/scripts/readmdir.py @@ -99,7 +99,16 @@ class Tag: return struct.unpack('b', struct.pack('B', self.chunk))[0] def is_(self, type): - return (self.type & TAG_TYPES[type][0]) == TAG_TYPES[type][1] + try: + if ' ' in type: + type1, type3 = type.split() + return (self.is_(type1) and + (self.type & ~TAG_TYPES[type1][0]) == int(type3, 0)) + + return self.type == int(type, 0) + + except (ValueError, KeyError): + return (self.type & TAG_TYPES[type][0]) == TAG_TYPES[type][1] def mkmask(self): return Tag( @@ -109,14 +118,19 @@ class Tag: def chid(self, nid): ntag = Tag(self.type, nid, self.size) - if hasattr(self, 'off'): ntag.off = self.off - if hasattr(self, 'data'): ntag.data = self.data - if hasattr(self, 'crc'): ntag.crc = self.crc + if hasattr(self, 'off'): ntag.off = self.off + if hasattr(self, 'data'): ntag.data = self.data + if hasattr(self, 'crc'): ntag.crc = self.crc + if hasattr(self, 'erased'): ntag.erased = self.erased return ntag def typerepr(self): if self.is_('crc') and getattr(self, 'crc', 0xffffffff) != 0xffffffff: - return 'crc (bad)' + crc_status = ' (bad)' + elif self.is_('crc') and getattr(self, 'erased', False): + crc_status = ' (era)' + else: + crc_status = '' reverse_types = {v: k for k, v in TAG_TYPES.items()} for prefix in range(12): @@ -124,12 +138,12 @@ class Tag: if (mask, self.type & mask) in reverse_types: type = reverse_types[mask, self.type & mask] if prefix > 0: - return '%s %#0*x' % ( - type, prefix//4, self.type & ((1 << prefix)-1)) + return '%s %#x%s' % ( + type, self.type & ((1 << prefix)-1), crc_status) else: - return type + return '%s%s' % (type, crc_status) else: - return '%02x' % self.type + return '%02x%s' % (self.type, crc_status) def idrepr(self): return repr(self.id) if self.id != 0x3ff else '.' @@ -182,11 +196,13 @@ class MetadataPair: while len(block) - off >= 4: ntag, = struct.unpack('>I', block[off:off+4]) - tag = Tag(int(tag) ^ ntag) + tag = Tag((int(tag) ^ ntag) & 0x7fffffff) tag.off = off + 4 tag.data = block[off+4:off+tag.dsize] - if tag.is_('crc'): - crc = binascii.crc32(block[off:off+4+4], crc) + if tag.is_('crc 0x3'): + crc = binascii.crc32(block[off:off+4*4], crc) + elif tag.is_('crc'): + crc = binascii.crc32(block[off:off+2*4], crc) else: crc = binascii.crc32(block[off:off+tag.dsize], crc) tag.crc = crc @@ -201,9 +217,18 @@ class MetadataPair: if not corrupt: self.log = self.all_.copy() + # end of commit? + if tag.is_('crc 0x3'): + esize, ecrc = struct.unpack(' 0; + + lfs_mount(&lfs, cfg) => 0; + lfs_mkdir(&lfs, "notebook") => 0; + lfs_file_t file; + lfs_file_open(&lfs, &file, "notebook/paper", + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_APPEND) => 0; + char buffer[256]; + strcpy(buffer, "hello"); + lfs_size_t size = strlen("hello"); + for (int i = 0; i < 5; i++) { + lfs_file_write(&lfs, &file, buffer, size) => size; + lfs_file_sync(&lfs, &file) => 0; + } + lfs_file_close(&lfs, &file) => 0; + + char rbuffer[256]; + lfs_file_open(&lfs, &file, "notebook/paper", LFS_O_RDONLY) => 0; + for (int i = 0; i < 5; i++) { + lfs_file_read(&lfs, &file, rbuffer, size) => size; + assert(memcmp(rbuffer, buffer, size) == 0); + } + lfs_file_close(&lfs, &file) => 0; + lfs_unmount(&lfs) => 0; + + // get pair/rev count + lfs_mount(&lfs, cfg) => 0; + lfs_dir_t dir; + lfs_dir_open(&lfs, &dir, "notebook") => 0; + lfs_block_t pair[2] = {dir.m.pair[0], dir.m.pair[1]}; + uint32_t rev = dir.m.rev; + lfs_dir_close(&lfs, &dir) => 0; + lfs_unmount(&lfs) => 0; + + // write just the revision count + uint8_t bbuffer[BLOCK_SIZE]; + cfg->read(cfg, pair[1], 0, bbuffer, BLOCK_SIZE) => 0; + + memcpy(bbuffer, &(uint32_t){lfs_tole32(rev+1)}, sizeof(uint32_t)); + + cfg->erase(cfg, pair[1]) => 0; + cfg->prog(cfg, pair[1], 0, bbuffer, BLOCK_SIZE) => 0; + + lfs_mount(&lfs, cfg) => 0; + + // can read? + lfs_file_open(&lfs, &file, "notebook/paper", LFS_O_RDONLY) => 0; + for (int i = 0; i < 5; i++) { + lfs_file_read(&lfs, &file, rbuffer, size) => size; + assert(memcmp(rbuffer, buffer, size) == 0); + } + lfs_file_close(&lfs, &file) => 0; + + // can write? + lfs_file_open(&lfs, &file, "notebook/paper", + LFS_O_WRONLY | LFS_O_APPEND) => 0; + strcpy(buffer, "goodbye"); + size = strlen("goodbye"); + for (int i = 0; i < 5; i++) { + lfs_file_write(&lfs, &file, buffer, size) => size; + lfs_file_sync(&lfs, &file) => 0; + } + lfs_file_close(&lfs, &file) => 0; + + lfs_file_open(&lfs, &file, "notebook/paper", LFS_O_RDONLY) => 0; + strcpy(buffer, "hello"); + size = strlen("hello"); + for (int i = 0; i < 5; i++) { + lfs_file_read(&lfs, &file, rbuffer, size) => size; + assert(memcmp(rbuffer, buffer, size) == 0); + } + strcpy(buffer, "goodbye"); + size = strlen("goodbye"); + for (int i = 0; i < 5; i++) { + lfs_file_read(&lfs, &file, rbuffer, size) => size; + assert(memcmp(rbuffer, buffer, size) == 0); + } + lfs_file_close(&lfs, &file) => 0; + + lfs_unmount(&lfs) => 0; +''' + +# partial prog, may not be byte in order! +[cases.test_powerloss_partial_prog] +defines.BYTE_OFF = ["0", "PROG_SIZE-1", "PROG_SIZE/2"] +defines.BYTE_VALUE = [0x33, 0xcc] +in = "lfs.c" +code = ''' + lfs_t lfs; + lfs_format(&lfs, cfg) => 0; + + lfs_mount(&lfs, cfg) => 0; + lfs_mkdir(&lfs, "notebook") => 0; + lfs_file_t file; + lfs_file_open(&lfs, &file, "notebook/paper", + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_APPEND) => 0; + char buffer[256]; + strcpy(buffer, "hello"); + lfs_size_t size = strlen("hello"); + for (int i = 0; i < 5; i++) { + lfs_file_write(&lfs, &file, buffer, size) => size; + lfs_file_sync(&lfs, &file) => 0; + } + lfs_file_close(&lfs, &file) => 0; + + char rbuffer[256]; + lfs_file_open(&lfs, &file, "notebook/paper", LFS_O_RDONLY) => 0; + for (int i = 0; i < 5; i++) { + lfs_file_read(&lfs, &file, rbuffer, size) => size; + assert(memcmp(rbuffer, buffer, size) == 0); + } + lfs_file_close(&lfs, &file) => 0; + lfs_unmount(&lfs) => 0; + + // imitate a partial prog, value should not matter, if littlefs + // doesn't notice the partial prog testbd will assert + + // get offset to next prog + lfs_mount(&lfs, cfg) => 0; + lfs_dir_t dir; + lfs_dir_open(&lfs, &dir, "notebook") => 0; + lfs_block_t block = dir.m.pair[0]; + lfs_off_t off = dir.m.off; + lfs_dir_close(&lfs, &dir) => 0; + lfs_unmount(&lfs) => 0; + + // tweak byte + uint8_t bbuffer[BLOCK_SIZE]; + cfg->read(cfg, block, 0, bbuffer, BLOCK_SIZE) => 0; + + bbuffer[off + BYTE_OFF] = BYTE_VALUE; + + cfg->erase(cfg, block) => 0; + cfg->prog(cfg, block, 0, bbuffer, BLOCK_SIZE) => 0; + + lfs_mount(&lfs, cfg) => 0; + + // can read? + lfs_file_open(&lfs, &file, "notebook/paper", LFS_O_RDONLY) => 0; + for (int i = 0; i < 5; i++) { + lfs_file_read(&lfs, &file, rbuffer, size) => size; + assert(memcmp(rbuffer, buffer, size) == 0); + } + lfs_file_close(&lfs, &file) => 0; + + // can write? + lfs_file_open(&lfs, &file, "notebook/paper", + LFS_O_WRONLY | LFS_O_APPEND) => 0; + strcpy(buffer, "goodbye"); + size = strlen("goodbye"); + for (int i = 0; i < 5; i++) { + lfs_file_write(&lfs, &file, buffer, size) => size; + lfs_file_sync(&lfs, &file) => 0; + } + lfs_file_close(&lfs, &file) => 0; + + lfs_file_open(&lfs, &file, "notebook/paper", LFS_O_RDONLY) => 0; + strcpy(buffer, "hello"); + size = strlen("hello"); + for (int i = 0; i < 5; i++) { + lfs_file_read(&lfs, &file, rbuffer, size) => size; + assert(memcmp(rbuffer, buffer, size) == 0); + } + strcpy(buffer, "goodbye"); + size = strlen("goodbye"); + for (int i = 0; i < 5; i++) { + lfs_file_read(&lfs, &file, rbuffer, size) => size; + assert(memcmp(rbuffer, buffer, size) == 0); + } + lfs_file_close(&lfs, &file) => 0; + + lfs_unmount(&lfs) => 0; +''' From 91ad673c4520c08006272ba3bad133fe21048887 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sun, 6 Dec 2020 00:20:09 -0600 Subject: [PATCH 2/6] Cleaned up a few additional commit corner cases - 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 --- lfs.c | 211 +++++++++++++++++--------------------- tests/test_powerloss.toml | 1 + 2 files changed, 97 insertions(+), 115 deletions(-) diff --git a/lfs.c b/lfs.c index abd5e649..c8f87b02 100644 --- a/lfs.c +++ b/lfs.c @@ -1160,12 +1160,13 @@ 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 @@ -1173,12 +1174,7 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, 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; } @@ -1192,11 +1188,7 @@ 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; } @@ -1204,51 +1196,21 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, dir->erased = true; break; } - } else { + } else if (lfs_tag_chunk(tag) == 2) { // end of block commit - // TODO handle backwards compat? 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 } - - 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; - } - 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; @@ -1256,33 +1218,64 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, } 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; + } } } } @@ -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; } @@ -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; } @@ -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; - } + 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; + } - 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; - } + // check against known crc for non-padding commits + if (crc != crc1) { + return LFS_ERR_CORRUPT; + } - crc = lfs_crc(crc, &dat, 1); - } + // 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; + } - // detected write error? - if (crc != 0) { - return LFS_ERR_CORRUPT; - } - - // 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; diff --git a/tests/test_powerloss.toml b/tests/test_powerloss.toml index 98afe4fe..06f8661d 100644 --- a/tests/test_powerloss.toml +++ b/tests/test_powerloss.toml @@ -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" From b4091c6871c05a5439b70db4db13df39e85f1639 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sun, 6 Dec 2020 23:54:55 -0600 Subject: [PATCH 3/6] Switched to separate-tag encoding of forward-looking CRCs Previously forward-looking CRCs was just two new CRC types, one for commits with forward-looking CRCs, one without. These both contained the CRC needed to complete the current commit (note that the commit CRC must come last!). [-- 32 --|-- 32 --|-- 32 --|-- 32 --] with: [ crc3 tag | nprog size | nprog crc | commit crc ] without: [ crc2 tag | commit crc ] This meant there had to be several checks for the two possible structure sizes, messying up the implementation. [-- 32 --|-- 32 --|-- 32 --|-- 32 --|-- 32 --] with: [nprogcrc tag| nprog size | nprog crc | commit tag | commit crc ] without: [ commit tag | commit crc ] But we already have a mechanism for storing optional metadata! The different metadata tags! So why not use a separate tage for the forward-looking CRC, separate from the commit CRC? I wasn't sure this would actually help that much, there are still necessary conditions for wether or not a forward-looking CRC is there, but in the end it simplified the code quite nicely, and resulted in a ~200 byte code-cost saving. --- lfs.c | 130 +++++++++++++++++++++----------------------- lfs.h | 2 + scripts/readmdir.py | 30 ++++++---- 3 files changed, 82 insertions(+), 80 deletions(-) diff --git a/lfs.c b/lfs.c index c8f87b02..853a646f 100644 --- a/lfs.c +++ b/lfs.c @@ -345,6 +345,10 @@ static inline uint16_t lfs_tag_type1(lfs_tag_t tag) { return (tag & 0x70000000) >> 20; } +static inline uint16_t lfs_tag_type2(lfs_tag_t tag) { + return (tag & 0x78000000) >> 20; +} + static inline uint16_t lfs_tag_type3(lfs_tag_t tag) { return (tag & 0x7ff00000) >> 20; } @@ -1071,6 +1075,9 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, bool tempsplit = false; lfs_stag_t tempbesttag = besttag; + bool hasestate = false; + struct lfs_estate estate; + dir->rev = lfs_tole32(dir->rev); uint32_t crc = lfs_crc(0xffffffff, &dir->rev, sizeof(dir->rev)); dir->rev = lfs_fromle32(dir->rev); @@ -1102,32 +1109,12 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, ptag = tag; - if (lfs_tag_type1(tag) == LFS_TYPE_CRC) { - lfs_off_t noff = off + sizeof(tag); - struct lfs_estate estate; - - if (lfs_tag_chunk(tag) == 3) { - err = lfs_bd_read(lfs, - NULL, &lfs->rcache, lfs->cfg->block_size, - dir->pair[0], noff, &estate, sizeof(estate)); - if (err) { - if (err == LFS_ERR_CORRUPT) { - dir->erased = false; - break; - } - return err; - } - - crc = lfs_crc(crc, &estate, sizeof(estate)); - lfs_estate_fromle32(&estate); - noff += sizeof(estate); - } - + if (lfs_tag_type2(tag) == LFS_TYPE_CRC) { // check the crc attr uint32_t dcrc; err = lfs_bd_read(lfs, NULL, &lfs->rcache, lfs->cfg->block_size, - dir->pair[0], noff, &dcrc, sizeof(dcrc)); + dir->pair[0], off+sizeof(tag), &dcrc, sizeof(dcrc)); if (err) { if (err == LFS_ERR_CORRUPT) { dir->erased = false; @@ -1157,10 +1144,7 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, dir->tail[1] = temptail[1]; dir->split = tempsplit; - // reset crc - crc = 0xffffffff; - - if (lfs_tag_chunk(tag) == 3) { + if (hasestate) { // check if the next page is erased // // this may look inefficient, but since cache_size is @@ -1196,15 +1180,19 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, dir->erased = true; break; } - } else if (lfs_tag_chunk(tag) == 2) { - // end of block commit - dir->erased = false; - break; - } else { + } else if (lfs_tag_chunk(tag) < 2) { // for backwards compatibility we fall through here on // unrecognized tags, leaving it up to the CRC to reject // bad commits + } else { + // end of block commit + dir->erased = false; + break; } + + // reset crc + crc = 0xffffffff; + hasestate = false; } else { // crc the entry first, hopefully leaving it in the cache err = lfs_bd_crc(lfs, @@ -1240,7 +1228,8 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, err = lfs_bd_read(lfs, NULL, &lfs->rcache, lfs->cfg->block_size, - dir->pair[0], off+sizeof(tag), &temptail, 8); + dir->pair[0], off+sizeof(tag), + &temptail, sizeof(temptail)); if (err) { if (err == LFS_ERR_CORRUPT) { dir->erased = false; @@ -1248,6 +1237,19 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, } } lfs_pair_fromle32(temptail); + } else if (lfs_tag_type1(tag) == LFS_TYPE_NPROGCRC) { + err = lfs_bd_read(lfs, + NULL, &lfs->rcache, lfs->cfg->block_size, + dir->pair[0], off+sizeof(tag), + &estate, sizeof(estate)); + if (err) { + if (err == LFS_ERR_CORRUPT) { + dir->erased = false; + break; + } + } + lfs_estate_fromle32(&estate); + hasestate = true; } // found a match for our fetcher? @@ -1589,13 +1591,9 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { // - 4-word crc with estate to check following prog (middle of block) // - 2-word crc with no following prog (end of block) const lfs_off_t end = lfs_alignup( - lfs_min(commit->off + 4*sizeof(uint32_t), lfs->cfg->block_size), + lfs_min(commit->off + 5*sizeof(uint32_t), lfs->cfg->block_size), lfs->cfg->prog_size); - // clamp erase size to tag size, this gives us the full tag as potential - // to intentionally invalidate erase CRCs - const lfs_size_t esize = lfs_max(lfs->cfg->prog_size, sizeof(lfs_tag_t)); - lfs_off_t off1 = 0; uint32_t crc1 = 0; @@ -1609,23 +1607,12 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { noff = lfs_min(noff, end - 2*sizeof(uint32_t)); } - // build crc tag + // clamp erase size to tag size, this gives us the full tag as potential + // to intentionally invalidate erase CRCs + const lfs_size_t esize = lfs_max( + lfs->cfg->prog_size, sizeof(lfs_tag_t)); lfs_tag_t etag = 0; - lfs_tag_t tag = LFS_MKTAG(LFS_TYPE_CRC + 2, 0x3ff, noff - off); - lfs_size_t size = 2*sizeof(uint32_t); - struct { - lfs_tag_t tag; - union { - struct { - uint32_t crc; - } crc2; - struct { - struct lfs_estate estate; - uint32_t crc; - } crc3; - } u; - } data; - + // space for estate? also only emit on last commit in padding commits if (noff <= lfs->cfg->block_size - esize) { // first we get a tag-worth of bits, this is so we can // tweak our current tag to force future writes to be @@ -1646,36 +1633,43 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { return err; } - data.u.crc3.estate.size = esize; - data.u.crc3.estate.crc = ecrc; - lfs_estate_tole32(&data.u.crc3.estate); - - // indicate we include estate - tag |= LFS_MKTAG(1, 0, 0); - size += sizeof(struct lfs_estate); + struct lfs_estate estate = {.size = esize, .crc = ecrc}; + lfs_estate_tole32(&estate); + err = lfs_dir_commitattr(lfs, commit, + LFS_MKTAG(LFS_TYPE_NPROGCRC, 0x3ff, sizeof(estate)), + &estate); + if (err) { + return err; + } } - data.tag = lfs_tobe32(tag ^ commit->ptag); - commit->crc = lfs_crc(commit->crc, &data, size-sizeof(uint32_t)); - ((uint32_t*)&data)[size/sizeof(uint32_t) - 1] = lfs_tole32(commit->crc); + // build crc state + off = commit->off + sizeof(lfs_tag_t); + struct { + lfs_tag_t tag; + uint32_t crc; + } cstate; + lfs_tag_t ctag = LFS_MKTAG(LFS_TYPE_COMMITCRC, 0x3ff, noff-off); + cstate.tag = lfs_tobe32(ctag ^ commit->ptag); + commit->crc = lfs_crc(commit->crc, &cstate.tag, sizeof(cstate.tag)); + cstate.crc = lfs_tole32(commit->crc); int err = lfs_bd_prog(lfs, &lfs->pcache, &lfs->rcache, false, - commit->block, commit->off, &data, size); + commit->block, commit->off, &cstate, sizeof(cstate)); if (err) { return err; } // keep track of non-padding checksum to verify if (off1 == 0) { - //off1 = commit->off + sizeof(uint32_t); - off1 = commit->off + size-sizeof(uint32_t); + off1 = off; crc1 = commit->crc; } - commit->off += sizeof(tag)+lfs_tag_size(tag); + commit->off = noff; // perturb valid bit? - commit->ptag = tag | (0x80000000 & ~lfs_frombe32(etag)); + commit->ptag = ctag | (0x80000000 & ~lfs_frombe32(etag)); // reset crc for next commit commit->crc = 0xffffffff; } diff --git a/lfs.h b/lfs.h index ab534526..c52e62c6 100644 --- a/lfs.h +++ b/lfs.h @@ -112,6 +112,8 @@ enum lfs_type { LFS_TYPE_SOFTTAIL = 0x600, LFS_TYPE_HARDTAIL = 0x601, LFS_TYPE_MOVESTATE = 0x7ff, + LFS_TYPE_COMMITCRC = 0x502, + LFS_TYPE_NPROGCRC = 0x5ff, // internal chip sources LFS_FROM_NOOP = 0x000, diff --git a/scripts/readmdir.py b/scripts/readmdir.py index cf6d9d88..e86b1d97 100755 --- a/scripts/readmdir.py +++ b/scripts/readmdir.py @@ -24,6 +24,7 @@ TAG_TYPES = { 'gstate': (0x700, 0x700), 'movestate': (0x7ff, 0x7ff), 'crc': (0x700, 0x500), + 'nprogcrc': (0x7ff, 0x5ff), } class Tag: @@ -125,9 +126,10 @@ class Tag: return ntag def typerepr(self): - if self.is_('crc') and getattr(self, 'crc', 0xffffffff) != 0xffffffff: + if (self.is_('crc') and not self.is_('nprogcrc') and + getattr(self, 'crc', 0xffffffff) != 0xffffffff): crc_status = ' (bad)' - elif self.is_('crc') and getattr(self, 'erased', False): + elif self.is_('nprogcrc') and getattr(self, 'erased', False): crc_status = ' (era)' else: crc_status = '' @@ -186,6 +188,8 @@ class MetadataPair: self.rev, = struct.unpack(' Date: Wed, 7 Dec 2022 23:02:19 -0600 Subject: [PATCH 4/6] Continued implementation of forward-crcs, adopted new test runners This fixes most of the remaining bugs (except one with multiple padding commits + noop erases in test_badblocks), with some other code tweaks. The biggest change was dropping reliance on end-of-block commits to know when to stop parsing commits. We can just continue to parse tags and rely on the crc for catch bad commits, avoiding a backwards-compatiblity hiccup. So no new commit tag. Also renamed nprogcrc -> fcrc and commitcrc -> ccrc and made naming in the code a bit more consistent. --- bd/lfs_emubd.c | 9 +- lfs.c | 290 ++++++++++++++++++++++---------------------- lfs.h | 4 +- scripts/readmdir.py | 17 ++- 4 files changed, 161 insertions(+), 159 deletions(-) diff --git a/bd/lfs_emubd.c b/bd/lfs_emubd.c index 97bcf035..29925538 100644 --- a/bd/lfs_emubd.c +++ b/bd/lfs_emubd.c @@ -584,13 +584,14 @@ lfs_emubd_swear_t lfs_emubd_wear(const struct lfs_config *cfg, wear = 0; } - LFS_EMUBD_TRACE("lfs_emubd_wear -> %"PRIu32, wear); + LFS_EMUBD_TRACE("lfs_emubd_wear -> %"PRIi32, wear); return wear; } int lfs_emubd_setwear(const struct lfs_config *cfg, lfs_block_t block, lfs_emubd_wear_t wear) { - LFS_EMUBD_TRACE("lfs_emubd_setwear(%p, %"PRIu32")", (void*)cfg, block); + LFS_EMUBD_TRACE("lfs_emubd_setwear(%p, %"PRIu32", %"PRIi32")", + (void*)cfg, block, wear); lfs_emubd_t *bd = cfg->context; // check if block is valid @@ -599,12 +600,12 @@ int lfs_emubd_setwear(const struct lfs_config *cfg, // set the wear lfs_emubd_block_t *b = lfs_emubd_mutblock(cfg, &bd->blocks[block]); if (!b) { - LFS_EMUBD_TRACE("lfs_emubd_setwear -> %"PRIu32, LFS_ERR_NOMEM); + LFS_EMUBD_TRACE("lfs_emubd_setwear -> %d", LFS_ERR_NOMEM); return LFS_ERR_NOMEM; } b->wear = wear; - LFS_EMUBD_TRACE("lfs_emubd_setwear -> %"PRIu32, 0); + LFS_EMUBD_TRACE("lfs_emubd_setwear -> %d", 0); return 0; } diff --git a/lfs.c b/lfs.c index 853a646f..7e9e53be 100644 --- a/lfs.c +++ b/lfs.c @@ -133,6 +133,7 @@ static int lfs_bd_cmp(lfs_t *lfs, for (lfs_off_t i = 0; i < size; i += diff) { uint8_t dat[8]; + diff = lfs_min(size-i, sizeof(dat)); int err = lfs_bd_read(lfs, pcache, rcache, hint-i, @@ -437,21 +438,23 @@ static inline void lfs_gstate_tole32(lfs_gstate_t *a) { } #endif -// operations on estate in CRC tags -struct lfs_estate { +// operations on forward-CRCs used to track erased state +struct lfs_fcrc { lfs_size_t size; uint32_t crc; }; -static void lfs_estate_fromle32(struct lfs_estate *estate) { - estate->size = lfs_fromle32(estate->size); - estate->crc = lfs_fromle32(estate->crc); +static void lfs_fcrc_fromle32(struct lfs_fcrc *fcrc) { + fcrc->size = lfs_fromle32(fcrc->size); + fcrc->crc = lfs_fromle32(fcrc->crc); } -static void lfs_estate_tole32(struct lfs_estate *estate) { - estate->size = lfs_tole32(estate->size); - estate->crc = lfs_tole32(estate->crc); +#ifndef LFS_READONLY +static void lfs_fcrc_tole32(struct lfs_fcrc *fcrc) { + fcrc->size = lfs_tole32(fcrc->size); + fcrc->crc = lfs_tole32(fcrc->crc); } +#endif // other endianness operations static void lfs_ctz_fromle32(struct lfs_ctz *ctz) { @@ -1075,8 +1078,8 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, bool tempsplit = false; lfs_stag_t tempbesttag = besttag; - bool hasestate = false; - struct lfs_estate estate; + bool hasfcrc = false; + struct lfs_fcrc fcrc; dir->rev = lfs_tole32(dir->rev); uint32_t crc = lfs_crc(0xffffffff, &dir->rev, sizeof(dir->rev)); @@ -1144,61 +1147,85 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, dir->tail[1] = temptail[1]; dir->split = tempsplit; - if (hasestate) { - // check if the next page is erased - // + // 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 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; + // 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, &etag, sizeof(etag)); + dir->pair[0], dir->off, &eperturb, 1); if (err && err != LFS_ERR_CORRUPT) { return err; } // perturb valid bit? - dir->etag |= 0x80000000 & ~lfs_frombe32(etag); + dir->etag |= (0x80 & ~eperturb) << 24; - // crc the rest the full prog_size, including etag in case - // the actual esize is < tag size (though this shouldn't - // happen normally) - uint32_t tcrc = 0xffffffff; + // 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, estate.size, &tcrc); + dir->pair[0], dir->off, fcrc.size, &ecrc); if (err && err != LFS_ERR_CORRUPT) { return err; } - if (tcrc == estate.crc) { + // found beginning of erased part? + if (ecrc == fcrc.crc) { dir->erased = true; break; } - } else if (lfs_tag_chunk(tag) < 2) { - // for backwards compatibility we fall through here on - // unrecognized tags, leaving it up to the CRC to reject - // bad commits - } else { - // end of block commit - dir->erased = false; - break; } // reset crc crc = 0xffffffff; - hasestate = false; - } else { - // crc the entry first, hopefully leaving it in the cache - err = lfs_bd_crc(lfs, + hasfcrc = false; + 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; + } + 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, NULL, &lfs->rcache, lfs->cfg->block_size, - dir->pair[0], off+sizeof(tag), - lfs_tag_dsize(tag)-sizeof(tag), &crc); + dir->pair[0], off+sizeof(tag), &temptail, 8); if (err) { if (err == LFS_ERR_CORRUPT) { dir->erased = false; @@ -1206,78 +1233,47 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, } 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; + lfs_pair_fromle32(temptail); + } else if (lfs_tag_type3(tag) == LFS_TYPE_FCRC) { + err = lfs_bd_read(lfs, + NULL, &lfs->rcache, lfs->cfg->block_size, + dir->pair[0], off+sizeof(tag), + &fcrc, sizeof(fcrc)); + if (err) { + if (err == LFS_ERR_CORRUPT) { + dir->erased = false; + break; } - } 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, sizeof(temptail)); - if (err) { - if (err == LFS_ERR_CORRUPT) { - dir->erased = false; - break; - } - } - lfs_pair_fromle32(temptail); - } else if (lfs_tag_type1(tag) == LFS_TYPE_NPROGCRC) { - err = lfs_bd_read(lfs, - NULL, &lfs->rcache, lfs->cfg->block_size, - dir->pair[0], off+sizeof(tag), - &estate, sizeof(estate)); - if (err) { - if (err == LFS_ERR_CORRUPT) { - dir->erased = false; - break; - } - } - lfs_estate_fromle32(&estate); - hasestate = true; } - // 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; - } + lfs_fcrc_fromle32(&fcrc); + hasfcrc = true; + } - 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; } } } @@ -1584,11 +1580,12 @@ static int lfs_dir_commitattr(lfs_t *lfs, struct lfs_commit *commit, #endif #ifndef LFS_READONLY + static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { // align to program units // // this gets a bit complex as we have two types of crcs: - // - 4-word crc with estate to check following prog (middle of block) + // - 5-word crc with fcrc to check following prog (middle of block) // - 2-word crc with no following prog (end of block) const lfs_off_t end = lfs_alignup( lfs_min(commit->off + 5*sizeof(uint32_t), lfs->cfg->block_size), @@ -1601,75 +1598,77 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { // padding is not crced, which lets fetches skip padding but // makes committing a bit more complicated while (commit->off < end) { - lfs_off_t off = commit->off + sizeof(lfs_tag_t); - lfs_off_t noff = lfs_min(end - off, 0x3fe) + off; + lfs_off_t noff = lfs_min(end - (commit->off+sizeof(lfs_tag_t)), 0x3fe) + + (commit->off+sizeof(lfs_tag_t)); + // too large for crc tag? need padding commits if (noff < end) { - noff = lfs_min(noff, end - 2*sizeof(uint32_t)); + noff = lfs_min(noff, end - 5*sizeof(uint32_t)); } - // clamp erase size to tag size, this gives us the full tag as potential - // to intentionally invalidate erase CRCs - const lfs_size_t esize = lfs_max( - lfs->cfg->prog_size, sizeof(lfs_tag_t)); - lfs_tag_t etag = 0; - // space for estate? also only emit on last commit in padding commits - if (noff <= lfs->cfg->block_size - esize) { - // 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 + // space for fcrc? + uint8_t eperturb = -1; + if (noff < lfs->cfg->block_size) { + // first read the leading byte, this always contains a bit + // we can perturb to avoid writes that don't change the fcrc int err = lfs_bd_read(lfs, - NULL, &lfs->rcache, esize, - commit->block, noff, &etag, sizeof(etag)); + NULL, &lfs->rcache, lfs->cfg->prog_size, + commit->block, noff, &eperturb, 1); if (err && err != LFS_ERR_CORRUPT) { return err; } - // find expected erased state - uint32_t ecrc = 0xffffffff; + // find the expected fcrc, don't bother avoiding a reread + // of the eperturb, it should still be in our cache + struct lfs_fcrc fcrc = { + // 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, - NULL, &lfs->rcache, esize, - commit->block, noff, esize, &ecrc); + NULL, &lfs->rcache, fcrc.size, + commit->block, noff, fcrc.size, &fcrc.crc); if (err && err != LFS_ERR_CORRUPT) { return err; } - struct lfs_estate estate = {.size = esize, .crc = ecrc}; - lfs_estate_tole32(&estate); + lfs_fcrc_tole32(&fcrc); err = lfs_dir_commitattr(lfs, commit, - LFS_MKTAG(LFS_TYPE_NPROGCRC, 0x3ff, sizeof(estate)), - &estate); + LFS_MKTAG(LFS_TYPE_FCRC, 0x3ff, sizeof(struct lfs_fcrc)), + &fcrc); if (err) { return err; } } - // build crc state - off = commit->off + sizeof(lfs_tag_t); + // build commit crc struct { lfs_tag_t tag; uint32_t crc; - } cstate; - lfs_tag_t ctag = LFS_MKTAG(LFS_TYPE_COMMITCRC, 0x3ff, noff-off); - cstate.tag = lfs_tobe32(ctag ^ commit->ptag); - commit->crc = lfs_crc(commit->crc, &cstate.tag, sizeof(cstate.tag)); - cstate.crc = lfs_tole32(commit->crc); + } ccrc; + lfs_tag_t ntag = LFS_MKTAG(LFS_TYPE_CCRC, 0x3ff, + noff - (commit->off+sizeof(lfs_tag_t))); + ccrc.tag = lfs_tobe32(ntag ^ commit->ptag); + commit->crc = lfs_crc(commit->crc, &ccrc.tag, sizeof(lfs_tag_t)); + ccrc.crc = lfs_tole32(commit->crc); int err = lfs_bd_prog(lfs, &lfs->pcache, &lfs->rcache, false, - commit->block, commit->off, &cstate, sizeof(cstate)); + commit->block, commit->off, &ccrc, sizeof(ccrc)); if (err) { return err; } // keep track of non-padding checksum to verify if (off1 == 0) { - off1 = off; + off1 = commit->off + sizeof(lfs_tag_t); crc1 = commit->crc; } commit->off = noff; // perturb valid bit? - commit->ptag = ctag | (0x80000000 & ~lfs_frombe32(etag)); + commit->ptag = ntag | ((0x80 & ~eperturb) << 24); // reset crc for next commit commit->crc = 0xffffffff; } @@ -1693,12 +1692,12 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { return err; } - // check against known crc for non-padding commits + // check non-padding commits against known crc if (crc != crc1) { return LFS_ERR_CORRUPT; } - // make sure to check crc in case we happened to pick + // make sure to check crc in case we happen to pick // up an unrelated crc (frozen block?) err = lfs_bd_crc(lfs, NULL, &lfs->rcache, sizeof(uint32_t), @@ -4556,7 +4555,8 @@ static lfs_stag_t lfs_fs_parent(lfs_t *lfs, const lfs_block_t pair[2], #ifndef LFS_READONLY static int lfs_fs_preporphans(lfs_t *lfs, int8_t orphans) { - LFS_ASSERT(lfs_tag_size(lfs->gstate.tag) > 0 || orphans >= 0); + LFS_ASSERT(lfs_tag_size(lfs->gstate.tag) > 0x000 || orphans >= 0); + LFS_ASSERT(lfs_tag_size(lfs->gstate.tag) < 0x3ff || orphans <= 0); lfs->gstate.tag += orphans; lfs->gstate.tag = ((lfs->gstate.tag & ~LFS_MKTAG(0x800, 0, 0)) | ((uint32_t)lfs_gstate_hasorphans(&lfs->gstate) << 31)); @@ -4587,6 +4587,10 @@ static int lfs_fs_demove(lfs_t *lfs) { lfs->gdisk.pair[1], lfs_tag_id(lfs->gdisk.tag)); + // no other gstate is supported at this time, so if we found something else + // something most likely went wrong in gstate calculation + LFS_ASSERT(lfs_tag_type3(lfs->gdisk.tag) == LFS_TYPE_DELETE); + // fetch and delete the moved entry lfs_mdir_t movedir; int err = lfs_dir_fetch(lfs, &movedir, lfs->gdisk.pair); diff --git a/lfs.h b/lfs.h index c52e62c6..82f69854 100644 --- a/lfs.h +++ b/lfs.h @@ -112,8 +112,8 @@ enum lfs_type { LFS_TYPE_SOFTTAIL = 0x600, LFS_TYPE_HARDTAIL = 0x601, LFS_TYPE_MOVESTATE = 0x7ff, - LFS_TYPE_COMMITCRC = 0x502, - LFS_TYPE_NPROGCRC = 0x5ff, + LFS_TYPE_CCRC = 0x502, + LFS_TYPE_FCRC = 0x5ff, // internal chip sources LFS_FROM_NOOP = 0x000, diff --git a/scripts/readmdir.py b/scripts/readmdir.py index e86b1d97..d4f39c44 100755 --- a/scripts/readmdir.py +++ b/scripts/readmdir.py @@ -23,8 +23,8 @@ TAG_TYPES = { 'hardtail': (0x7ff, 0x601), 'gstate': (0x700, 0x700), 'movestate': (0x7ff, 0x7ff), - 'crc': (0x700, 0x500), - 'nprogcrc': (0x7ff, 0x5ff), + 'crc': (0x780, 0x500), + 'fcrc': (0x7ff, 0x5ff), } class Tag: @@ -126,10 +126,9 @@ class Tag: return ntag def typerepr(self): - if (self.is_('crc') and not self.is_('nprogcrc') and - getattr(self, 'crc', 0xffffffff) != 0xffffffff): + if (self.is_('crc') and getattr(self, 'crc', 0xffffffff) != 0xffffffff): crc_status = ' (bad)' - elif self.is_('nprogcrc') and getattr(self, 'erased', False): + elif self.is_('fcrc') and getattr(self, 'erased', False): crc_status = ' (era)' else: crc_status = '' @@ -203,7 +202,7 @@ class MetadataPair: tag = Tag((int(tag) ^ ntag) & 0x7fffffff) tag.off = off + 4 tag.data = block[off+4:off+tag.dsize] - if tag.is_('crc') and not tag.is_('nprogcrc'): + if tag.is_('crc'): crc = binascii.crc32(block[off:off+2*4], crc) else: crc = binascii.crc32(block[off:off+tag.dsize], crc) @@ -212,7 +211,7 @@ class MetadataPair: self.all_.append(tag) - if tag.is_('nprogcrc') and len(tag.data) == 8: + if tag.is_('fcrc') and len(tag.data) == 8: etag = tag estate = struct.unpack(' Date: Sat, 10 Dec 2022 13:16:52 -0600 Subject: [PATCH 5/6] 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. --- lfs.c | 168 ++++++++++++++++++++------------------------ lfs.h | 2 +- scripts/readmdir.py | 38 +++++----- 3 files changed, 98 insertions(+), 110 deletions(-) diff --git a/lfs.c b/lfs.c index 7e9e53be..12864c79 100644 --- a/lfs.c +++ b/lfs.c @@ -1078,6 +1078,8 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, bool tempsplit = false; lfs_stag_t tempbesttag = besttag; + // assume not erased until proven otherwise + bool maybeerased = false; bool hasfcrc = false; struct lfs_fcrc fcrc; @@ -1095,24 +1097,26 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, if (err) { if (err == LFS_ERR_CORRUPT) { // can't continue? - dir->erased = false; break; } return err; } 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? - if (off + lfs_tag_dsize(tag) > lfs->cfg->block_size) { - dir->erased = false; + } else if (off + lfs_tag_dsize(tag) > lfs->cfg->block_size) { break; } ptag = tag; - if (lfs_tag_type2(tag) == LFS_TYPE_CRC) { + if (lfs_tag_type2(tag) == LFS_TYPE_CCRC) { // check the crc attr uint32_t dcrc; 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)); if (err) { if (err == LFS_ERR_CORRUPT) { - dir->erased = false; break; } return err; @@ -1128,10 +1131,12 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, dcrc = lfs_fromle32(dcrc); if (crc != dcrc) { - dir->erased = false; 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 // pseudorandom numbers, note we use another crc here // 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->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 crc = 0xffffffff; - hasfcrc = false; continue; } + // fcrc is only valid when last tag was a crc + hasfcrc = false; + // crc the entry first, hopefully leaving it in the cache err = lfs_bd_crc(lfs, 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); if (err) { if (err == LFS_ERR_CORRUPT) { - dir->erased = false; break; } return err; @@ -1228,7 +1196,6 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, dir->pair[0], off+sizeof(tag), &temptail, 8); if (err) { if (err == LFS_ERR_CORRUPT) { - dir->erased = false; break; } return err; @@ -1241,7 +1208,6 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, &fcrc, sizeof(fcrc)); if (err) { if (err == LFS_ERR_CORRUPT) { - dir->erased = false; break; } } @@ -1256,7 +1222,6 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, dir->pair[0], off+sizeof(tag)}); if (res < 0) { if (res == LFS_ERR_CORRUPT) { - dir->erased = false; break; } return res; @@ -1278,35 +1243,54 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs, } } - // consider what we have good enough - if (dir->off > 0) { - // synthetic move - if (lfs_gstate_hasmovehere(&lfs->gdisk, dir->pair)) { - if (lfs_tag_id(lfs->gdisk.tag) == lfs_tag_id(besttag)) { - besttag |= 0x80000000; - } else if (besttag != -1 && - lfs_tag_id(lfs->gdisk.tag) < lfs_tag_id(besttag)) { - besttag -= LFS_MKTAG(0, 1, 0); - } + // found no valid commits? + if (dir->off == 0) { + // try the other block? + lfs_pair_swap(dir->pair); + dir->rev = revs[(r+1)%2]; + continue; + } + + // 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? - if (id) { - *id = lfs_min(lfs_tag_id(besttag), dir->count); - } + // found beginning of erased part? + dir->erased = (fcrc_ == fcrc.crc); + } - if (lfs_tag_isvalid(besttag)) { - return besttag; - } else if (lfs_tag_id(besttag) < dir->count) { - return LFS_ERR_NOENT; - } else { - return 0; + // synthetic move + if (lfs_gstate_hasmovehere(&lfs->gdisk, dir->pair)) { + if (lfs_tag_id(lfs->gdisk.tag) == lfs_tag_id(besttag)) { + besttag |= 0x80000000; + } else if (besttag != -1 && + lfs_tag_id(lfs->gdisk.tag) < lfs_tag_id(besttag)) { + besttag -= LFS_MKTAG(0, 1, 0); } } - // failed, try the other block? - lfs_pair_swap(dir->pair); - dir->rev = revs[(r+1)%2]; + // found tag? or found best id? + if (id) { + *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"}", @@ -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 // makes committing a bit more complicated while (commit->off < end) { - lfs_off_t noff = lfs_min(end - (commit->off+sizeof(lfs_tag_t)), 0x3fe) - + (commit->off+sizeof(lfs_tag_t)); + lfs_off_t noff = ( + lfs_min(end - (commit->off+sizeof(lfs_tag_t)), 0x3fe) + + (commit->off+sizeof(lfs_tag_t))); // too large for crc tag? need padding commits if (noff < end) { 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? 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 // we can perturb to avoid writes that don't change the fcrc 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 // of the eperturb, it should still be in our cache - struct lfs_fcrc fcrc = { - // 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, - }; + struct lfs_fcrc fcrc = {.size=lfs->cfg->prog_size, .crc=0xffffffff}; err = lfs_bd_crc(lfs, - NULL, &lfs->rcache, fcrc.size, + NULL, &lfs->rcache, lfs->cfg->prog_size, commit->block, noff, fcrc.size, &fcrc.crc); if (err && err != LFS_ERR_CORRUPT) { return err; @@ -1647,7 +1626,8 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { lfs_tag_t tag; uint32_t crc; } 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))); ccrc.tag = lfs_tobe32(ntag ^ commit->ptag); 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; // perturb valid bit? - commit->ptag = ntag | ((0x80 & ~eperturb) << 24); + commit->ptag = ntag ^ ((0x80 & ~eperturb) << 24); // reset crc for next commit commit->crc = 0xffffffff; - } - // flush buffers - int err = lfs_bd_sync(lfs, &lfs->pcache, &lfs->rcache, false); - if (err) { - return err; + // manually flush here since we don't prog the padding, this confuses + // the caching layer + if (noff >= end || noff >= lfs->pcache.off + lfs->cfg->cache_size) { + // flush buffers + int err = lfs_bd_sync(lfs, &lfs->pcache, &lfs->rcache, false); + if (err) { + return err; + } + } } // 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 lfs_off_t off = commit->begin; uint32_t crc = 0xffffffff; - err = lfs_bd_crc(lfs, + int err = lfs_bd_crc(lfs, NULL, &lfs->rcache, off1+sizeof(uint32_t), commit->block, off, off1-off, &crc); if (err) { diff --git a/lfs.h b/lfs.h index 82f69854..a1de75e8 100644 --- a/lfs.h +++ b/lfs.h @@ -112,7 +112,7 @@ enum lfs_type { LFS_TYPE_SOFTTAIL = 0x600, LFS_TYPE_HARDTAIL = 0x601, LFS_TYPE_MOVESTATE = 0x7ff, - LFS_TYPE_CCRC = 0x502, + LFS_TYPE_CCRC = 0x500, LFS_TYPE_FCRC = 0x5ff, // internal chip sources diff --git a/scripts/readmdir.py b/scripts/readmdir.py index d4f39c44..98816df9 100755 --- a/scripts/readmdir.py +++ b/scripts/readmdir.py @@ -23,7 +23,8 @@ TAG_TYPES = { 'hardtail': (0x7ff, 0x601), 'gstate': (0x700, 0x700), 'movestate': (0x7ff, 0x7ff), - 'crc': (0x780, 0x500), + 'crc': (0x700, 0x500), + 'ccrc': (0x780, 0x500), 'fcrc': (0x7ff, 0x5ff), } @@ -121,12 +122,13 @@ class Tag: ntag = Tag(self.type, nid, self.size) if hasattr(self, 'off'): ntag.off = self.off 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 return ntag 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)' elif self.is_('fcrc') and getattr(self, 'erased', False): crc_status = ' (era)' @@ -187,8 +189,8 @@ class MetadataPair: self.rev, = struct.unpack(' Date: Sat, 10 Dec 2022 22:38:20 -0600 Subject: [PATCH 6/6] Fixed issue where deorphan could get stuck circling between two half-orphans This of course should never happen normally, two half-orphans requires two parents, which is disallowed in littlefs for this reason. But it can happen if there is an outdated half-orphan later in the metadata linked-list. The two half-orphans can cause the deorphan step to get stuck, constantly "fixing" the first half-orphan before it has a chance to remove the problematic, outdated half-orphan later in the list. The solution here is to do a full check for half-orphans before restarting the half-orphan loop. This strategy has the potential to visit more metadata blocks unnecessarily, but avoids situations where removing a later half-orphan will eventually cause an earlier half-orphan to resolve itself. Found with heuristic powerloss testing with test_relocations_reentrant_renames after 192 nested powerlosses. --- lfs.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lfs.c b/lfs.c index 12864c79..7a4b41a8 100644 --- a/lfs.c +++ b/lfs.c @@ -4603,7 +4603,6 @@ static int lfs_fs_deorphan(lfs_t *lfs, bool powerloss) { int8_t found = 0; -restart: // Check for orphans in two separate passes: // - 1 for half-orphans (relocations) // - 2 for full-orphans (removes/renames) @@ -4612,10 +4611,12 @@ restart: // references to full-orphans, effectively hiding them from the deorphan // search. // - for (int pass = 0; pass < 2; pass++) { + int pass = 0; + while (pass < 2) { // Fix any orphans lfs_mdir_t pdir = {.split = true, .tail = {0, 1}}; lfs_mdir_t dir; + bool moreorphans = false; // iterate over all directory directory entries while (!lfs_pair_isnull(pdir.tail)) { @@ -4676,7 +4677,7 @@ restart: // did our commit create more orphans? if (state == LFS_OK_ORPHANED) { - goto restart; + moreorphans = true; } // refetch tail @@ -4712,7 +4713,7 @@ restart: // did our commit create more orphans? if (state == LFS_OK_ORPHANED) { - goto restart; + moreorphans = true; } // refetch tail @@ -4722,6 +4723,8 @@ restart: pdir = dir; } + + pass = moreorphans ? 0 : pass+1; } // mark orphans as fixed