From e7341686bbd7e21571a674ac444102566a8d6d47 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 20 Feb 2024 18:30:52 -0600 Subject: [PATCH] Ended up implementing direct pcache access in bd prog utils I may have been slightly nerd sniped. I did start to worry about where evicting the rcache could lead to performance pitfalls. One concerning, and not out-there case: - Consider converting an inlined sparse file into a block. If the file is sparse, we may end up with a number of lfsr_bd_set calls to fill holes, but these holes may be quite small. If rcache is quite big, we benefit greatly from keeping it in memory during this operation. If rcache == block_size, we can even get away with a single read. But lfsr_bd_set hijacking the rcache would through a wrench in this, forcing rcache eviction and a reread for every hole. That and after sitting on it for a bit, trading IO for CPU feels wrong. Even if the IO penalty is rare. So decided to revisit and implement the same optimization we have for bd read utils for bd prog utils. --- Implementation wise is basically the same as the read case, with some small differences: - We need to flush the pcache in both caching and bypassing progs, fortunately lfsr_bd_flush is already its own function. - It's up to the caller the evaluate the eager cksum. So there is now an explicit crc32c call in both lfsr_bd_prog and lfsr_bd_set. Though lfsr_bd_set never actually uses the eager cksum. We let cross-function const propagation optimize this out in case we do need it in the future. - lfsr_bd_prognext assumes the prog succeeds in the calling bd util, even though the data has not been written yet. If the bd util errors before writing the data, the prog MUST be dropped or garbage will be written. - lfsr_bd_prognext only works because we lazily flush our pcache So I guess the lazy flushing is a requirement now, instead of an implementation quirk. At least lfsr_bd_prog is off the stack-hot-path this time, so no stack changes: code stack before: 33792 2872 after: 33948 (+0.5%) 2872 (+0.0%) --- lfs.c | 143 ++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 93 insertions(+), 50 deletions(-) diff --git a/lfs.c b/lfs.c index b3ef6932..407fc52b 100644 --- a/lfs.c +++ b/lfs.c @@ -160,7 +160,8 @@ static int lfsr_bd_prog_(lfs_t *lfs, lfs_block_t block, lfs_size_t off, } static int lfsr_bd_readnext(lfs_t *lfs, - lfs_block_t block, lfs_size_t off, lfs_size_t hint, lfs_size_t size, + lfs_block_t block, lfs_size_t off, lfs_size_t hint, + lfs_size_t size, const uint8_t **buffer_, lfs_size_t *size_) { // check for in-bounds LFS_ASSERT(block < lfs->cfg->block_count); @@ -309,11 +310,54 @@ static int lfsr_bd_flush(lfs_t *lfs, uint32_t *flcksum_) { return 0; } +static int lfsr_bd_prognext(lfs_t *lfs, lfs_block_t block, lfs_size_t off, + lfs_size_t size, + uint8_t **buffer_, lfs_size_t *size_, + uint32_t *flcksum_) { + // check for in-bounds + LFS_ASSERT(block < lfs->cfg->block_count); + if (off+size > lfs->cfg->block_size) { + return LFS_ERR_RANGE; + } + + // need to flush pcache? + if (!(block == lfs->pcache.block + && off >= lfs->pcache.off + && off < lfs->pcache.off + lfs->cfg->cache_size)) { + int err = lfsr_bd_flush(lfs, flcksum_); + if (err) { + return err; + } + } + + // unused pcache? make sure to move it so we never overwrite + if (lfs->pcache.size == 0) { + lfs->pcache.block = block; + lfs->pcache.off = lfs_aligndown(off, lfs->cfg->prog_size); + } + + // zero to avoid any information leaks + memset(&lfs->pcache.buffer[lfs->pcache.size], + 0xff, + (off-lfs->pcache.off) - lfs->pcache.size); + lfs->pcache.size = lfs_max( + lfs->pcache.size, + lfs_min( + (off-lfs->pcache.off) + size, + lfs->cfg->cache_size)); + + *buffer_ = &lfs->pcache.buffer[off-lfs->pcache.off]; + *size_ = lfs_min( + size, + lfs->cfg->cache_size - (off-lfs->pcache.off)); + return 0; +} + // caching prog // // this has two ways to calculate a cksum, we end up using both: // - cksum - cksum immediately -// - flcksum - wait until flush to cksum +// - flcksum - cksum on flush // static int lfsr_bd_prog(lfs_t *lfs, lfs_block_t block, lfs_size_t off, const void *buffer, lfs_size_t size, @@ -329,13 +373,21 @@ static int lfsr_bd_prog(lfs_t *lfs, lfs_block_t block, lfs_size_t off, lfs_size_t size_ = size; while (size_ > 0) { // bypass cache? - // - // make sure we flush our pcache first since some devices - // don't support out-of-order progs in a block - // - if (lfs->pcache.size == 0 - && off_ % lfs->cfg->prog_size == 0 - && size_ >= lfs->cfg->prog_size) { + if (off_ % lfs->cfg->prog_size == 0 + && size_ >= lfs->cfg->prog_size + // pcache takes priority + && !(block == lfs->pcache.block + && off_ >= lfs->pcache.off + && off_ < lfs->pcache.off + lfs->cfg->cache_size)) { + // make sure we flush our pcache first, some devices + // don't support out-of-order progs in a block + if (lfs->pcache.size != 0) { + int err = lfsr_bd_flush(lfs, flcksum_); + if (err) { + return err; + } + } + lfs_size_t d = lfs_aligndown(size_, lfs->cfg->prog_size); int err = lfsr_bd_prog_(lfs, block, off_, buffer_, d, flcksum_); @@ -349,41 +401,20 @@ static int lfsr_bd_prog(lfs_t *lfs, lfs_block_t block, lfs_size_t off, continue; } - // fits in pcache? - if (lfs->pcache.size == 0 - || (block == lfs->pcache.block - && off_ >= lfs->pcache.off - && off_ < lfs->pcache.off + lfs->cfg->cache_size)) { - // unused pcache? make sure to move it so we never overwrite - if (lfs->pcache.size == 0) { - lfs->pcache.block = block; - lfs->pcache.off = lfs_aligndown(off_, lfs->cfg->prog_size); - } - - // zero to avoid any information leaks - memset(&lfs->pcache.buffer[lfs->pcache.size], - 0xff, - (off_-lfs->pcache.off) - lfs->pcache.size); - - lfs_size_t d = lfs_min( - size_, - lfs->cfg->cache_size - (off_-lfs->pcache.off)); - memcpy(&lfs->pcache.buffer[off_-lfs->pcache.off], buffer_, d); - lfs->pcache.size = lfs_max( - lfs->pcache.size, - (off_-lfs->pcache.off) + d); - - off_ += d; - buffer_ += d; - size_ -= d; - continue; - } - - // flush pcache so the above can't fail - int err = lfsr_bd_flush(lfs, flcksum_); + uint8_t *buffer__; + lfs_size_t size__; + int err = lfsr_bd_prognext(lfs, block, off_, size_, + &buffer__, &size__, + flcksum_); if (err) { return err; } + + memcpy(buffer__, buffer_, size__); + + off_ += size__; + buffer_ += size__; + size_ -= size__; } // optional checksum @@ -423,7 +454,8 @@ static int lfsr_bd_erase(lfs_t *lfs, lfs_block_t block) { // other block device utils static int lfsr_bd_cksum(lfs_t *lfs, - lfs_block_t block, lfs_size_t off, lfs_size_t hint, lfs_size_t size, + lfs_block_t block, lfs_size_t off, lfs_size_t hint, + lfs_size_t size, uint32_t *cksum_) { // check for in-bounds LFS_ASSERT(block < lfs->cfg->block_count); @@ -528,20 +560,31 @@ static int lfsr_bd_cpy(lfs_t *lfs, static int lfsr_bd_set(lfs_t *lfs, lfs_block_t block, lfs_size_t off, uint8_t c, lfs_size_t size, uint32_t *cksum_, uint32_t *flcksum_) { - // hijack the rcache - lfsr_cache_drop(&lfs->rcache); - memset(lfs->rcache.buffer, c, lfs_min(size, lfs->cfg->cache_size)); + // check for in-bounds + LFS_ASSERT(block < lfs->cfg->block_count); + if (off+size > lfs->cfg->block_size) { + return LFS_ERR_RANGE; + } while (size > 0) { - lfs_size_t d = lfs_min(size, lfs->cfg->cache_size); - int err = lfsr_bd_prog(lfs, block, off, lfs->rcache.buffer, d, - cksum_, flcksum_); + uint8_t *buffer__; + lfs_size_t size__; + int err = lfsr_bd_prognext(lfs, block, off, size, + &buffer__, &size__, + flcksum_); if (err) { return err; } - off += d; - size -= d; + memset(buffer__, c, size__); + + // optional checksum + if (cksum_) { + *cksum_ = lfs_crc32c(*cksum_, buffer__, size__); + } + + off += size__; + size -= size__; } return 0;