| Message ID | 1458164424-15363-2-git-send-email-eric@nelint.com |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
On 03/16/2016 03:40 PM, Eric Nelson wrote: > Signed-off-by: Eric Nelson <eric@nelint.com> A patch description would be useful here; the cover letter wouldn't be checked in. > diff --git a/drivers/block/cache_block.c b/drivers/block/cache_block.c > +static int cache_iftype = -1; > +static int cache_devnum = -1; > +static lbaint_t cache_start = -1; > +static lbaint_t cache_blkcnt = -1; > +static unsigned long cache_blksz; > +static void *cache; Since variable "cache" is in BSS and hence is 0, which is included in all checks below, none of the other values need to be initialized. > +int cache_block_read(int iftype, int dev, > + lbaint_t start, lbaint_t blkcnt, > + unsigned long blksz, void *buffer) > +{ > + if ((iftype == cache_iftype) && > + (dev == cache_devnum) && > + (start == cache_start) && > + (blkcnt <= cache_blkcnt) && > + (blksz == cache_blksz) && > + (cache != 0)) { I'd suggest putting (cache != 0) first, since that check indicates whether any of the others are relevant. > + memcpy(buffer, cache, blksz*blkcnt); Nit: space around the * operator. Same comment everywhere. It's probably hard to fix this given the simple API and lack of MMU page-tables to remap on a per-page basis, but one deficiency in this (deliberately) simple implementation is that if someone caches blocks 0..7 then later tries to read 2..10 (or even 2..3) they won't get a cache hit. While partial hits (the 2..10 case) are hard to deal with, perhaps it's useful to make subset cases (2..3 with 0..7 cached) work? > +void cache_block_fill(int iftype, int dev, > + lbaint_t start, lbaint_t blkcnt, > + unsigned long blksz, void const *buffer) > + bytes = blksz*blkcnt; > + if (cache != 0) { > + if (bytes != (cache_blksz*cache_blkcnt)) { > + free(cache); > + cache = malloc(blksz*blkcnt); > + if (!cache) > + return; > + } /* change in size */ > + } else { > + cache = malloc(blksz*blkcnt); > + if (!cache) > + return; > + } Is it better to put the if (!cache) test after the whole if/else block so it's unified? > +void cache_block_invalidate(int iftype, int dev) > +{ > + cache_iftype = -1; That isn't actually necessary, since assigning 0 to cache below will prevent anything from using the cache. > + if (cache) { > + free(cache); > + cache = 0; > + } > +} > diff --git a/include/part.h b/include/part.h > +#ifdef CONFIG_BLOCK_CACHE > +/** > + * cache_block_read() - attempt to read a set of blocks from cache > + * > + * @param iftype - IF_TYPE_x for type of device > + * @param dev - device index of particular type > + * @param start - starting block number > + * @param blkcnt - number of blocks to read > + * @param blksz - size in bytes of each block > + * @param buf - buffer to contain cached data s/contain/receive/? > + * > + * @return - '1' if block returned from cache, '0' otherwise. > + */ > +int cache_block_read > + (int iftype, int dev, > + lbaint_t start, lbaint_t blkcnt, > + unsigned long blksz, void *buffer); Nit: ( and probably the first n parameters should be on the same line as the function name.
Thanks for the review(s) Stephen. On 03/17/2016 02:16 PM, Stephen Warren wrote: > On 03/16/2016 03:40 PM, Eric Nelson wrote: >> Signed-off-by: Eric Nelson <eric@nelint.com> > > A patch description would be useful here; the cover letter wouldn't be > checked in. > Yeah. Please hote the RFC. I was really hoping for some broader feedback about whether this is a better approach than the more specialized ext4 extent cache. If I can get an ack on the approach, I think a minimal block device cache would support at least 2 or 4 entries, and I'd need to be able to answer the questions from your other response: > Do you have any stats on how many operations this saves for typical FS operations such as: > > - Partition table type identification (with various types such as MBR/DOS, GPT, ...) > - Partition enumeration > - Filesystem identification (with various filesystems such as FAT, ext, ...) > - File reads Should I interpret this as support of a small(ish) block device cache? Regards, Eric
On 03/17/2016 03:33 PM, Eric Nelson wrote: > Thanks for the review(s) Stephen. > > On 03/17/2016 02:16 PM, Stephen Warren wrote: >> On 03/16/2016 03:40 PM, Eric Nelson wrote: >>> Signed-off-by: Eric Nelson <eric@nelint.com> >> >> A patch description would be useful here; the cover letter wouldn't be >> checked in. >> > > Yeah. Please hote the RFC. > > I was really hoping for some broader feedback about whether this > is a better approach than the more specialized ext4 extent cache. > > If I can get an ack on the approach, I think a minimal block > device cache would support at least 2 or 4 entries, and I'd > need to be able to answer the questions from your other > response: > >> Do you have any stats on how many operations this saves for typical FS operations such as: >> >> - Partition table type identification (with various types such as MBR/DOS, GPT, ...) >> - Partition enumeration >> - Filesystem identification (with various filesystems such as FAT, ext, ...) >> - File reads > > Should I interpret this as support of a small(ish) block device cache? Conceptually I think it sounds OK provided it yields some demonstrable improvement across a variety of use-cases, and the design doesn't prevent it addressing obvious other cases it should address. You probably want to Cc (or hope they see this and provide feedback anyway) a few other people such as Tom Rini, Simon Glass, Marek Vasut, etc.
On Thu, Mar 17, 2016 at 02:33:04PM -0700, Eric Nelson wrote: > Thanks for the review(s) Stephen. > > On 03/17/2016 02:16 PM, Stephen Warren wrote: > > On 03/16/2016 03:40 PM, Eric Nelson wrote: > >> Signed-off-by: Eric Nelson <eric@nelint.com> > > > > A patch description would be useful here; the cover letter wouldn't be > > checked in. > > > > Yeah. Please hote the RFC. > > I was really hoping for some broader feedback about whether this > is a better approach than the more specialized ext4 extent cache. Well, I guess it comes down to how hard it is to also re-use this on say USB (another common case and one I assume you can test on your HW) since all of EXT4 and FAT and MMC and USB are fairly common and we should be able to address these with your approach, or at least be within "rework some other stuff and then.." distance.
Thanks for the feedback Tom, On 03/20/2016 03:13 PM, Tom Rini wrote: > On Thu, Mar 17, 2016 at 02:33:04PM -0700, Eric Nelson wrote: >> Thanks for the review(s) Stephen. >> >> On 03/17/2016 02:16 PM, Stephen Warren wrote: >>> On 03/16/2016 03:40 PM, Eric Nelson wrote: >>>> Signed-off-by: Eric Nelson <eric@nelint.com> >>> >>> A patch description would be useful here; the cover letter wouldn't be >>> checked in. >>> >> >> Yeah. Please hote the RFC. >> >> I was really hoping for some broader feedback about whether this >> is a better approach than the more specialized ext4 extent cache. > > Well, I guess it comes down to how hard it is to also re-use this on say > USB (another common case and one I assume you can test on your HW) since > all of EXT4 and FAT and MMC and USB are fairly common and we should be > able to address these with your approach, or at least be within "rework > some other stuff and then.." distance. > It should be trivial to include this support for USB, SATA, IDE, or any other block device, but not transparent. I'm close to a V2 RFC patch so people can throw stones at an actual implementation. Regards, Eric
diff --git a/drivers/block/Makefile b/drivers/block/Makefile index b5c7ae1..056a48b 100644 --- a/drivers/block/Makefile +++ b/drivers/block/Makefile @@ -24,3 +24,4 @@ obj-$(CONFIG_IDE_SIL680) += sil680.o obj-$(CONFIG_SANDBOX) += sandbox.o obj-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o obj-$(CONFIG_SYSTEMACE) += systemace.o +obj-$(CONFIG_BLOCK_CACHE) += cache_block.o diff --git a/drivers/block/cache_block.c b/drivers/block/cache_block.c new file mode 100644 index 0000000..12e60ac --- /dev/null +++ b/drivers/block/cache_block.c @@ -0,0 +1,76 @@ +/* + * Copyright (C) Nelson Integration, LLC 2016 + * Author: Eric Nelson<eric@nelint.com> + * + * SPDX-License-Identifier: GPL-2.0+ + * + */ +#include <config.h> +#include <common.h> +#include <malloc.h> +#include <part.h> + +#define MAX_CACHEBLOCKS 8 + +static int cache_iftype = -1; +static int cache_devnum = -1; +static lbaint_t cache_start = -1; +static lbaint_t cache_blkcnt = -1; +static unsigned long cache_blksz; +static void *cache; + +int cache_block_read(int iftype, int dev, + lbaint_t start, lbaint_t blkcnt, + unsigned long blksz, void *buffer) +{ + if ((iftype == cache_iftype) && + (dev == cache_devnum) && + (start == cache_start) && + (blkcnt <= cache_blkcnt) && + (blksz == cache_blksz) && + (cache != 0)) { + memcpy(buffer, cache, blksz*blkcnt); + return 1; + } + return 0; +} + +void cache_block_fill(int iftype, int dev, + lbaint_t start, lbaint_t blkcnt, + unsigned long blksz, void const *buffer) +{ + lbaint_t bytes; + + /* don't cache big stuff */ + if (blkcnt > MAX_CACHEBLOCKS) + return; + + bytes = blksz*blkcnt; + if (cache != 0) { + if (bytes != (cache_blksz*cache_blkcnt)) { + free(cache); + cache = malloc(blksz*blkcnt); + if (!cache) + return; + } /* change in size */ + } else { + cache = malloc(blksz*blkcnt); + if (!cache) + return; + } + memcpy(cache, buffer, bytes); + cache_iftype = iftype; + cache_devnum = dev; + cache_start = start; + cache_blkcnt = blkcnt; + cache_blksz = blksz; +} + +void cache_block_invalidate(int iftype, int dev) +{ + cache_iftype = -1; + if (cache) { + free(cache); + cache = 0; + } +} diff --git a/include/part.h b/include/part.h index 6d8f520..21f820f 100644 --- a/include/part.h +++ b/include/part.h @@ -369,4 +369,69 @@ int gpt_verify_partitions(struct blk_desc *dev_desc, gpt_header *gpt_head, gpt_entry **gpt_pte); #endif +#ifdef CONFIG_BLOCK_CACHE +/** + * cache_block_read() - attempt to read a set of blocks from cache + * + * @param iftype - IF_TYPE_x for type of device + * @param dev - device index of particular type + * @param start - starting block number + * @param blkcnt - number of blocks to read + * @param blksz - size in bytes of each block + * @param buf - buffer to contain cached data + * + * @return - '1' if block returned from cache, '0' otherwise. + */ +int cache_block_read + (int iftype, int dev, + lbaint_t start, lbaint_t blkcnt, + unsigned long blksz, void *buffer); + +/** + * cache_block_fill() - make data read from a block device available + * to the block cache + * + * @param iftype - IF_TYPE_x for type of device + * @param dev - device index of particular type + * @param start - starting block number + * @param blkcnt - number of blocks available + * @param blksz - size in bytes of each block + * @param buf - buffer containing data to cache + * + */ +void cache_block_fill + (int iftype, int dev, + lbaint_t start, lbaint_t blkcnt, + unsigned long blksz, void const *buffer); + +/** + * cache_block_invalidate() - discard the cache for a set of blocks + * because of a write or device (re)initialization. + * + * @param iftype - IF_TYPE_x for type of device + * @param dev - device index of particular type + */ +void cache_block_invalidate + (int iftype, int dev); + +#else + +static inline int cache_block_read + (int iftype, int dev, + lbaint_t start, lbaint_t blkcnt, + unsigned long blksz, void *buffer) +{ + return 0; +} + +static inline void cache_block_fill + (int iftype, int dev, + lbaint_t start, lbaint_t blkcnt, + unsigned long blksz, void const *buffer) {} + +static inline void cache_block_invalidate + (int iftype, int dev) {} + +#endif + #endif /* _PART_H */
Signed-off-by: Eric Nelson <eric@nelint.com> --- drivers/block/Makefile | 1 + drivers/block/cache_block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++ include/part.h | 65 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 142 insertions(+) create mode 100644 drivers/block/cache_block.c