Github/littlefs
1
Fork 0
Code Actions Activity

Fixed a number of issues that made becksums ineffective

Browse Source 
Unfortunately we don't really have a way to prove that block-level
erased-state checksums are working short of benchmarking, so sure
enough block-level erased-state checksums were not working.

Some of the issues were a bit silly:

- Forgot so zero-init the checksum.

- Typo meant we were calculating the becksum of the wrong block.

- Flushing when buffer is non-empty triggered redundant flushes if the
  buffer was filled for reading.

The last one is a bit more fundamental.

Humorously, it wasn't caught earlier because the buffer is always
in-sync with disk. So redundant flushes aren't an _error_, but they sure
hurt performance.

The fix is a little bit tricky. We want to flush only when
LFS_F_UNFLUSHED is set, but this confuses lfsr_file_write into thinking
it can clobber small file buffers.

The solution here is to only flush when LFS_F_UNFLUSHED is set, always
set LFS_F_UNFLUSHED on small files, which makes a bit of sense if you
think about it.

This means LFS_F_UNFLUSHED can be set on read-only and in-sync files,
but that should hopefully not be an issue.
This commit is contained in:
Christopher Haster
2023-12-17 23:49:23 -06:00
parent 8eea06286f
commit f116823aa4
1 changed files with 61 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+61 -51
View File
@@ -3051,8 +3051,8 @@ static int lfsr_rbyd_appendcksum(lfs_t *lfs, lfsr_rbyd_t *rbyd) {
// perturb byte, as it should still be in our cache // perturb byte, as it should still be in our cache
lfsr_ecksum_t ecksum = {.size=lfs->cfg->prog_size}; lfsr_ecksum_t ecksum = {.size=lfs->cfg->prog_size};
err = lfsr_bd_cksum(lfs, err = lfsr_bd_cksum(lfs,
rbyd->blocks[0], aligned_eoff, lfs->cfg->prog_size, rbyd->blocks[0], aligned_eoff, ecksum.size,
lfs->cfg->prog_size, ecksum.size,
&ecksum.cksum); &ecksum.cksum);
if (err && err != LFS_ERR_CORRUPT) { if (err && err != LFS_ERR_CORRUPT) {
return err; return err;
@@ -9319,6 +9319,8 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file,
goto failed_with_buffer; goto failed_with_buffer;
} }
// small files remain perpetually unflushed
file->flags |= LFS_F_UNFLUSHED;
file->buffer_pos = 0; file->buffer_pos = 0;
file->buffer_size = file->size; file->buffer_size = file->size;
file->ftree = LFSR_FTREE_NULL(); file->ftree = LFSR_FTREE_NULL();
@@ -10170,9 +10172,10 @@ static int lfsr_ftree_flush(lfs_t *lfs,
lfsr_ecksum_t becksum = {.size=-1}; lfsr_ecksum_t becksum = {.size=-1};
if (bptr.cksize < lfs->cfg->block_size) { if (bptr.cksize < lfs->cfg->block_size) {
becksum.size = lfs->cfg->prog_size; becksum.size = lfs->cfg->prog_size;
err = lfsr_bd_cksum(lfs, bptr.data.u.disk.off, becksum.cksum = 0;
bptr.cksize, lfs->cfg->prog_size, err = lfsr_bd_cksum(lfs,
lfs->cfg->prog_size, bptr.data.u.disk.block, bptr.cksize, becksum.size,
becksum.size,
&becksum.cksum); &becksum.cksum);
if (err && err != LFS_ERR_CORRUPT) { if (err && err != LFS_ERR_CORRUPT) {
return err; return err;
@@ -10366,7 +10369,7 @@ lfs_ssize_t lfsr_file_read(lfs_t *lfs, lfsr_file_t *file,
// note that flush does not change the actual file data, so if // note that flush does not change the actual file data, so if
// a read fails it's ok to fall back to our flushed state // a read fails it's ok to fall back to our flushed state
// //
if (file->buffer_size != 0) { if (lfsr_f_isunflushed(file->flags)) {
int err = lfsr_file_flush(lfs, file); int err = lfsr_file_flush(lfs, file);
if (err) { if (err) {
return err; return err;
@@ -10421,6 +10424,7 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file,
// copy state so we can recover from errors // copy state so we can recover from errors
lfs_off_t pos_ = file->pos; lfs_off_t pos_ = file->pos;
bool unflushed_ = lfsr_f_isunflushed(file->flags);
lfs_off_t buffer_pos_ = file->buffer_pos; lfs_off_t buffer_pos_ = file->buffer_pos;
lfs_size_t buffer_size_ = file->buffer_size; lfs_size_t buffer_size_ = file->buffer_size;
lfsr_ftree_t ftree_ = file->ftree; lfsr_ftree_t ftree_ = file->ftree;
@@ -10439,6 +10443,7 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file,
&& pos_ <= lfs->cfg->cache_size && pos_ <= lfs->cfg->cache_size
&& pos_ <= lfs->cfg->inline_size && pos_ <= lfs->cfg->inline_size
&& pos_ <= lfs->cfg->fragment_size) { && pos_ <= lfs->cfg->fragment_size) {
LFS_ASSERT(unflushed_);
LFS_ASSERT(file->size == buffer_size_); LFS_ASSERT(file->size == buffer_size_);
memset(&file->buffer[buffer_size_], memset(&file->buffer[buffer_size_],
0, 0,
@@ -10454,7 +10459,7 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file,
// strictly necessary, but enforces a more intuitive write order // strictly necessary, but enforces a more intuitive write order
// and avoids weird cases with low-level write heuristics // and avoids weird cases with low-level write heuristics
// //
if (buffer_size_ == 0 && size >= lfs->cfg->cache_size) { if (!unflushed_ && size >= lfs->cfg->cache_size) {
err = lfsr_ftree_flush(lfs, &file->mdir, &ftree_, err = lfsr_ftree_flush(lfs, &file->mdir, &ftree_,
pos_, buffer_, size); pos_, buffer_, size);
if (err) { if (err) {
@@ -10477,9 +10482,16 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file,
// 2. Bypassing the buffer above means we only write to the // 2. Bypassing the buffer above means we only write to the
// buffer once, and flush at most twice. // buffer once, and flush at most twice.
// //
if (pos_ >= buffer_pos_ if (!unflushed_
&& pos_ <= buffer_pos_ + buffer_size_ || (pos_ >= buffer_pos_
&& pos_ < buffer_pos_ + lfs->cfg->cache_size) { && pos_ <= buffer_pos_ + buffer_size_
&& pos_ < buffer_pos_ + lfs->cfg->cache_size)) {
// unused buffer? we can move it where we need it
if (!unflushed_) {
buffer_pos_ = pos_;
buffer_size_ = 0;
}
lfs_size_t d = lfs_min32( lfs_size_t d = lfs_min32(
size, size,
lfs->cfg->cache_size - (pos_ - buffer_pos_)); lfs->cfg->cache_size - (pos_ - buffer_pos_));
@@ -10488,6 +10500,7 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file,
buffer_size_, buffer_size_,
pos_+d - buffer_pos_); pos_+d - buffer_pos_);
unflushed_ = true;
pos_ += d; pos_ += d;
buffer_ += d; buffer_ += d;
size -= d; size -= d;
@@ -10500,14 +10513,22 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file,
if (err) { if (err) {
goto failed; goto failed;
} }
buffer_pos_ = pos_; unflushed_ = false;
buffer_pos_ = 0;
buffer_size_ = 0; buffer_size_ = 0;
} }
// mark as unflushed and unsynced, update file, and return amount written // mark as unflushed and unsynced, update file, and return amount written
lfs_size_t written = pos_ - ( lfs_size_t written;
(lfsr_o_isappend(file->flags)) ? file->size : file->pos); if (lfsr_o_isappend(file->flags)) {
file->flags |= LFS_F_UNSYNCED | LFS_F_UNFLUSHED; written = pos_ - file->size;
} else {
written = pos_ - file->pos;
}
file->flags |= LFS_F_UNSYNCED;
if (unflushed_) {
file->flags |= LFS_F_UNFLUSHED;
}
file->pos = pos_; file->pos = pos_;
file->size = lfs_max32(file->size, pos_); file->size = lfs_max32(file->size, pos_);
file->buffer_pos = buffer_pos_; file->buffer_pos = buffer_pos_;
@@ -10524,7 +10545,10 @@ failed:;
static int lfsr_file_flush(lfs_t *lfs, lfsr_file_t *file) { static int lfsr_file_flush(lfs_t *lfs, lfsr_file_t *file) {
// do nothing if our file is readonly // do nothing if our file is readonly
if (!lfsr_o_iswriteable(file->flags)) { if (!lfsr_o_iswriteable(file->flags)) {
LFS_ASSERT(!lfsr_f_isunflushed(file->flags)); LFS_ASSERT(!lfsr_f_isunflushed(file->flags)
|| (file->size <= lfs->cfg->cache_size
&& file->size <= lfs->cfg->inline_size
&& file->size <= lfs->cfg->fragment_size));
return 0; return 0;
} }
@@ -10533,34 +10557,33 @@ static int lfsr_file_flush(lfs_t *lfs, lfsr_file_t *file) {
return 0; return 0;
} }
int err; // do nothing if our file is small
// if our file is small don't do anything //
// note this means small files remain perpetually unflushed
if (file->size <= lfs->cfg->cache_size if (file->size <= lfs->cfg->cache_size
&& file->size <= lfs->cfg->inline_size && file->size <= lfs->cfg->inline_size
&& file->size <= lfs->cfg->fragment_size) { && file->size <= lfs->cfg->fragment_size) {
// our file must reside entirely in our buffer // our file must reside entirely in our buffer
LFS_ASSERT(file->buffer_pos == 0); LFS_ASSERT(file->buffer_pos == 0);
LFS_ASSERT(file->buffer_size == file->size); LFS_ASSERT(file->buffer_size == file->size);
return 0;
}
} else { // checkpoint the allocator
// flush our buffer if it contains any unwritten data lfs_alloc_ckpoint(lfs);
if (lfsr_f_isunflushed(file->flags)) {
// checkpoint the allocator
lfs_alloc_ckpoint(lfs);
// copy state so we can recover from errors int err;
lfsr_ftree_t ftree_ = file->ftree; // flush our buffer if it contains any unwritten data
if (lfsr_f_isunflushed(file->flags) && file->buffer_size != 0) {
// flush // copy state so we can recover from errors
err = lfsr_ftree_flush(lfs, &file->mdir, &ftree_, lfsr_ftree_t ftree_ = file->ftree;
file->buffer_pos, file->buffer, file->buffer_size); // flush
if (err) { err = lfsr_ftree_flush(lfs, &file->mdir, &ftree_,
goto failed; file->buffer_pos, file->buffer, file->buffer_size);
} if (err) {
goto failed;
// update file
file->ftree = ftree_;
} }
file->ftree = ftree_;
} }
file->flags &= ~LFS_F_UNFLUSHED; file->flags &= ~LFS_F_UNFLUSHED;
@@ -10584,7 +10607,6 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) {
// do nothing if our file is readonly // do nothing if our file is readonly
if (!lfsr_o_iswriteable(file->flags)) { if (!lfsr_o_iswriteable(file->flags)) {
LFS_ASSERT(!lfsr_f_isunflushed(file->flags));
LFS_ASSERT(!lfsr_f_isunsynced(file->flags)); LFS_ASSERT(!lfsr_f_isunsynced(file->flags));
return 0; return 0;
} }
@@ -10750,6 +10772,8 @@ int lfsr_file_truncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size) {
size - file->buffer_size); size - file->buffer_size);
} }
// small files remain perpetually unflushed
file->flags |= LFS_F_UNFLUSHED;
file->buffer_pos = 0; file->buffer_pos = 0;
file->buffer_size = size; file->buffer_size = size;
file->ftree = LFSR_FTREE_NULL(); file->ftree = LFSR_FTREE_NULL();
@@ -10774,14 +10798,6 @@ int lfsr_file_truncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size) {
file->buffer_size = lfs_min32( file->buffer_size = lfs_min32(
file->buffer_size, file->buffer_size,
size - lfs_min32(file->buffer_pos, size)); size - lfs_min32(file->buffer_pos, size));
// our file became not small with data in buffer, mark as unflushed
if (file->size <= lfs->cfg->cache_size
&& file->size <= lfs->cfg->inline_size
&& file->size <= lfs->cfg->fragment_size
&& file->buffer_size != 0) {
file->flags |= LFS_F_UNFLUSHED;
}
} }
// mark as unsynced and update our size // mark as unsynced and update our size
@@ -10858,6 +10874,8 @@ int lfsr_file_fruncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size) {
size - file->buffer_size); size - file->buffer_size);
} }
// small files remain perpetually unflushed
file->flags |= LFS_F_UNFLUSHED;
file->buffer_pos = 0; file->buffer_pos = 0;
file->buffer_size = size; file->buffer_size = size;
file->ftree = LFSR_FTREE_NULL(); file->ftree = LFSR_FTREE_NULL();
@@ -10889,14 +10907,6 @@ int lfsr_file_fruncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size) {
lfs_smax32(file->size - size - file->buffer_pos, 0), lfs_smax32(file->size - size - file->buffer_pos, 0),
file->buffer_size); file->buffer_size);
file->buffer_pos -= lfs_smin32(file->size - size, file->buffer_pos); file->buffer_pos -= lfs_smin32(file->size - size, file->buffer_pos);
// our file became not small with data in buffer, mark as unflushed
if (file->size <= lfs->cfg->cache_size
&& file->size <= lfs->cfg->inline_size
&& file->size <= lfs->cfg->fragment_size
&& file->buffer_size != 0) {
file->flags |= LFS_F_UNFLUSHED;
}
} }
// mark as unsynced and update our size // mark as unsynced and update our size