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

[Eric Nelson <eric@nelint.com>]

Hi Tom,

On 03/30/2016 08:19 AM, Tom Rini wrote:
> On Wed, Mar 30, 2016 at 08:36:21AM -0600, 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]

>>> @@ -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.
> 
> My initial reaction here is that we should stay conservative and
> invalidate the cache more often rather than too infrequent.  I mean,
> what's the worst case here, an extra read? A few extra reads?  We want
> to make sure we keep the complexity to functionality ratio right here,
> if we make the recovery/flashing/factory cases a whole lot better but
> are leaving 1 second of wall clock time on the table when we've just
> gained a minute, we're OK.
> 

I don't think this is called during every command, at least not
with mmc.

>>> 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.
> 
> That is a good point.  But how would you hit this?  The problem in
> ums/dfu was that it was several megabytes, yes?  My quick read over the
> code right now has me thinking this is something measured in kilobytes.
> 

36 bytes for the node, plus 512 bytes or 1k for the block data unless
tuned through the blkcache command.

And the block data is going to be allocated at the same time.

Regards,


Eric