[U-Boot] [PATCH V3 1/3] drivers: block: add block device cache

[Stephen Warren <swarren@wwwdotorg.org>]

On 03/28/2016 11:05 AM, Eric Nelson wrote:
> Add a block device cache to speed up repeated reads of block devices by
> various filesystems.
>
> This small amount of cache can dramatically speed up filesystem
> operations by skipping repeated reads of common areas of a block
> device (typically directory structures).
>
> This has shown to have some benefit on FAT filesystem operations of
> loading a kernel and RAM disk, but more dramatic benefits on ext4
> filesystems when the kernel and/or RAM disk are spread across
> multiple extent header structures as described in commit fc0fc50.
>
> The cache is implemented through a minimal list (block_cache) maintained
> in most-recently-used order and count of the current number of entries
> (cache_count). It uses a maximum block count setting to prevent copies
> of large block reads and an upper bound on the number of cached areas.
>
> The maximum number of entries in the cache defaults to 32 and the maximum
> number of blocks per cache entry has a default of 2, which has shown to
> produce the best results on testing of ext4 and FAT filesystems.
>
> The 'blkcache' command (enabled through CONFIG_CMD_BLOCK_CACHE) allows
> changing these values and can be used to tune for a particular filesystem
> layout.

> diff --git a/cmd/blkcache.c b/cmd/blkcache.c

> +static int blkc_show(cmd_tbl_t *cmdtp, int flag,
> +		     int argc, char * const argv[])
> +{
> +	struct block_cache_stats stats;
> +	blkcache_stats(&stats);
> +
> +	printf("    hits: %u\n"
> +	       "    misses: %u\n"
> +	       "    entries: %u\n"
> +	       "    max blocks/entry: %u\n"
> +	       "    max cache entries: %u\n",

Nit: I'm not sure why all that command output is indented. Perhaps it 
made sense before if some kind of title was printed before that text, 
but I don't think it is any more.

> +static int do_blkcache(cmd_tbl_t *cmdtp, int flag,

> +	c = find_cmd_tbl(argv[0], &cmd_blkc_sub[0], ARRAY_SIZE(cmd_blkc_sub));
> +
> +	if (c)
> +		return c->cmd(cmdtp, flag, argc, argv);
> +	else
> +		return CMD_RET_USAGE;
> +
> +	return 0;

Nit: I'd prefer to see the error handling immediately follow the 
function call whose result is being tested, and the non-failure-case 
code be not indented, i.e.:


c = find_cmd_tbl(argv[0], &cmd_blkc_sub[0], ARRAY_SIZE(cmd_blkc_sub));
if (!c)
     return CMD_RET_USAGE;

return c->cmd(cmdtp, flag, argc, argv);

> diff --git a/disk/part.c b/disk/part.c

> @@ -268,6 +268,8 @@ void part_init(struct blk_desc *dev_desc)
>   	const int n_ents = ll_entry_count(struct part_driver, part_driver);
>   	struct part_driver *entry;
>
> +	blkcache_invalidate(dev_desc->if_type, dev_desc->devnum);

Doesn't this invalidate the cache far too often? I expect that function 
is called for command the user executes from the command-line, whereas 
it'd be nice if the cache persisted across commands. I suppose this is a 
reasonable (and very safe) first implementation though, and saves having 
to go through each storage provider type and find out the right place to 
detect media changes.

> diff --git a/drivers/block/blkcache.c b/drivers/block/blkcache.c

> +struct block_cache_node {
> +	struct list_head lh;
> +	int iftype;
> +	int devnum;
> +	lbaint_t start;
> +	lbaint_t blkcnt;
> +	unsigned long blksz;
> +	char *cache;
> +};
> +
> +static LIST_HEAD(block_cache);
> +
> +static struct block_cache_stats _stats = {
> +	.max_blocks_per_entry = 2,
> +	.max_entries = 32
> +};

Now is a good time to mention another reason why I don't like using a 
dynamically allocated linked list for this: Memory fragmentation. By 
dynamically allocating the cache, we could easily run into a situation 
where the user runs a command that allocates memory and also adds to the 
block cache, then most of that memory gets freed when U-Boot returns to 
the command prompt, then the user runs the command again but it fails 
since it can't allocate the memory due to fragmentation of the heap. 
This is a real problem I've seen e.g. with the "ums" and "dfu" commands, 
since they might initialize the USB controller the first time they're 
run, which allocates some new memory. Statically allocation would avoid 
this.

> diff --git a/include/blk.h b/include/blk.h

> +int blkcache_read
> +	(int iftype, int dev,
> +	 lbaint_t start, lbaint_t blkcnt,
> +	 unsigned long blksz, void *buffer);

Nit: The opening ( and first n arguments should be wrapped onto the same 
line as the function name.