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

[Eric Nelson]

Thanks again for the detailed review, Stephen.

On 03/30/2016 07:36 AM, Stephen Warren wrote:
> 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.
>>
<snip>
>> +
>> +    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.
> 

Okay.

>> +    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);
> 

Okay. As Tom pointed out, I copied this verbatim from i2c.c.

>> 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.
> 

I'm not sure it does. I traced through the mmc initialization and it's
only called when the card itself is initialized.

IOW, at the point where we'd recognize a card swap.

>> 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.
> 

We're going to allocate a block or set of blocks every time we allocate
a new node for the list, so having the list in an array doesn't fix the
problem.

While re-working the code, I also thought more about using an array and
still don't see how the implementation doesn't get more complex.

The key bit is that the list is implemented in MRU order so
invalidating the oldest is trivial.

Doing this in an array would require keeping the array sorted (a bad
idea) or keeping an access sequence in each node of the array and
searching for oldest when invalidation is needed (when the max entries
limit is hit).

... unless I'm still missing something and you or Marek have something
else in mind.

I could also allocate an array of "free" nodes once the first time
around (as a buffer pool), but I don't think this is warranted because
of the block allocations I mentioned above.

>> 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.

Aargh. You've now pointed this out several times and I keep missing it.

Will fix in V4.

Regards,


Eric