[U-Boot] ext4 and caching

[U-Boot] ext4 and caching

[Eric Nelson]

	Hi all,

I've been seeing the same sort of issues repoted by Ionut
and as addressed by this patch:
	http://lists.denx.de/pipermail/u-boot/2014-January/171459.html

That patch was added in commit fc0fc50 and reverted in commit 715b56f.

It no longer applies cleanly, and when I tried to resurrect it,
I saw errors traversing directories and perhaps something went
wrong with my merge.

Ionut, do you have a current version of this patch?

When I looked a little further, I found that a read of a ~150 MiB
was around 30x slower than typical, and that the **same** 8 blocks
accounted for the majority of the reads.

In fact, these same blocks were read back-to-back.

The following is a quick picture of the output from a simple
printf in mmc_bread of the block number and count during a
load of the problem file.

~$ uniq -c < mmc_bread.log | sort -n
      1 mmc_bread: 0/1
      1 mmc_bread: 2293760/1
      1 mmc_bread: 2293762/2
      1 mmc_bread: 2293768/1
      1 mmc_bread: 2293768/1
      1 mmc_bread: 2293768/1
      1 mmc_bread: 2293768/1
      1 mmc_bread: 2295264/1
      1 mmc_bread: 2295270/1
      1 mmc_bread: 2295290/1
      1 mmc_bread: 2295645/1
      1 mmc_bread: 7536640/131072
      1 mmc_bread: 7667712/32768
      1 mmc_bread: 7700480/16384
      1 mmc_bread: 7729152/65536
      1 mmc_bread: 7798784/130722
      1 mmc_bread: 7929506/1
      1 mmc_init: 0, time 129
      2 mmc_bread: 0/1
      6 mmc_bread: 2359120/1
     10 mmc_bread: 2358752/1
     34 mmc_bread: 2358808/1
   2048 mmc_bread: 2557936/8
   4096 mmc_bread: 2557936/8
   8193 mmc_bread: 2557936/8
  16340 mmc_bread: 2557936/8
  16384 mmc_bread: 2557936/8

~$ sort < mmc_bread.log | uniq -c | sort -n | tail -n 1
  47061 mmc_bread: 2557936/8

In English, the 8 blocks starting@2557936 are read
47061 times, and back to back in large (2k/4k/16k) bunches,
so a very simple **single** block cache of the last read
will fix the speed issue and I hacked something up to
verify that.

Is anybody else working on things in this area?

I think this is something that's probably easier to fix
at the block device level rather than within the ext4
filesystem code.

That said, the 2k/4k/16k bunches above may also indicate
a simpler fix in the ext4 code.

Please chime in with your thoughts.

Regards,


Eric Nelson

[U-Boot] [RFC PATCH 0/2] simple cache layer for block devices

[Eric Nelson]

Here's an example of a very simple cache for block devices that 
will prevent duplicate back-to-back reads from the same block
device.

By itself, this is sufficient to speed reads of certain files from
ext4 by 30x as described in this thread.

The areas I think could benefit most from some level of block cache
in U-Boot are:
	- reads from partition tables
	- directory searches

Having more than one cache entry is probably better for both of
those, but I'd like to get some feedback first.

The implementation explicitly prevents reads of more than
8 blocks to prevent slowing down the most speed-critical operations
(reading files) and I'm not sure this is the right choice of size.

The small size limit also prevents the overhead in the interface to
cache_block_fill() from being a problem. A more robust approach would
allow the cache to take over previously allocated blocks, but this
would also be more invasive, since some cacheable blocks are currently
allocated on the stack.

Eric Nelson (2):
  add block device cache
  mmc: add support for block device cache

 drivers/block/Makefile      |  1 +
 drivers/block/cache_block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/mmc.c           | 10 +++++-
 drivers/mmc/mmc_write.c     |  7 +++++
 include/part.h              | 65 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 158 insertions(+), 1 deletion(-)
 create mode 100644 drivers/block/cache_block.c

-- 
2.6.2

[U-Boot] [RFC PATCH 1/2] add block device cache

[Eric Nelson]

Signed-off-by: Eric Nelson <eric@nelint.com>
---
 drivers/block/Makefile      |  1 +
 drivers/block/cache_block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
 include/part.h              | 65 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+)
 create mode 100644 drivers/block/cache_block.c

diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index b5c7ae1..056a48b 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -24,3 +24,4 @@ obj-$(CONFIG_IDE_SIL680) += sil680.o
 obj-$(CONFIG_SANDBOX) += sandbox.o
 obj-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
 obj-$(CONFIG_SYSTEMACE) += systemace.o
+obj-$(CONFIG_BLOCK_CACHE) += cache_block.o
diff --git a/drivers/block/cache_block.c b/drivers/block/cache_block.c
new file mode 100644
index 0000000..12e60ac
--- /dev/null
+++ b/drivers/block/cache_block.c
@@ -0,0 +1,76 @@
+/*
+ * Copyright (C) Nelson Integration, LLC 2016
+ * Author: Eric Nelson<eric@nelint.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ *
+ */
+#include <config.h>
+#include <common.h>
+#include <malloc.h>
+#include <part.h>
+
+#define MAX_CACHEBLOCKS 8
+
+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;
+
+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)) {
+		memcpy(buffer, cache, blksz*blkcnt);
+		return 1;
+	}
+	return 0;
+}
+
+void cache_block_fill(int iftype, int dev,
+		      lbaint_t start, lbaint_t blkcnt,
+		      unsigned long blksz, void const *buffer)
+{
+	lbaint_t bytes;
+
+	/* don't cache big stuff */
+	if (blkcnt > MAX_CACHEBLOCKS)
+		return;
+
+	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;
+	}
+	memcpy(cache, buffer, bytes);
+	cache_iftype = iftype;
+	cache_devnum = dev;
+	cache_start = start;
+	cache_blkcnt = blkcnt;
+	cache_blksz = blksz;
+}
+
+void cache_block_invalidate(int iftype, int dev)
+{
+	cache_iftype = -1;
+	if (cache) {
+		free(cache);
+		cache = 0;
+	}
+}
diff --git a/include/part.h b/include/part.h
index 6d8f520..21f820f 100644
--- a/include/part.h
+++ b/include/part.h
@@ -369,4 +369,69 @@ int gpt_verify_partitions(struct blk_desc *dev_desc,
 			  gpt_header *gpt_head, gpt_entry **gpt_pte);
 #endif
 
+#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
+ *
+ * @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);
+
+/**
+ * cache_block_fill() - make data read from a block device available
+ * to the block 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 available
+ * @param blksz - size in bytes of each block
+ * @param buf - buffer containing data to cache
+ *
+ */
+void cache_block_fill
+	(int iftype, int dev,
+	 lbaint_t start, lbaint_t blkcnt,
+	 unsigned long blksz, void const *buffer);
+
+/**
+ * cache_block_invalidate() - discard the cache for a set of blocks
+ * because of a write or device (re)initialization.
+ *
+ * @param iftype - IF_TYPE_x for type of device
+ * @param dev - device index of particular type
+ */
+void cache_block_invalidate
+	(int iftype, int dev);
+
+#else
+
+static inline int cache_block_read
+	(int iftype, int dev,
+	 lbaint_t start, lbaint_t blkcnt,
+	 unsigned long blksz, void *buffer)
+{
+	return 0;
+}
+
+static inline void cache_block_fill
+	(int iftype, int dev,
+	 lbaint_t start, lbaint_t blkcnt,
+	 unsigned long blksz, void const *buffer) {}
+
+static inline void cache_block_invalidate
+	(int iftype, int dev) {}
+
+#endif
+
 #endif /* _PART_H */
-- 
2.6.2

[U-Boot] [RFC PATCH 2/2] mmc: add support for block device cache

[Eric Nelson]

Signed-off-by: Eric Nelson <eric@nelint.com>
---
 drivers/mmc/mmc.c       | 10 +++++++++-
 drivers/mmc/mmc_write.c |  7 +++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 8b2e606..956f4e1 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -6,7 +6,6 @@
  *
  * SPDX-License-Identifier:	GPL-2.0+
  */
-
 #include <config.h>
 #include <common.h>
 #include <command.h>
@@ -240,6 +239,8 @@ static ulong mmc_bread(struct blk_desc *block_dev, lbaint_t start,
 	int dev_num = block_dev->devnum;
 	int err;
 	lbaint_t cur, blocks_todo = blkcnt;
+	void *outbuf = dst;
+	lbaint_t outblk = start;
 
 	if (blkcnt == 0)
 		return 0;
@@ -260,6 +261,10 @@ static ulong mmc_bread(struct blk_desc *block_dev, lbaint_t start,
 		return 0;
 	}
 
+	if (cache_block_read(IF_TYPE_MMC, dev_num, start, blkcnt,
+			     mmc->read_bl_len, dst))
+		return blkcnt;
+
 	if (mmc_set_blocklen(mmc, mmc->read_bl_len)) {
 		debug("%s: Failed to set blocklen\n", __func__);
 		return 0;
@@ -277,6 +282,9 @@ static ulong mmc_bread(struct blk_desc *block_dev, lbaint_t start,
 		dst += cur * mmc->read_bl_len;
 	} while (blocks_todo > 0);
 
+	cache_block_fill(IF_TYPE_MMC, dev_num, outblk, blkcnt,
+			 mmc->read_bl_len, outbuf);
+
 	return blkcnt;
 }
 
diff --git a/drivers/mmc/mmc_write.c b/drivers/mmc/mmc_write.c
index 7b186f8..a877c78 100644
--- a/drivers/mmc/mmc_write.c
+++ b/drivers/mmc/mmc_write.c
@@ -12,6 +12,7 @@
 #include <part.h>
 #include <div64.h>
 #include <linux/math64.h>
+#include <part.h>
 #include "mmc_private.h"
 
 static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
@@ -20,6 +21,8 @@ static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
 	ulong end;
 	int err, start_cmd, end_cmd;
 
+	cache_block_invalidate(IF_TYPE_MMC, mmc->block_dev.dev);
+
 	if (mmc->high_capacity) {
 		end = start + blkcnt - 1;
 	} else {
@@ -82,6 +85,8 @@ unsigned long mmc_berase(struct blk_desc *block_dev, lbaint_t start,
 	if (err < 0)
 		return -1;
 
+	cache_block_invalidate(IF_TYPE_MMC, dev_num);
+
 	/*
 	 * We want to see if the requested start or total block count are
 	 * unaligned.  We discard the whole numbers and only care about the
@@ -186,6 +191,8 @@ ulong mmc_bwrite(struct blk_desc *block_dev, lbaint_t start, lbaint_t blkcnt,
 	if (err < 0)
 		return 0;
 
+	cache_block_invalidate(IF_TYPE_MMC, dev_num);
+
 	if (mmc_set_blocklen(mmc, mmc->write_bl_len))
 		return 0;
 
-- 
2.6.2

[U-Boot] [RFC PATCH 1/2] add block device cache

[Stephen Warren]

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.

[U-Boot] [RFC PATCH 2/2] mmc: add support for block device cache

[Stephen Warren]

On 03/16/2016 03:40 PM, Eric Nelson wrote:
> Signed-off-by: Eric Nelson <eric@nelint.com>

Patch description.

> ---
>   drivers/mmc/mmc.c       | 10 +++++++++-
>   drivers/mmc/mmc_write.c |  7 +++++++

Presumably it makes sense for the cache to work for IDE, SATA, USB, 
SCSI, ... too. I wonder if it's possible to put this code somewhere more 
central than mmc*.c so it automatically applies to 
dev_desc->block_read() (see include/part.h). Perhaps not since each 
implementation supplies its own block_read function directly, so the 
cache calls do need to be duplicated everywhere.

> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>    *
>    * SPDX-License-Identifier:	GPL-2.0+
>    */
> -
>   #include <config.h>

Nit: unrelated change.

I think there is a missing call to cache_block_invalidate() when the MMC 
device gets re-enumerated/re-initialized. The user would do something to 
trigger this (e.g. mmc rescan) when they'd swapped an SD card out for 
example.

Do you have any stats on how many operations this saves for typical FS 
operations such as:

- Partition table type identification (with various types such as 
MBR/DOS, GPT, ...)
- Partition enumeration
- Filesystem identification (with various filesystems such as FAT, ext, ...)
- File reads

[U-Boot] [RFC PATCH 1/2] add block device cache

[Eric Nelson]

Thanks for the review(s) Stephen.

On 03/17/2016 02:16 PM, Stephen Warren wrote:
> 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.
> 

Yeah. Please hote the RFC.

I was really hoping for some broader feedback about whether this
is a better approach than the more specialized ext4 extent cache.

If I can get an ack on the approach, I think a minimal block
device cache would support at least 2 or 4 entries, and I'd
need to be able to answer the questions from your other
response:

> Do you have any stats on how many operations this saves for typical FS operations such as:
> 
> - Partition table type identification (with various types such as MBR/DOS, GPT, ...)
> - Partition enumeration
> - Filesystem identification (with various filesystems such as FAT, ext, ...)
> - File reads 

Should I interpret this as support of a small(ish) block device cache?

Regards,


Eric

[U-Boot] [RFC PATCH 1/2] add block device cache

[Stephen Warren]

On 03/17/2016 03:33 PM, Eric Nelson wrote:
> Thanks for the review(s) Stephen.
>
> On 03/17/2016 02:16 PM, Stephen Warren wrote:
>> 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.
>>
>
> Yeah. Please hote the RFC.
>
> I was really hoping for some broader feedback about whether this
> is a better approach than the more specialized ext4 extent cache.
>
> If I can get an ack on the approach, I think a minimal block
> device cache would support at least 2 or 4 entries, and I'd
> need to be able to answer the questions from your other
> response:
>
>> Do you have any stats on how many operations this saves for typical FS operations such as:
>>
>> - Partition table type identification (with various types such as MBR/DOS, GPT, ...)
>> - Partition enumeration
>> - Filesystem identification (with various filesystems such as FAT, ext, ...)
>> - File reads
>
> Should I interpret this as support of a small(ish) block device cache?

Conceptually I think it sounds OK provided it yields some demonstrable 
improvement across a variety of use-cases, and the design doesn't 
prevent it addressing obvious other cases it should address.

You probably want to Cc (or hope they see this and provide feedback 
anyway) a few other people such as Tom Rini, Simon Glass, Marek Vasut, etc.

[U-Boot] ext4 and caching

[Ioan Nicu]

Hi Eric,

On Wed, Mar 16, 2016 at 11:42:55AM -0700, EXT Eric Nelson wrote:
> 	Hi all,
> 
> I've been seeing the same sort of issues repoted by Ionut
> and as addressed by this patch:
> 	http://lists.denx.de/pipermail/u-boot/2014-January/171459.html
> 
> That patch was added in commit fc0fc50 and reverted in commit 715b56f.
> 
> It no longer applies cleanly, and when I tried to resurrect it,
> I saw errors traversing directories and perhaps something went
> wrong with my merge.
> 
> Ionut, do you have a current version of this patch?
> 

We reverted that patch because it was breaking ext4 write support. I initially
developed the patch on top of an older u-boot which didn't have write supoort
at all.

I have tried to refactor my patch to work with both read/write, but I gave up
when I saw that the ext4 write code relied on the old behavior of
read_allocated_block/ext4fs_get_extent_block. I could have worked around that,
but then the whole thing would have looked like hack, so I didn't like it ...

If you are interested in a current version of this patch, I could try to
revive it. But it has the limitation I mentioned above, so I guess you could
use it just for some ext4 read performance measurement test.

Regards,
Ioan

[U-Boot] ext4 and caching

[Eric Nelson]

Thanks Ioan,

On 03/19/2016 08:42 AM, Ioan Nicu wrote:
> Hi Eric,
> 
> On Wed, Mar 16, 2016 at 11:42:55AM -0700, EXT Eric Nelson wrote:
>> 	Hi all,
>>
>> I've been seeing the same sort of issues repoted by Ionut
>> and as addressed by this patch:
>> 	http://lists.denx.de/pipermail/u-boot/2014-January/171459.html
>>
>> That patch was added in commit fc0fc50 and reverted in commit 715b56f.
>>
>> It no longer applies cleanly, and when I tried to resurrect it,
>> I saw errors traversing directories and perhaps something went
>> wrong with my merge.
>>
>> Ionut, do you have a current version of this patch?
>>
> 
> We reverted that patch because it was breaking ext4 write support. I initially
> developed the patch on top of an older u-boot which didn't have write supoort
> at all.
> 
> I have tried to refactor my patch to work with both read/write, but I gave up
> when I saw that the ext4 write code relied on the old behavior of
> read_allocated_block/ext4fs_get_extent_block. I could have worked around that,
> but then the whole thing would have looked like hack, so I didn't like it ...
> 
> If you are interested in a current version of this patch, I could try to
> revive it. But it has the limitation I mentioned above, so I guess you could
> use it just for some ext4 read performance measurement test.
> 

I do want to get rid of the performance problem, but as I suggested
by my patch set, I think that a more general-purpose cache is a better
approach.

Regards,


Eric

[U-Boot] [RFC PATCH 2/2] mmc: add support for block device cache

[Eric Nelson]

Hi Stephen,

On 03/17/2016 02:23 PM, Stephen Warren wrote:
> On 03/16/2016 03:40 PM, Eric Nelson wrote:
>> Signed-off-by: Eric Nelson <eric@nelint.com>
> 
> Patch description.
> 
>> ---
>>   drivers/mmc/mmc.c       | 10 +++++++++-
>>   drivers/mmc/mmc_write.c |  7 +++++++
> 
> Presumably it makes sense for the cache to work for IDE, SATA, USB,
> SCSI, ... too. I wonder if it's possible to put this code somewhere more
> central than mmc*.c so it automatically applies to
> dev_desc->block_read() (see include/part.h). Perhaps not since each
> implementation supplies its own block_read function directly, so the
> cache calls do need to be duplicated everywhere.
> 

Yeah. I haven't found a spot that would allow interception of
the various block_read/write functions.

The get_dev_hwpart() routine in disk/part.c is close, but it seems
to be bypassed by cmd_mmc, cmd_sata, et cetera.

I think the best that can be done is to provide a common shim that can
easily be inserted from within the various block driver code.

>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>    *
>>    * SPDX-License-Identifier:    GPL-2.0+
>>    */
>> -
>>   #include <config.h>
> 
> Nit: unrelated change.
> 
> I think there is a missing call to cache_block_invalidate() when the MMC
> device gets re-enumerated/re-initialized. The user would do something to
> trigger this (e.g. mmc rescan) when they'd swapped an SD card out for
> example.
> 

Good catch.

> Do you have any stats on how many operations this saves for typical FS
> operations such as:
> 
> - Partition table type identification (with various types such as
> MBR/DOS, GPT, ...)
> - Partition enumeration
> - Filesystem identification (with various filesystems such as FAT, ext,
> ...)
> - File reads
> 

Not yet, but I'm working something up that will allow this to be
gathered easily. As soon as we implement a cache, it provides a nice
spot for tracing operations.

Regards,


Eric

[U-Boot] [RFC PATCH 1/2] add block device cache

[Tom Rini]

On Thu, Mar 17, 2016 at 02:33:04PM -0700, Eric Nelson wrote:
> Thanks for the review(s) Stephen.
> 
> On 03/17/2016 02:16 PM, Stephen Warren wrote:
> > 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.
> > 
> 
> Yeah. Please hote the RFC.
> 
> I was really hoping for some broader feedback about whether this
> is a better approach than the more specialized ext4 extent cache.

Well, I guess it comes down to how hard it is to also re-use this on say
USB (another common case and one I assume you can test on your HW) since
all of EXT4 and FAT and MMC and USB are fairly common and we should be
able to address these with your approach, or at least be within "rework
some other stuff and then.." distance.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160320/9e106d8a/attachment.sig>

[U-Boot] [RFC PATCH 2/2] mmc: add support for block device cache

[Tom Rini]

On Sun, Mar 20, 2016 at 12:35:53PM -0700, Eric Nelson wrote:
> Hi Stephen,
> 
> On 03/17/2016 02:23 PM, Stephen Warren wrote:
> > On 03/16/2016 03:40 PM, Eric Nelson wrote:
> >> Signed-off-by: Eric Nelson <eric@nelint.com>
> > 
> > Patch description.
> > 
> >> ---
> >>   drivers/mmc/mmc.c       | 10 +++++++++-
> >>   drivers/mmc/mmc_write.c |  7 +++++++
> > 
> > Presumably it makes sense for the cache to work for IDE, SATA, USB,
> > SCSI, ... too. I wonder if it's possible to put this code somewhere more
> > central than mmc*.c so it automatically applies to
> > dev_desc->block_read() (see include/part.h). Perhaps not since each
> > implementation supplies its own block_read function directly, so the
> > cache calls do need to be duplicated everywhere.
> > 
> 
> Yeah. I haven't found a spot that would allow interception of
> the various block_read/write functions.

Shouldn't DM also help here?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160320/514e3f12/attachment.sig>

[U-Boot] [RFC PATCH 1/2] add block device cache

[Eric Nelson]

Thanks for the feedback Tom,

On 03/20/2016 03:13 PM, Tom Rini wrote:
> On Thu, Mar 17, 2016 at 02:33:04PM -0700, Eric Nelson wrote:
>> Thanks for the review(s) Stephen.
>>
>> On 03/17/2016 02:16 PM, Stephen Warren wrote:
>>> 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.
>>>
>>
>> Yeah. Please hote the RFC.
>>
>> I was really hoping for some broader feedback about whether this
>> is a better approach than the more specialized ext4 extent cache.
> 
> Well, I guess it comes down to how hard it is to also re-use this on say
> USB (another common case and one I assume you can test on your HW) since
> all of EXT4 and FAT and MMC and USB are fairly common and we should be
> able to address these with your approach, or at least be within "rework
> some other stuff and then.." distance.
> 

It should be trivial to include this support for USB, SATA, IDE, or
any other block device, but not transparent.

I'm close to a V2 RFC patch so people can throw stones at an actual
implementation.

Regards,


Eric

[U-Boot] [RFC PATCH 2/2] mmc: add support for block device cache

[Eric Nelson]

Hi Tom,

On 03/20/2016 03:13 PM, Tom Rini wrote:
> On Sun, Mar 20, 2016 at 12:35:53PM -0700, Eric Nelson wrote:
>> Hi Stephen,
>>
>> On 03/17/2016 02:23 PM, Stephen Warren wrote:
>>> On 03/16/2016 03:40 PM, Eric Nelson wrote:
>>>> Signed-off-by: Eric Nelson <eric@nelint.com>
>>>
>>> Patch description.
>>>
>>>> ---
>>>>   drivers/mmc/mmc.c       | 10 +++++++++-
>>>>   drivers/mmc/mmc_write.c |  7 +++++++
>>>
>>> Presumably it makes sense for the cache to work for IDE, SATA, USB,
>>> SCSI, ... too. I wonder if it's possible to put this code somewhere more
>>> central than mmc*.c so it automatically applies to
>>> dev_desc->block_read() (see include/part.h). Perhaps not since each
>>> implementation supplies its own block_read function directly, so the
>>> cache calls do need to be duplicated everywhere.
>>>
>>
>> Yeah. I haven't found a spot that would allow interception of
>> the various block_read/write functions.
> 
> Shouldn't DM also help here?
> 

I haven't yet looked, but this may be true.

I'm seeing some build breakage on master surrounding the use
of DM though.

If I select DM and BLK on top of nitrogen6q_defconfig, I get
lots of build errors.

I want to get a V2 RFC patch out before digging through the
details of that.

Regards,


Eric

[U-Boot] [RFC PATCH 2/2] mmc: add support for block device cache

[Eric Nelson]

Hi Stephen,

On 03/20/2016 12:35 PM, Eric Nelson wrote:
> On 03/17/2016 02:23 PM, Stephen Warren wrote:
>> On 03/16/2016 03:40 PM, Eric Nelson wrote:
...
>> Do you have any stats on how many operations this saves for typical FS
>> operations such as:
>>
>> - Partition table type identification (with various types such as
>> MBR/DOS, GPT, ...)
>> - Partition enumeration
>> - Filesystem identification (with various filesystems such as FAT, ext,
>> ...)
>> - File reads
>>
> 
> Not yet, but I'm working something up that will allow this to be
> gathered easily. As soon as we implement a cache, it provides a nice
> spot for tracing operations.
> 

The V2 RFC patch set implements a command (blkcache) that allows
tracing block read operations as they pass through the cache, among
other things:

=> blkcache trace
=> load mmc 0 10008000 /zImage
miss: start 0, count 1
fill: start 0, count 1
miss: start 2000, count 1
fill: start 2000, count 1
reading /zImage
hit: start 2000, count 1
miss: start 2011, count 2
miss: start 2001, count 6
miss: start 2031, count 9678
miss: start 45ff, count 1
fill: start 45ff, count 1
4955304 bytes read in 258 ms (18.3 MiB/s)

http://lists.denx.de/pipermail/u-boot/2016-March/249155.html
https://patchwork.ozlabs.org/patch/599942/

[U-Boot] [RFC PATCH 2/2] mmc: add support for block device cache

[Eric Nelson]

Hi Tom,

On 03/20/2016 03:54 PM, Eric Nelson wrote:
> On 03/20/2016 03:13 PM, Tom Rini wrote:
>> On Sun, Mar 20, 2016 at 12:35:53PM -0700, Eric Nelson wrote:
>>> On 03/17/2016 02:23 PM, Stephen Warren wrote:
>>>> On 03/16/2016 03:40 PM, Eric Nelson wrote:
>>>>> Signed-off-by: Eric Nelson <eric@nelint.com>
>>>>
>>>> Patch description.
>>>>
>>>>> ---
>>>>>   drivers/mmc/mmc.c       | 10 +++++++++-
>>>>>   drivers/mmc/mmc_write.c |  7 +++++++
>>>>
>>>> Presumably it makes sense for the cache to work for IDE, SATA, USB,
>>>> SCSI, ... too. I wonder if it's possible to put this code somewhere more
>>>> central than mmc*.c so it automatically applies to
>>>> dev_desc->block_read() (see include/part.h). Perhaps not since each
>>>> implementation supplies its own block_read function directly, so the
>>>> cache calls do need to be duplicated everywhere.
>>>>
>>>
>>> Yeah. I haven't found a spot that would allow interception of
>>> the various block_read/write functions.
>>
>> Shouldn't DM also help here?
>>
> 
> I haven't yet looked, but this may be true.
> 
> I'm seeing some build breakage on master surrounding the use
> of DM though.
> 
> If I select DM and BLK on top of nitrogen6q_defconfig, I get
> lots of build errors.
> 
> I want to get a V2 RFC patch out before digging through the
> details of that.
> 

I'm obviously not up to speed on the state of DM and I hadn't
seen Simon's patch adding blk.h.

The new blk_dread/write/erase functions do provide a convenient
spot for checking cache, though they're not universally used yet.

In particular, hooking up the cache there will lose visibility
into things like the "mmc write" command.

I'm also not sure of the current state of DM with respect to
block drivers and wonder if a block cache should wait a cycle
or two.

Simon, I'd appreciate some feedback when you have a chance.

Regards,


Eric

[U-Boot] [RFC PATCH 2/2] mmc: add support for block device cache

[Eric Nelson]

Hi all,

On 03/21/2016 11:31 AM, Eric Nelson wrote:
> On 03/20/2016 03:54 PM, Eric Nelson wrote:
>> On 03/20/2016 03:13 PM, Tom Rini wrote:
>>> On Sun, Mar 20, 2016 at 12:35:53PM -0700, Eric Nelson wrote:
>>>> On 03/17/2016 02:23 PM, Stephen Warren wrote:
>>>>> On 03/16/2016 03:40 PM, Eric Nelson wrote:
>>>>>> Signed-off-by: Eric Nelson <eric@nelint.com>
>>>>>
...

>> I'm seeing some build breakage on master surrounding the use
>> of DM though.
>>

Peng's patch made it clear that DM wasn't supported by fsl_esdhc.

>> If I select DM and BLK on top of nitrogen6q_defconfig, I get
>> lots of build errors.
>>

It's CONFIG_BLK that produces lots of issues, and from what
I can tell, it's only currently supported for sandbox.

Out of ignorance, I was conflating the two.

>> I want to get a V2 RFC patch out before digging through the
>> details of that.
>>
> 
> I'm obviously not up to speed on the state of DM and I hadn't
> seen Simon's patch adding blk.h.
> 
> The new blk_dread/write/erase functions do provide a convenient
> spot for checking cache, though they're not universally used yet.
> 
> In particular, hooking up the cache there will lose visibility
> into things like the "mmc write" command.
> 
> I'm also not sure of the current state of DM with respect to
> block drivers and wonder if a block cache should wait a cycle
> or two.
> 
> Simon, I'd appreciate some feedback when you have a chance.
> 

I think I have a better handle on this now.

I'm still a bit confused on what needs to be done in order
for CONFIG_BLK to work against real hardware.

From what I can tell, the some modules in cmd/ need to be
updated to use blk_dread/blk_dwrite/blk_derase and some kind
of re-structuring needs to occur in drivers/mmc to support
the "blk" uclass.

Does that sound about right?

Is somebody currently working on this?

Please advise,


Eric

[U-Boot] [RFC PATCH 2/2] mmc: add support for block device cache

[Simon Glass]

Hi Eric,

On 20 March 2016 at 16:54, Eric Nelson <eric@nelint.com> wrote:
> Hi Tom,
>
> On 03/20/2016 03:13 PM, Tom Rini wrote:
>> On Sun, Mar 20, 2016 at 12:35:53PM -0700, Eric Nelson wrote:
>>> Hi Stephen,
>>>
>>> On 03/17/2016 02:23 PM, Stephen Warren wrote:
>>>> On 03/16/2016 03:40 PM, Eric Nelson wrote:
>>>>> Signed-off-by: Eric Nelson <eric@nelint.com>
>>>>
>>>> Patch description.
>>>>
>>>>> ---
>>>>>   drivers/mmc/mmc.c       | 10 +++++++++-
>>>>>   drivers/mmc/mmc_write.c |  7 +++++++
>>>>
>>>> Presumably it makes sense for the cache to work for IDE, SATA, USB,
>>>> SCSI, ... too. I wonder if it's possible to put this code somewhere more
>>>> central than mmc*.c so it automatically applies to
>>>> dev_desc->block_read() (see include/part.h). Perhaps not since each
>>>> implementation supplies its own block_read function directly, so the
>>>> cache calls do need to be duplicated everywhere.
>>>>
>>>
>>> Yeah. I haven't found a spot that would allow interception of
>>> the various block_read/write functions.
>>
>> Shouldn't DM also help here?
>>
>
> I haven't yet looked, but this may be true.
>
> I'm seeing some build breakage on master surrounding the use
> of DM though.
>
> If I select DM and BLK on top of nitrogen6q_defconfig, I get
> lots of build errors.
>
> I want to get a V2 RFC patch out before digging through the
> details of that.

I'm about to send out a series that rationalises the block devices a
bit. In the meantime, see u-boot-dm/blkb-working for some MMC ideas.

Regards,
Simon

[U-Boot] [RFC PATCH 2/2] mmc: add support for block device cache

[Eric Nelson]

Hi Simon,

On 04/09/2016 10:55 AM, Simon Glass wrote:
> On 20 March 2016 at 16:54, Eric Nelson <eric@nelint.com> wrote:
>> On 03/20/2016 03:13 PM, Tom Rini wrote:
>>> On Sun, Mar 20, 2016 at 12:35:53PM -0700, Eric Nelson wrote:
>>>> On 03/17/2016 02:23 PM, Stephen Warren wrote:
>>>>> On 03/16/2016 03:40 PM, Eric Nelson wrote:
>>>>>> Signed-off-by: Eric Nelson <eric@nelint.com>
>>>>>
>>>>> Patch description.
>>>>>
>>>>>> ---
>>>>>>   drivers/mmc/mmc.c       | 10 +++++++++-
>>>>>>   drivers/mmc/mmc_write.c |  7 +++++++
>>>>>
>>>>> Presumably it makes sense for the cache to work for IDE, SATA, USB,
>>>>> SCSI, ... too. I wonder if it's possible to put this code somewhere more
>>>>> central than mmc*.c so it automatically applies to
>>>>> dev_desc->block_read() (see include/part.h). Perhaps not since each
>>>>> implementation supplies its own block_read function directly, so the
>>>>> cache calls do need to be duplicated everywhere.
>>>>>
>>>>
>>>> Yeah. I haven't found a spot that would allow interception of
>>>> the various block_read/write functions.
>>>
>>> Shouldn't DM also help here?
>>
>> I haven't yet looked, but this may be true.
>>
>> I'm seeing some build breakage on master surrounding the use
>> of DM though.
>>
>> If I select DM and BLK on top of nitrogen6q_defconfig, I get
>> lots of build errors.
>>
>> I want to get a V2 RFC patch out before digging through the
>> details of that.
> 
> I'm about to send out a series that rationalises the block devices a
> bit. In the meantime, see u-boot-dm/blkb-working for some MMC ideas.
> 

I figured things out and sent V2 and V3 versions of these patches.

Tom applied V3 to master, though I do still have three patches to
address Stephen's review pending:

http://lists.denx.de/pipermail/u-boot/2016-April/thread.html#250331

https://patchwork.ozlabs.org/patch/605421/
https://patchwork.ozlabs.org/patch/605420/
https://patchwork.ozlabs.org/patch/605422/

Regards,


Eric