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.