Skip to content

Commit

Permalink
Tweaked tag endianness to catch power-loss after <1 word is written
Browse files Browse the repository at this point in the history
There was an interesting subtlety with the existing layout of tags that
could become a problem in the future. Basically, littlefs avoids writing to
any region of storage it is not absolutely sure has been erased
beforehand. This is a part of limiting the number of assumptions about
storage. It's possible a storage technology can't support writes without
erases in a way that is undetectable at write time (Maybe changing a bit
without an erase decreases the longevity of the information stored on
the bit).

But the existing layout had a very tiny corner case where this wasn't
true. Consider the location of the valid bit in the tag struct:

[1|---  31  ---]
 ^--- valid bit

The responsibility of this bit is to indicate if an attempt has been
made to write the following commit. If it is not set (the specific value
is dependent on a previous read and identified by the preceeding commit),
the assumption is that it is safe to write to the next region because it
has been erased previously. If it is set, we check if the next commit is
valid, if it isn't (because of CRC failure, likely due to power-loss), we
discard the commit. But because an attempt has been made to write to
that storage, we must then do a compaction to move to the other block in
the metadata-pair.

This plan looks good on paper, but what does it look like on storage?
The problem is that words in littlefs are in little-endian. So on
storage the tag actually looks like this:

[- 8 -|- 8 -|- 8 -|1|- 7 -]
                   ^-- valid bit

This means that we don't actually set the valid bit before writing the
tag! We write the lower bytes first. If we lose power, we may have
written 3 bytes without this fact being detectable.

We could restructure the tag structure to store the valid bit lower,
however because none of the fields are 7 bits, this would make the
extraction more costly, and we then lose the ability to check this
valid bit with a sign comparison.

The simple solution is to just store the tag in big-endian. A small
benefit is that this will actually have a negative code cost on
big-endian machines.

This mixture of endiannesses is frustrating, however it is a pragmatic
solution with only a 20-byte code size cost.
  • Loading branch information
geky committed Oct 22, 2018
1 parent 4a1b8ae commit 5b26c68
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 7 deletions.
10 changes: 5 additions & 5 deletions lfs.c
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ static int lfs_dir_traverse(lfs_t *lfs,
return err;
}

ntag = lfs_fromle32(ntag) ^ tag;
ntag = lfs_frombe32(ntag) ^ tag;
tag |= 0x80000000;
}

Expand Down Expand Up @@ -617,7 +617,7 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
}

crc = lfs_crc(crc, &tag, sizeof(tag));
tag = lfs_fromle32(tag) ^ ptag;
tag = lfs_frombe32(tag) ^ ptag;

// next commit not yet programmed
if (!lfs_tag_isvalid(tag)) {
Expand Down Expand Up @@ -1134,7 +1134,7 @@ static int lfs_commit_attr(lfs_t *lfs, struct lfs_commit *commit,
}

// write out tag
lfs_tag_t ntag = lfs_tole32((tag & 0x7fffffff) ^ commit->ptag);
lfs_tag_t ntag = lfs_tobe32((tag & 0x7fffffff) ^ commit->ptag);
int err = lfs_commit_prog(lfs, commit, &ntag, sizeof(ntag));
if (err) {
return err;
Expand Down Expand Up @@ -1193,13 +1193,13 @@ static int lfs_commit_crc(lfs_t *lfs, struct lfs_commit *commit,
}

// build crc tag
bool reset = ~lfs_fromle32(tag) >> 31;
bool reset = ~lfs_frombe32(tag) >> 31;
tag = LFS_MKTAG(LFS_TYPE_CRC + 2*compacting + reset,
0x1ff, off - (commit->off+sizeof(lfs_tag_t)));

// write out crc
uint32_t footer[2];
footer[0] = lfs_tole32(tag ^ commit->ptag);
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,
Expand Down
46 changes: 46 additions & 0 deletions lfs_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,52 @@ static inline uint16_t lfs_tole16(uint16_t a) {
return lfs_fromle16(a);
}

// Convert between 32-bit big-endian and native order
static inline uint32_t lfs_frombe32(uint32_t a) {
#if !defined(LFS_NO_INTRINSICS) && ( \
(defined( BYTE_ORDER ) && BYTE_ORDER == ORDER_LITTLE_ENDIAN ) || \
(defined(__BYTE_ORDER ) && __BYTE_ORDER == __ORDER_LITTLE_ENDIAN ) || \
(defined(__BYTE_ORDER__) && __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__))
return __builtin_bswap32(a);
#elif !defined(LFS_NO_INTRINSICS) && ( \
(defined( BYTE_ORDER ) && BYTE_ORDER == ORDER_BIG_ENDIAN ) || \
(defined(__BYTE_ORDER ) && __BYTE_ORDER == __ORDER_BIG_ENDIAN ) || \
(defined(__BYTE_ORDER__) && __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__))
return a;
#else
return (((uint8_t*)&a)[0] << 24) |
(((uint8_t*)&a)[1] << 16) |
(((uint8_t*)&a)[2] << 8) |
(((uint8_t*)&a)[3] << 0);
#endif
}

static inline uint32_t lfs_tobe32(uint32_t a) {
return lfs_frombe32(a);
}

// Convert between 16-bit big-endian and native order
static inline uint16_t lfs_frombe16(uint16_t a) {
#if !defined(LFS_NO_INTRINSICS) && ( \
(defined( BYTE_ORDER ) && BYTE_ORDER == ORDER_LITTLE_ENDIAN ) || \
(defined(__BYTE_ORDER ) && __BYTE_ORDER == __ORDER_LITTLE_ENDIAN ) || \
(defined(__BYTE_ORDER__) && __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__))
return __builtin_bswap16(a);
#elif !defined(LFS_NO_INTRINSICS) && ( \
(defined( BYTE_ORDER ) && BYTE_ORDER == ORDER_BIG_ENDIAN ) || \
(defined(__BYTE_ORDER ) && __BYTE_ORDER == __ORDER_BIG_ENDIAN ) || \
(defined(__BYTE_ORDER__) && __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__))
return a;
#else
return (((uint8_t*)&a)[0] << 8) |
(((uint8_t*)&a)[1] << 0);
#endif
}

static inline uint16_t lfs_tobe16(uint16_t a) {
return lfs_frombe16(a);
}

// Calculate CRC-32 with polynomial = 0x04c11db7
uint32_t lfs_crc(uint32_t crc, const void *buffer, size_t size);

Expand Down
2 changes: 1 addition & 1 deletion tests/corrupt.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def corrupt(block):
tag = 0xffffffff
while True:
try:
ntag, = struct.unpack('<I', file.read(4))
ntag, = struct.unpack('>I', file.read(4))
except struct.error:
break

Expand Down
2 changes: 1 addition & 1 deletion tests/debug.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def main(*blocks):
try:
data = file.read(4)
crc = binascii.crc32(data, crc)
ntag, = struct.unpack('<I', data)
ntag, = struct.unpack('>I', data)
except struct.error:
break

Expand Down

0 comments on commit 5b26c68

Please sign in to comment.