Github/littlefs
1
Fork 0
Code Actions Activity

Abandoned data-backed cache, use indirect lfs3_data_t on stack

Browse Source 
This abandons the data-backed cache idea due to concerns around
readability and maintainability. Mixing const/mutable buffers in
lfs3_data_t was not great.

Instead, we now just allocate an indirect lfs3_data_t on the stack in
lfs3_file_sync_ to avoid the previous undefined behavior.

This actually results in less stack usage total, due to lfs3_file_t
allocations in lfs3_set/read, and avoid the more long-term memory cost
in lfs3_file_t:

              code          stack          ctx
  before:    36832           2376          684
  after:     36840 (+0.0%)   2368 (-0.3%)  684 (+0.0%)

Oh. And lfs3_file_sync_ isn't even on the stack hot-path, so this is a
net benefit over the previous cache -> data cast:

              code          stack          ctx
  before sa: 36844           2368          684
  after sa:  36840 (-0.0%)   2368 (+0.0%)  684 (+0.0%)

Still less cool though.
This commit is contained in:
Christopher Haster
2025-07-22 13:39:43 -05:00
parent 5035aa566b
commit 238dbc705d
2 changed files with 60 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs3.c
+56 -52
View File
@@ -11586,7 +11586,7 @@ static inline void lfs3_file_discardcache(lfs3_file_t *file) {
#ifndef LFS3_KVONLY #ifndef LFS3_KVONLY
file->cache.pos = 0; file->cache.pos = 0;
#endif #endif
file->cache.d.size = 0; file->cache.size = 0;
} }
#ifndef LFS3_KVONLY #ifndef LFS3_KVONLY
@@ -11611,7 +11611,7 @@ static inline lfs3_size_t lfs3_file_cachesize(lfs3_t *lfs3,
static inline lfs3_off_t lfs3_file_size_(const lfs3_file_t *file) { static inline lfs3_off_t lfs3_file_size_(const lfs3_file_t *file) {
return lfs3_max( return lfs3_max(
LFS3_IFDEF_KVONLY(0, file->cache.pos) + file->cache.d.size, LFS3_IFDEF_KVONLY(0, file->cache.pos) + file->cache.size,
file->b.shrub.r.weight); file->b.shrub.r.weight);
} }
@@ -11729,17 +11729,17 @@ int lfs3_file_opencfg_(lfs3_t *lfs3, lfs3_file_t *file,
// the file cache, so make sure not to clobber it // the file cache, so make sure not to clobber it
if (lfs3_o_iswrset(file->b.h.flags)) { if (lfs3_o_iswrset(file->b.h.flags)) {
file->b.h.flags |= LFS3_o_UNFLUSH; file->b.h.flags |= LFS3_o_UNFLUSH;
file->cache.d.u.buffer_ = file->cfg->cache_buffer; file->cache.buffer = file->cfg->cache_buffer;
#ifndef LFS3_KVONLY #ifndef LFS3_KVONLY
file->cache.pos = 0; file->cache.pos = 0;
#endif #endif
file->cache.d.size = file->cfg->cache_size; file->cache.size = file->cfg->cache_size;
} else if (file->cfg->cache_buffer) { } else if (file->cfg->cache_buffer) {
file->cache.d.u.buffer_ = file->cfg->cache_buffer; file->cache.buffer = file->cfg->cache_buffer;
} else { } else {
#ifndef LFS3_KVONLY #ifndef LFS3_KVONLY
file->cache.d.u.buffer_ = lfs3_malloc(lfs3_file_cachesize(lfs3, file)); file->cache.buffer = lfs3_malloc(lfs3_file_cachesize(lfs3, file));
if (!file->cache.d.u.buffer_) { if (!file->cache.buffer) {
return LFS3_ERR_NOMEM; return LFS3_ERR_NOMEM;
} }
#else #else
@@ -11821,9 +11821,9 @@ int lfs3_file_opencfg_(lfs3_t *lfs3, lfs3_file_t *file,
// small file wrset? can we atomically commit everything in one // small file wrset? can we atomically commit everything in one
// commit? currently this is only possible via lfs3_set // commit? currently this is only possible via lfs3_set
if (lfs3_o_iswrset(file->b.h.flags) if (lfs3_o_iswrset(file->b.h.flags)
&& file->cache.d.size <= lfs3->cfg->inline_size && file->cache.size <= lfs3->cfg->inline_size
&& file->cache.d.size <= lfs3->cfg->fragment_size && file->cache.size <= lfs3->cfg->fragment_size
&& file->cache.d.size < lfs3->cfg->crystal_thresh) { && file->cache.size < lfs3->cfg->crystal_thresh) {
// we need to mark as unsync for sync to do anything // we need to mark as unsync for sync to do anything
file->b.h.flags |= LFS3_o_UNSYNC; file->b.h.flags |= LFS3_o_UNSYNC;
@@ -11944,7 +11944,7 @@ static void lfs3_file_close_(lfs3_t *lfs3, const lfs3_file_t *file) {
(void)lfs3; (void)lfs3;
// clean up memory // clean up memory
if (!file->cfg->cache_buffer) { if (!file->cfg->cache_buffer) {
lfs3_free(file->cache.d.u.buffer_); lfs3_free(file->cache.buffer);
} }
// are we orphaning a file? // are we orphaning a file?
@@ -12165,14 +12165,14 @@ lfs3_ssize_t lfs3_file_read(lfs3_t *lfs3, lfs3_file_t *file,
lfs3_ssize_t d = lfs3_min(size, lfs3_file_size_(file) - pos_); lfs3_ssize_t d = lfs3_min(size, lfs3_file_size_(file) - pos_);
// any data in our cache? // any data in our cache?
if (pos_ < file->cache.pos + file->cache.d.size if (pos_ < file->cache.pos + file->cache.size
&& file->cache.d.size != 0) { && file->cache.size != 0) {
if (pos_ >= file->cache.pos) { if (pos_ >= file->cache.pos) {
lfs3_ssize_t d_ = lfs3_min( lfs3_ssize_t d_ = lfs3_min(
d, d,
file->cache.d.size - (pos_ - file->cache.pos)); file->cache.size - (pos_ - file->cache.pos));
lfs3_memcpy(buffer_, lfs3_memcpy(buffer_,
&file->cache.d.u.buffer_[pos_ - file->cache.pos], &file->cache.buffer[pos_ - file->cache.pos],
d_); d_);
pos_ += d_; pos_ += d_;
@@ -12207,13 +12207,13 @@ lfs3_ssize_t lfs3_file_read(lfs3_t *lfs3, lfs3_file_t *file,
// try to fill our cache with some data // try to fill our cache with some data
if (!lfs3_o_isunflush(file->b.h.flags)) { if (!lfs3_o_isunflush(file->b.h.flags)) {
lfs3_ssize_t d_ = lfs3_file_readnext(lfs3, file, lfs3_ssize_t d_ = lfs3_file_readnext(lfs3, file,
pos_, file->cache.d.u.buffer_, d); pos_, file->cache.buffer, d);
if (d_ < 0) { if (d_ < 0) {
LFS3_ASSERT(d != LFS3_ERR_NOENT); LFS3_ASSERT(d != LFS3_ERR_NOENT);
return d_; return d_;
} }
file->cache.pos = pos_; file->cache.pos = pos_;
file->cache.d.size = d_; file->cache.size = d_;
continue; continue;
} }
} }
@@ -13338,10 +13338,10 @@ lfs3_ssize_t lfs3_file_write(lfs3_t *lfs3, lfs3_file_t *file,
// note we need to clear the cache anyways to avoid any // note we need to clear the cache anyways to avoid any
// out-of-date data // out-of-date data
file->cache.pos = pos + size - lfs3_file_cachesize(lfs3, file); file->cache.pos = pos + size - lfs3_file_cachesize(lfs3, file);
lfs3_memcpy(file->cache.d.u.buffer_, lfs3_memcpy(file->cache.buffer,
&buffer_[size - lfs3_file_cachesize(lfs3, file)], &buffer_[size - lfs3_file_cachesize(lfs3, file)],
lfs3_file_cachesize(lfs3, file)); lfs3_file_cachesize(lfs3, file));
file->cache.d.size = lfs3_file_cachesize(lfs3, file); file->cache.size = lfs3_file_cachesize(lfs3, file);
file->b.h.flags &= ~LFS3_o_UNFLUSH; file->b.h.flags &= ~LFS3_o_UNFLUSH;
written += size; written += size;
@@ -13363,25 +13363,25 @@ lfs3_ssize_t lfs3_file_write(lfs3_t *lfs3, lfs3_file_t *file,
// //
if (!lfs3_o_isunflush(file->b.h.flags) if (!lfs3_o_isunflush(file->b.h.flags)
|| (pos >= file->cache.pos || (pos >= file->cache.pos
&& pos <= file->cache.pos + file->cache.d.size && pos <= file->cache.pos + file->cache.size
&& pos && pos
< file->cache.pos < file->cache.pos
+ lfs3_file_cachesize(lfs3, file))) { + lfs3_file_cachesize(lfs3, file))) {
// unused cache? we can move it where we need it // unused cache? we can move it where we need it
if (!lfs3_o_isunflush(file->b.h.flags)) { if (!lfs3_o_isunflush(file->b.h.flags)) {
file->cache.pos = pos; file->cache.pos = pos;
file->cache.d.size = 0; file->cache.size = 0;
} }
lfs3_size_t d = lfs3_min( lfs3_size_t d = lfs3_min(
size, size,
lfs3_file_cachesize(lfs3, file) lfs3_file_cachesize(lfs3, file)
- (pos - file->cache.pos)); - (pos - file->cache.pos));
lfs3_memcpy(&file->cache.d.u.buffer_[pos - file->cache.pos], lfs3_memcpy(&file->cache.buffer[pos - file->cache.pos],
buffer_, buffer_,
d); d);
file->cache.d.size = lfs3_max( file->cache.size = lfs3_max(
file->cache.d.size, file->cache.size,
pos+d - file->cache.pos); pos+d - file->cache.pos);
file->b.h.flags |= LFS3_o_UNFLUSH; file->b.h.flags |= LFS3_o_UNFLUSH;
@@ -13394,7 +13394,7 @@ lfs3_ssize_t lfs3_file_write(lfs3_t *lfs3, lfs3_file_t *file,
// flush our cache so the above can't fail // flush our cache so the above can't fail
err = lfs3_file_flush_(lfs3, file, err = lfs3_file_flush_(lfs3, file,
file->cache.pos, file->cache.d.u.buffer_, file->cache.d.size); file->cache.pos, file->cache.buffer, file->cache.size);
if (err) { if (err) {
goto failed; goto failed;
} }
@@ -13458,13 +13458,13 @@ int lfs3_file_flush(lfs3_t *lfs3, lfs3_file_t *file) {
if (lfs3_o_isunflush(file->b.h.flags)) { if (lfs3_o_isunflush(file->b.h.flags)) {
#ifdef LFS3_KVONLY #ifdef LFS3_KVONLY
err = lfs3_file_flushset_(lfs3, file, err = lfs3_file_flushset_(lfs3, file,
file->cache.d.u.buffer_, file->cache.d.size); file->cache.buffer, file->cache.size);
if (err) { if (err) {
goto failed; goto failed;
} }
#else #else
err = lfs3_file_flush_(lfs3, file, err = lfs3_file_flush_(lfs3, file,
file->cache.pos, file->cache.d.u.buffer_, file->cache.d.size); file->cache.pos, file->cache.buffer, file->cache.size);
if (err) { if (err) {
goto failed; goto failed;
} }
@@ -13504,6 +13504,7 @@ static int lfs3_file_sync_(lfs3_t *lfs3, lfs3_file_t *file,
lfs3_data_t name_data; lfs3_data_t name_data;
lfs3_rattr_t shrub_rattrs[1]; lfs3_rattr_t shrub_rattrs[1];
lfs3_size_t shrub_rattr_count = 0; lfs3_size_t shrub_rattr_count = 0;
lfs3_data_t file_data;
lfs3_shrubcommit_t shrub_commit; lfs3_shrubcommit_t shrub_commit;
// uncreated files must be unsync // uncreated files must be unsync
@@ -13547,7 +13548,7 @@ static int lfs3_file_sync_(lfs3_t *lfs3, lfs3_file_t *file,
#ifndef LFS3_KVONLY #ifndef LFS3_KVONLY
LFS3_ASSERT(file->cache.pos == 0); LFS3_ASSERT(file->cache.pos == 0);
#endif #endif
LFS3_ASSERT(file->cache.d.size == lfs3_file_size_(file)); LFS3_ASSERT(file->cache.size == lfs3_file_size_(file));
// discard any lingering bshrub state // discard any lingering bshrub state
#ifndef LFS3_KVONLY #ifndef LFS3_KVONLY
@@ -13556,10 +13557,13 @@ static int lfs3_file_sync_(lfs3_t *lfs3, lfs3_file_t *file,
lfs3_file_discardbshrub(file); lfs3_file_discardbshrub(file);
// build a small shrub commit // build a small shrub commit
if (file->cache.d.size > 0) { if (file->cache.size > 0) {
file_data = LFS3_DATA_BUF(
file->cache.buffer,
file->cache.size);
shrub_rattrs[shrub_rattr_count++] = LFS3_RATTR_DATA( shrub_rattrs[shrub_rattr_count++] = LFS3_RATTR_DATA(
LFS3_TAG_DATA, +file->cache.d.size, LFS3_TAG_DATA, +file->cache.size,
&file->cache.d); &file_data);
LFS3_ASSERT(shrub_rattr_count LFS3_ASSERT(shrub_rattr_count
<= sizeof(shrub_rattrs)/sizeof(lfs3_rattr_t)); <= sizeof(shrub_rattrs)/sizeof(lfs3_rattr_t));
@@ -13695,14 +13699,14 @@ static int lfs3_file_sync_(lfs3_t *lfs3, lfs3_file_t *file,
// //
// note we need to be careful if caches have different // note we need to be careful if caches have different
// sizes, prefer the most recent data in this case // sizes, prefer the most recent data in this case
lfs3_size_t d = file->cache.d.size - lfs3_min( lfs3_size_t d = file->cache.size - lfs3_min(
lfs3_file_cachesize(lfs3, file_), lfs3_file_cachesize(lfs3, file_),
file->cache.d.size); file->cache.size);
file_->cache.pos = file->cache.pos + d; file_->cache.pos = file->cache.pos + d;
lfs3_memcpy(file_->cache.d.u.buffer_, lfs3_memcpy(file_->cache.buffer,
file->cache.d.u.buffer_ + d, file->cache.buffer + d,
file->cache.d.size - d); file->cache.size - d);
file_->cache.d.size = file->cache.d.size - d; file_->cache.size = file->cache.size - d;
// update any custom attrs // update any custom attrs
for (lfs3_size_t i = 0; i < file->cfg->attr_count; i++) { for (lfs3_size_t i = 0; i < file->cfg->attr_count; i++) {
@@ -13778,10 +13782,10 @@ int lfs3_file_sync(lfs3_t *lfs3, lfs3_file_t *file) {
// though don't flush quite yet if our file is small and can be // though don't flush quite yet if our file is small and can be
// combined with sync in a single commit // combined with sync in a single commit
int err; int err;
if (!(file->cache.d.size == lfs3_file_size_(file) if (!(file->cache.size == lfs3_file_size_(file)
&& file->cache.d.size <= lfs3->cfg->inline_size && file->cache.size <= lfs3->cfg->inline_size
&& file->cache.d.size <= lfs3->cfg->fragment_size && file->cache.size <= lfs3->cfg->fragment_size
&& file->cache.d.size < lfs3->cfg->crystal_thresh)) { && file->cache.size < lfs3->cfg->crystal_thresh)) {
err = lfs3_file_flush(lfs3, file); err = lfs3_file_flush(lfs3, file);
if (err) { if (err) {
goto failed; goto failed;
@@ -13991,12 +13995,12 @@ int lfs3_file_truncate(lfs3_t *lfs3, lfs3_file_t *file, lfs3_off_t size_) {
} }
// truncate our cache // truncate our cache
file->cache.d.size = lfs3_min( file->cache.size = lfs3_min(
file->cache.d.size, file->cache.size,
size_ - lfs3_min(file->cache.pos, size_)); size_ - lfs3_min(file->cache.pos, size_));
file->cache.pos = lfs3_min(file->cache.pos, size_); file->cache.pos = lfs3_min(file->cache.pos, size_);
// mark as flushed if this completely truncates our cache // mark as flushed if this completely truncates our cache
if (file->cache.d.size == 0) { if (file->cache.size == 0) {
lfs3_file_discardcache(file); lfs3_file_discardcache(file);
} }
@@ -14073,27 +14077,27 @@ int lfs3_file_fruncate(lfs3_t *lfs3, lfs3_file_t *file, lfs3_off_t size_) {
} }
// fruncate our cache // fruncate our cache
lfs3_memmove(file->cache.d.u.buffer_, lfs3_memmove(file->cache.buffer,
&file->cache.d.u.buffer_[lfs3_min( &file->cache.buffer[lfs3_min(
lfs3_smax( lfs3_smax(
size - size_ - file->cache.pos, size - size_ - file->cache.pos,
0), 0),
file->cache.d.size)], file->cache.size)],
file->cache.d.size - lfs3_min( file->cache.size - lfs3_min(
lfs3_smax( lfs3_smax(
size - size_ - file->cache.pos, size - size_ - file->cache.pos,
0), 0),
file->cache.d.size)); file->cache.size));
file->cache.d.size -= lfs3_min( file->cache.size -= lfs3_min(
lfs3_smax( lfs3_smax(
size - size_ - file->cache.pos, size - size_ - file->cache.pos,
0), 0),
file->cache.d.size); file->cache.size);
file->cache.pos -= lfs3_smin( file->cache.pos -= lfs3_smin(
size - size_, size - size_,
file->cache.pos); file->cache.pos);
// mark as flushed if this completely truncates our cache // mark as flushed if this completely truncates our cache
if (file->cache.d.size == 0) { if (file->cache.size == 0) {
lfs3_file_discardcache(file); lfs3_file_discardcache(file);
} }
lfs3.h
+4 -2
View File
@@ -636,7 +636,6 @@ typedef struct lfs3_data {
lfs3_size_t size; lfs3_size_t size;
union { union {
const uint8_t *buffer; const uint8_t *buffer;
uint8_t *buffer_;
struct { struct {
lfs3_block_t block; lfs3_block_t block;
lfs3_size_t off; lfs3_size_t off;
@@ -742,11 +741,14 @@ typedef struct lfs3_file {
#endif #endif
// in-RAM cache // in-RAM cache
//
// note this lines up with lfs3_data_t's buffer representation
struct { struct {
#ifndef LFS3_KVONLY #ifndef LFS3_KVONLY
lfs3_off_t pos; lfs3_off_t pos;
#endif #endif
lfs3_data_t d; lfs3_off_t size;
uint8_t *buffer;
} cache; } cache;
// on-disk leaf bptr // on-disk leaf bptr