From d690ae5162eefdb16a8edea5d15ebd7eaac30b02 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 19 Feb 2024 02:59:37 -0600 Subject: [PATCH] Changed pcache/rcache interactions to wait to overwrite until flush Previous versions of littlefs saw very little pcache/rcache interaction, which was a nice simplification for the bd layer. But now, with rbyds, we rely overlapping pcaches/rcaches heavily. This is because building each rbyd trunk requires reading the previous rbyd trunk, which may have not made it to disk yet. The main issue this presents, is that reads always need to prioritize data in the pcache, even if it doesn't exist on disk yet. This gets a bit annoying with read/prog alignment requirements, which may require disk-reads that overlap the pcache. And even more annoying when you consider that after a flush, the rcache should reflect the new data even if pcache is dropped. The fact that the current impl works at all is because of tests and sweat... --- To solve these problems, the bd layer would overwrite the rcache on prog. This alone wasn't sufficient however, as we also need to overwrite the rcache on reads because of the above alignment issue. So: pcache rcache ................ ................ read(0..4) ................ aaaa............ prog(6..10) ......bbbb...... aaaa..bbbb...... read(0..8) ......bbbb...... aaaaccbbbb...... => aaaaccbb flush() ................ aaaaccbbbb...... read(0..8) ................ aaaaccbbbb...... => aaaacbbb Note we can't just not overwrite the rcache, since flushing the pcache leaves us with out-of-date information: pcache rcache ................ ................ read(0..4) ................ aaaa............ prog(6..10) ......bbbb...... aaaa............ read(0..8) ......bbbb...... aaaacccc........ => aaaaccbb flush() ................ aaaacccc........ read(0..8) ................ aaaacccc........ => aaaacccc !!! This commit adopts a slightly different strategy: overwrite when we flush: pcache rcache ................ ................ read(0..4) ................ aaaa............ prog(6..10) ......bbbb...... aaaa............ read(0..8) ......bbbb...... aaaacccc........ => aaaaccbb flush() ................ aaaaccbbbb...... read(0..8) ................ aaaaccbbbb...... => aaaaccbb This keeps the rcache always in sync with disk (we don't care if pcache is dropped without a flush), leaving unflushed pcache overwrites up to lfsr_bd_read, which it needs to handle correctly anyways because of the above alingment issue. This saves a single overwrite. Which isn't really that much when it comes to code cost: code stack before: 33560 2808 after: 33544 (-0.0%) 2800 (-0.3%) But hey at least we're doing fewer copies? And no one should be tempted to remove the overwrite-on-read code thinking it's redundant now (wasn't me!). --- lfs.c | 157 ++++++++++++++++++++++++++++------------------------------ 1 file changed, 75 insertions(+), 82 deletions(-) diff --git a/lfs.c b/lfs.c index 427fcf9d..5c43f82a 100644 --- a/lfs.c +++ b/lfs.c @@ -105,30 +105,6 @@ static inline void lfsr_cache_drop(lfs_cache_t *cache) { cache->size = 0; } -static int lfsr_bd_read__(lfs_t *lfs, lfs_block_t block, lfs_size_t off, - void *buffer, lfs_size_t size) { - int err = lfsr_bd_read_(lfs, block, off, buffer, size); - if (err) { - return err; - } - - // overwrite with pcache, since pcache may contain newer data - if (block == lfs->pcache.block - && off < lfs->pcache.off + lfs->pcache.size - && off + size > lfs->pcache.off) { - lfs_size_t off_ = lfs_max(off, lfs->pcache.off); - uint8_t *buffer_ = buffer; - lfs_size_t size_ = lfs_min( - size - (off_-off), - lfs->pcache.size - (off_-lfs->pcache.off)); - memcpy(&buffer_[off_-off], - &lfs->pcache.buffer[off_-lfs->pcache.off], - size_); - } - - return 0; -} - // caching read // // note hint has two convenience: @@ -185,7 +161,7 @@ static int lfsr_bd_read(lfs_t *lfs, && off_ % lfs->cfg->read_size == 0 && size_ >= lfs->cfg->read_size) { lfs_size_t d = lfs_aligndown(size_, lfs->cfg->read_size); - int err = lfsr_bd_read__(lfs, block, off_, buffer_, d); + int err = lfsr_bd_read_(lfs, block, off_, buffer_, d); if (err) { return err; } @@ -210,7 +186,7 @@ static int lfsr_bd_read(lfs_t *lfs, lfs->cfg->block_size) - off__, lfs->cfg->cache_size); - int err = lfsr_bd_read__(lfs, block, off__, + int err = lfsr_bd_read_(lfs, block, off__, lfs->rcache.buffer, size__); if (err) { return err; @@ -221,11 +197,80 @@ static int lfsr_bd_read(lfs_t *lfs, lfs->rcache.size = size__; } + // overwrite with pcache, since pcache may contain newer data + if (block == lfs->pcache.block + && off < lfs->pcache.off + lfs->pcache.size + && off + size > lfs->pcache.off) { + lfs_size_t off_ = lfs_max(off, lfs->pcache.off); + uint8_t *buffer_ = buffer; + lfs_size_t size_ = lfs_min( + size - (off_-off), + lfs->pcache.size - (off_-lfs->pcache.off)); + memcpy(&buffer_[off_-off], + &lfs->pcache.buffer[off_-lfs->pcache.off], + size_); + } + return 0; } -// needed in lfsr_bd_prog -static int lfsr_bd_flush(lfs_t *lfs, uint32_t *flcksum_); +static int lfsr_bd_flush_(lfs_t *lfs, lfs_block_t block, lfs_size_t off, + const void *buffer, lfs_size_t size, + uint32_t *flcksum_) { + int err = lfsr_bd_prog_(lfs, block, off, buffer, size); + if (err) { + return err; + } + + // update rcache if we overlap + if (block == lfs->rcache.block + && off < lfs->rcache.off + lfs->rcache.size + && off + size > lfs->rcache.off) { + lfs_size_t off_ = lfs_max(off, lfs->rcache.off); + const uint8_t *buffer_ = buffer; + lfs_size_t size_ = lfs_min( + size - (off_-off), + lfs->rcache.size - (off_-lfs->rcache.off)); + memcpy(&lfs->rcache.buffer[off_-lfs->rcache.off], + &buffer_[off_-off], + size_); + } + + // update flushed checksum if requested + if (flcksum_) { + *flcksum_ = lfs_crc32c(*flcksum_, buffer, size); + } + + return 0; +} + +static int lfsr_bd_flush(lfs_t *lfs, uint32_t *flcksum_) { + if (lfs->pcache.size != 0) { + // must be in-bounds + LFS_ASSERT(lfs->pcache.block < lfs->cfg->block_count); + + // zero to avoid any information leaks + lfs_size_t aligned_size = lfs_alignup( + lfs->pcache.size, + lfs->cfg->prog_size); + memset(&lfs->pcache.buffer[lfs->pcache.size], + 0xff, + aligned_size - lfs->pcache.size); + + // flush + int err = lfsr_bd_flush_(lfs, lfs->pcache.block, + lfs->pcache.off, lfs->pcache.buffer, aligned_size, + flcksum_); + if (err) { + return err; + } + + // make this cache available + lfsr_cache_drop(&lfs->pcache); + } + + return 0; +} // caching prog // @@ -255,16 +300,12 @@ static int lfsr_bd_prog(lfs_t *lfs, lfs_block_t block, lfs_size_t off, && off_ % lfs->cfg->prog_size == 0 && size_ >= lfs->cfg->prog_size) { lfs_size_t d = lfs_aligndown(size_, lfs->cfg->prog_size); - int err = lfsr_bd_prog_(lfs, block, off_, buffer_, d); + int err = lfsr_bd_flush_(lfs, block, off_, buffer_, d, + flcksum_); if (err) { return err; } - // update flushed checksum if requested - if (flcksum_) { - *flcksum_ = lfs_crc32c(*flcksum_, buffer_, d); - } - off_ += d; buffer_ += d; size_ -= d; @@ -308,20 +349,6 @@ static int lfsr_bd_prog(lfs_t *lfs, lfs_block_t block, lfs_size_t off, } } - // update rcache if we overlap - if (block == lfs->rcache.block - && off < lfs->rcache.off + lfs->rcache.size - && off + size > lfs->rcache.off) { - lfs_size_t off_ = lfs_max(off, lfs->rcache.off); - const uint8_t *buffer_ = buffer; - lfs_size_t size_ = lfs_min( - size - (off_-off), - lfs->rcache.size - (off_-lfs->rcache.off)); - memcpy(&lfs->rcache.buffer[off_-lfs->rcache.off], - &buffer_[off_-off], - size_); - } - // optional checksum if (cksum_) { *cksum_ = lfs_crc32c(*cksum_, buffer, size); @@ -330,40 +357,6 @@ static int lfsr_bd_prog(lfs_t *lfs, lfs_block_t block, lfs_size_t off, return 0; } -// flush any pending programs -static int lfsr_bd_flush(lfs_t *lfs, uint32_t *flcksum_) { - if (lfs->pcache.size != 0) { - // must be in-bounds - LFS_ASSERT(lfs->pcache.block < lfs->cfg->block_count); - - // zero to avoid any information leaks - lfs_size_t aligned_size = lfs_alignup( - lfs->pcache.size, - lfs->cfg->prog_size); - memset(&lfs->pcache.buffer[lfs->pcache.size], - 0xff, - aligned_size - lfs->pcache.size); - - // flush - int err = lfsr_bd_prog_(lfs, lfs->pcache.block, - lfs->pcache.off, lfs->pcache.buffer, aligned_size); - if (err) { - return err; - } - - // this is when we update the the flushed checksum if requested - if (flcksum_) { - *flcksum_ = lfs_crc32c(*flcksum_, - lfs->pcache.buffer, lfs->pcache.size); - } - - // make this cache available - lfsr_cache_drop(&lfs->pcache); - } - - return 0; -} - static int lfsr_bd_sync(lfs_t *lfs) { // make sure we flush any caches int err = lfsr_bd_flush(lfs, NULL);