ext4 and caching

U-Boot Superseded Eric Nelson 5 participants 20 messages Mar 16, 2016 – Apr 10, 2016

Subsystems (2)

Block layer MMC, SD and SDIO

lore

Share & tools

lore

Eric Nelson

author

2016-03-16 18:42 UTC #0 lore raw

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

author

2016-03-16 21:40 UTC #1 lore raw

cover simple cache layer for block devices

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

Signature · 1 line

2.6.2

Eric Nelson

author

2016-03-16 21:40 UTC #2 lore raw

RFC PATCH 1/2 add block device cache

Signed-off-by Eric Nelson <[email protected]>

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

drivers/block/Makefile +1 -0

index b5c7ae1..056a48b 100644
@@ -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

drivers/block/cache_block.c new file +76 -0

76 lines not on this page · raw message

include/part.h +65 -0

69 lines not on this page · raw message

Signature · 1 line

2.6.2

Stephen Warren

2016-03-17 21:16 UTC #3 lore raw

RFC PATCH 1/2 add block device cache

Quoted 1 line · Eric Nelson

Signed-off-by Eric Nelson <[email protected]>

A patch description would be useful here; the cover letter wouldn't be checked in.

drivers/block/cache_block.c line 20

Quoted 8 lines

drivers/block/cache_block.c +6 -0

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

drivers/block/cache_block.c line 31

Quoted 10 lines

drivers/block/cache_block.c +10 -0

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

drivers/block/cache_block.c line 32

Quoted 1 line

drivers/block/cache_block.c +1 -0
+		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?

Quoted 7 lines

drivers/block/cache_block.c +6 -0

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

drivers/block/cache_block.c line 60

Quoted 10 lines

drivers/block/cache_block.c +10 -0

+			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?

drivers/block/cache_block.c line 71

Quoted 3 lines

drivers/block/cache_block.c +3 -0

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

Quoted 8 lines

drivers/block/cache_block.c +5 -0
+	if (cache) {
+		free(cache);
+		cache = 0;
+	}
+}
include/part.h +0 -0
 

include/part.h int gpt_verify_partitions(struct blk_desc *dev_desc, · line 381

Quoted 10 lines

include/part.h +10 -0

+#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/?

include/part.h int gpt_verify_partitions(struct blk_desc *dev_desc, · line 388

Quoted 7 lines

+ *
+ * @return - '1' if block returned from cache, '0' otherwise.
+ */
include/part.h +4 -0
+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.

Eric Nelson

author

2016-03-17 21:33 UTC #4 lore raw

RFC PATCH 1/2 add block device cache

Thanks for the review(s) Stephen.

Quoted 6 lines · Stephen Warren

Quoted 1 line · Eric Nelson
Signed-off-by Eric Nelson <[email protected]>
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:

Quoted 6 lines

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

Stephen Warren

2016-03-17 21:41 UTC #5 lore raw

RFC PATCH 1/2 add block device cache

Quoted 20 lines · Eric Nelson

Thanks for the review(s) Stephen.
Quoted 6 lines · Stephen Warren
Quoted 1 line · Eric Nelson
Signed-off-by Eric Nelson <[email protected]>
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:

Quoted 8 lines

Quoted 6 lines
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.

Tom Rini

2016-03-20 22:13 UTC #6 lore raw

RFC PATCH 1/2 add block device cache

In reply to Eric Nelson

Quoted 6 lines · Eric Nelson

Thanks for the review(s) Stephen.
Quoted 3 lines · Stephen Warren
Quoted 1 line · Eric Nelson
Signed-off-by Eric Nelson <[email protected]>

Quoted 8 lines

Quoted 3 lines
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.

Signature · 8 lines

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>

Eric Nelson

author

2016-03-20 22:51 UTC #7 lore raw

RFC PATCH 1/2 add block device cache

Thanks for the feedback Tom,

Quoted 13 lines · Tom Rini

Quoted 12 lines · Eric Nelson
Thanks for the review(s) Stephen.
Quoted 6 lines · Stephen Warren
Quoted 1 line · Eric Nelson
Signed-off-by Eric Nelson <[email protected]>
A patch description would be useful here; the cover letter wouldn't be checked in.
Yeah. Please hote the RFC.

Quoted 9 lines

Quoted 2 lines
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

Eric Nelson

author

2016-03-16 21:40 UTC #8 lore raw

RFC PATCH 2/2 mmc: add support for block device cache

In reply to Eric Nelson

Signed-off-by Eric Nelson <[email protected]>

---
 drivers/mmc/mmc.c       | 10 +++++++++-
 drivers/mmc/mmc_write.c |  7 +++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

drivers/mmc/mmc.c +9 -1

index 8b2e606..956f4e1 100644
@@ -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;
 }

drivers/mmc/mmc_write.c +7 -0

index 7b186f8..a877c78 100644
@@ -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;

Signature · 1 line

2.6.2

Stephen Warren

2016-03-17 21:23 UTC #9 lore raw

RFC PATCH 2/2 mmc: add support for block device cache

Quoted 1 line · Eric Nelson

Signed-off-by Eric Nelson <[email protected]>

Patch description.

Quoted 3 lines

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

drivers/mmc/mmc.c line 9

Quoted 6 lines

drivers/mmc/mmc.c +0 -1

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

Eric Nelson

author

2016-03-20 19:35 UTC #10 lore raw

RFC PATCH 2/2 mmc: add support for block device cache

Hi Stephen,

Quoted 9 lines · Stephen Warren

Quoted 1 line · Eric Nelson
Signed-off-by Eric Nelson <[email protected]>
Patch description.
Quoted 3 lines
---
  drivers/mmc/mmc.c       | 10 +++++++++-
  drivers/mmc/mmc_write.c |  7 +++++++

Quoted 7 lines

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.

Quoted 9 lines

drivers/mmc/mmc.c line 9
Quoted 6 lines
drivers/mmc/mmc.c +0 -1
   *
   * SPDX-License-Identifier:    GPL-2.0+
   */
-
  #include <config.h>
Nit: unrelated change.

Quoted 5 lines

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.

Quoted 10 lines

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

Tom Rini

2016-03-20 22:13 UTC #11 lore raw

RFC PATCH 2/2 mmc: add support for block device cache

Quoted 20 lines · Eric Nelson

Hi Stephen,
Quoted 9 lines · Stephen Warren
Quoted 1 line · Eric Nelson
Signed-off-by Eric Nelson <[email protected]>
Patch description.
Quoted 3 lines
---
  drivers/mmc/mmc.c       | 10 +++++++++-
  drivers/mmc/mmc_write.c |  7 +++++++
Quoted 7 lines
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.

Quoted 2 lines

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

Shouldn't DM also help here?

Signature · 8 lines

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>

Eric Nelson

author

2016-03-20 22:54 UTC #12 lore raw

RFC PATCH 2/2 mmc: add support for block device cache

Hi Tom,

Quoted 21 lines · Tom Rini

Quoted 20 lines · Eric Nelson
Hi Stephen,
Quoted 16 lines · Stephen Warren
Quoted 1 line · Eric Nelson
Signed-off-by Eric Nelson <[email protected]>
Patch description.
Quoted 3 lines
---
  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.

Quoted 5 lines

Quoted 2 lines
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

Eric Nelson

author

2016-03-21 18:31 UTC #13 lore raw

RFC PATCH 2/2 mmc: add support for block device cache

Hi Tom,

Quoted 28 lines · Eric Nelson

Quoted 19 lines · Tom Rini
Quoted 18 lines · Eric Nelson
Quoted 16 lines · Stephen Warren
Quoted 1 line · Eric Nelson
Signed-off-by Eric Nelson <[email protected]>
Patch description.
Quoted 3 lines
---
  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.
Quoted 5 lines
Quoted 2 lines
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.

Quoted 9 lines

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

Eric Nelson

author

2016-03-26 00:11 UTC #14 lore raw

RFC PATCH 2/2 mmc: add support for block device cache

Hi all,

Quoted 7 lines · Eric Nelson

Quoted 6 lines · Eric Nelson
Quoted 5 lines · Tom Rini
Quoted 4 lines · Eric Nelson
Quoted 3 lines · Stephen Warren
Quoted 1 line · Eric Nelson
Signed-off-by Eric Nelson <[email protected]>

...

Quoted 3 lines

Quoted 3 lines
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.

Quoted 3 lines

Quoted 3 lines
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.

Quoted 10 lines

Quoted 3 lines
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.

Quoted 9 lines

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

Simon Glass

2016-04-09 17:55 UTC #15 lore raw

RFC PATCH 2/2 mmc: add support for block device cache

In reply to Eric Nelson

Hi Eric,

Quoted 32 lines · Eric Nelson

Hi Tom,
Quoted 21 lines · Tom Rini
Quoted 20 lines · Eric Nelson
Hi Stephen,
Quoted 16 lines · Stephen Warren
Quoted 1 line · Eric Nelson
Signed-off-by Eric Nelson <[email protected]>
Patch description.
Quoted 3 lines
---
  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.
Quoted 5 lines
Quoted 2 lines
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.

Quoted 8 lines

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

Eric Nelson

author

2016-04-10 14:31 UTC #16 lore raw

RFC PATCH 2/2 mmc: add support for block device cache

Hi Simon,

Quoted 31 lines · Simon Glass

Quoted 30 lines · Eric Nelson
Quoted 19 lines · Tom Rini
Quoted 18 lines · Eric Nelson
Quoted 16 lines · Stephen Warren
Quoted 1 line · Eric Nelson
Signed-off-by Eric Nelson <[email protected]>
Patch description.
Quoted 3 lines
---
  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.
Quoted 4 lines
Quoted 2 lines
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.

Quoted 9 lines

Quoted 5 lines
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

Eric Nelson

author

2016-03-21 14:27 UTC #17 lore raw

RFC PATCH 2/2 mmc: add support for block device cache

In reply to Eric Nelson

Hi Stephen,

Quoted 2 lines · Eric Nelson

Quoted 1 line · Stephen Warren
On 03/16/2016 03:40 PM, Eric Nelson wrote:

...

Quoted 11 lines

Quoted 10 lines
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

Quoted 4 lines

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/

Ioan Nicu

2016-03-19 15:42 UTC #18 lore raw

In reply to Eric Nelson

Hi Eric,

Quoted 6 lines · EXT 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

Quoted 8 lines

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

Eric Nelson

author

2016-03-20 15:02 UTC #19 lore raw

Thanks Ioan,

Quoted 22 lines · Ioan Nicu

Hi Eric,
Quoted 6 lines · EXT 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
Quoted 8 lines
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.

Quoted 9 lines

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