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

[Stephen Warren <swarren@wwwdotorg.org>]

On 03/20/2016 07:45 PM, 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 4 and the maximum
> number of blocks per cache entry has a default of 1, which matches the
> current implementation of at least FAT and ext4 that only read a single
> block at a time.

If the maximum size of the cache is fixed and small (which judging by 
the description it is), why not use an array rather than a linked list. 
That would be far simpler to manage.

> The 'blkcache' command (enabled through CONFIG_CMD_BLOCK_CACHE) allows
> changing these values and can be used to tune for a particular filesystem
> layout.

Even with this dynamic adjustment, using an array still feels simpler, 
although I have't looked at the code yet.

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

> +/*
> + * search for a set of blocks in the cache
> + *
> + * remove and return the node if found so it can be
> + * added back at the start of the list and maintain MRU order)
> + */

Splitting the remove/add feels a bit dangerous, since forgetting to 
re-do the add (or e.g. some error causing that to be skipped) could 
cause cache_count to become inconsistent.

The function name "find" is a bit misleading. cache_find_and_remove() 
would make its semantics more obvious. Or, simply put the list_del() 
somewhere else; it doesn't feel like part of a "find" operation to me. 
Or, put the entire move-to-front operation inside this function so it 
isn't split up - if so, cache_find_and_move_to_head().

> +static struct block_cache_node *cache_find
> +	(int iftype, int devnum,

Nit: the ( and first n arguments shouldn't be wrapped.

> +int cache_block_read(int iftype, int devnum,
> +		     lbaint_t start, lbaint_t blkcnt,
> +		     unsigned long blksz, void *buffer)

> +		memcpy(buffer, src, blksz*blkcnt);

Nit: Space around operators. checkpatch should catch this.

> +		if (trace)
> +			printf("hit: start " LBAF ", count " LBAFU "\n",
> +			       start, blkcnt);

I guess I'll find that trace can be adjusted at run-time somewhere later 
in this patch, but it's more typical to use debug() without the if 
statement for this. It would be awesome if debug() could be adjusted at 
run-time...

> +void cache_block_fill(int iftype, int devnum,
...
> +	if (node->cache == 0) {
> +		node->cache = malloc(bytes);
> +		if (!node->cache) {

Nit: may as well be consistent with those NULL checks.

> +void cache_block_invalidate(int iftype, int devnum)
...
> +void cache_block_invalidate_all(void)

There's no invalidate_blocks(); I imagine that writing-to/erasing (some 
blocks of) a device must call cache_block_invalidate() which will wipe 
out even data that wasn't written.

> +static void dump_block_cache(void)
...
> +	list_for_each_safe(entry, n, &block_cache) {

Nit: This doesn't need to be _safe since the list is not being modified.

> +static int do_blkcache(cmd_tbl_t *cmdtp, int flag,
> +		       int argc, char * const argv[])
> +{
> +	if ((argc == 1) || !strcmp("show", argv[1])) {
> +		printf("block cache:\n"
> +		       "%u\thits\n"
> +		       "%u\tmisses\n"
> +		       "%u\tentries in cache\n"
> +		       "trace %s\n"
> +		       "max blocks/entry %lu\n"
> +		       "max entries %lu\n",
> +		       block_cache_hits, block_cache_misses, cache_count,
> +		       trace ? "on" : "off",
> +		       max_blocks_per_entry, max_cache_entries);

Nit: Let's print the value and "label" in a consistent order. I would 
suggest everything being formatted as:

"    description: %u"

so that it's indented, has a delimiter between description and value, 
and so that irrespective of the length of the converted value, the 
indentation of the other parts of the string don't change (\t doesn't 
guarantee this after a certain number length).

I would rather have expected "show" to dump the entries too, but I 
suppose it's fine that they're separate.

> +	} else if ((argc == 2) && ('c' == argv[1][0])) {

Nit: the value of 'c' is always 'c', so it's pointless to test whether 
it's equal to something. The value being compared is arv[1][0], so the 
parameters to == should be swapped. I'm aware that == is mathematically 
commutative, but typical English reading of the operator is "is equal 
to" which has non-commutative implications re: what is being tested. I'm 
also aware that this construct is used to trigger compiler warnings if 
someone types = instead of ==. However, (a) you didn't and (b) compilers 
warn about this these days, so there's no need to write unusual code to 
get that feature anymore.

I worry that being imprecise in command-line parsing (i.e. treating both 
"crud" and "clear" as the same thing) will lead to problems expanding 
the command-line format in the future; we won't be able to ever add 
another option starting with "c" and maintain the same abbreviation 
semantics. I'd suggest requiring the full option name always.

> +	} else if ((argc == 3) && isdigit(argv[1][0]) && isdigit(argv[2][0])) {

Let's make this "blkcache set" or "blkcache configure" so that other 
sub-commands can take numeric parameters in the future, and have 
consistent command-line syntax.

> +U_BOOT_CMD(
> +	blkcache,	3,	0,	do_blkcache,
> +	"block cache control/statistics",
> +	"[show] - show statistics\n"
> +	"blkcache c[lear] - clear statistics\n"
> +	"blkcache d[ump] - dump cache entries\n"
> +	"blkcache i[nvalidate] - invalidate cache\n"
> +	"blkcache #maxblocks #maxentries\n"
> +	"\tset maximum blocks per cache entry, maximum entries\n"
> +	"blkcache t[race] [off] - enable (disable) tracing"
> +);

BTW, isn't there some support in U-Boot for sub-commands already, so you 
can write a separate function per sub-command and skip writing all the 
strcmp logic to select between them? I'm pretty sure I saw that somewhere.

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

> +static inline void cache_block_invalidate_all(void){}

Is that useful outside of the debug commands?