[U-Boot] [RFC][PATCH] block: Enable block cache by default

[U-Boot] [RFC][PATCH] block: Enable block cache by default

[Marek Vasut]

The recent ext4 cache discussion would indicate that the block cache
is a desired feature, yet hidden and not enabled most of the time.
Enable the block cache by default since it provides significant block
device access performance improvement and if there are some users who
cannot enable it ie. due to size limitations, those should disable it
explicitly in their board config.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Tom Rini <trini@konsulko.com>
---
 drivers/block/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 15fd1bcb2b..832d626bd4 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -29,7 +29,7 @@ config SPL_BLK
 
 config BLOCK_CACHE
 	bool "Use block device cache"
-	default n
+	default y
 	help
 	  This option enables a disk-block cache for all block devices.
 	  This is most useful when accessing filesystems under U-Boot since
-- 
2.16.2

[U-Boot] [RFC][PATCH] block: Enable block cache by default

[Simon Glass]

On 18 May 2018 at 03:22, Marek Vasut <marex@denx.de> wrote:
>
> The recent ext4 cache discussion would indicate that the block cache
> is a desired feature, yet hidden and not enabled most of the time.
> Enable the block cache by default since it provides significant block
> device access performance improvement and if there are some users who
> cannot enable it ie. due to size limitations, those should disable it
> explicitly in their board config.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  drivers/block/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [RFC][PATCH] block: Enable block cache by default

[Marek Vasut]

On 05/19/2018 04:36 PM, Simon Glass wrote:
> On 18 May 2018 at 03:22, Marek Vasut <marex@denx.de> wrote:
>>
>> The recent ext4 cache discussion would indicate that the block cache
>> is a desired feature, yet hidden and not enabled most of the time.
>> Enable the block cache by default since it provides significant block
>> device access performance improvement and if there are some users who
>> cannot enable it ie. due to size limitations, those should disable it
>> explicitly in their board config.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Michal Simek <michal.simek@xilinx.com>
>> Cc: Tom Rini <trini@konsulko.com>
>> ---
>>  drivers/block/Kconfig | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
I was hoping to get some feedback ?

-- 
Best regards,
Marek Vasut

[U-Boot] [RFC][PATCH] block: Enable block cache by default

[Tom Rini]

On Sat, May 19, 2018 at 08:20:30PM +0200, Marek Vasut wrote:
> On 05/19/2018 04:36 PM, Simon Glass wrote:
> > On 18 May 2018 at 03:22, Marek Vasut <marex@denx.de> wrote:
> >>
> >> The recent ext4 cache discussion would indicate that the block cache
> >> is a desired feature, yet hidden and not enabled most of the time.
> >> Enable the block cache by default since it provides significant block
> >> device access performance improvement and if there are some users who
> >> cannot enable it ie. due to size limitations, those should disable it
> >> explicitly in their board config.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Simon Glass <sjg@chromium.org>
> >> Cc: Michal Simek <michal.simek@xilinx.com>
> >> Cc: Tom Rini <trini@konsulko.com>
> >> ---
> >>  drivers/block/Kconfig | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> I was hoping to get some feedback ?

So, as I was asking on IRC, can you show the code paths where this gets
used outside of CONFIG_BLK and then really ext4/fat/btrfs as I do in my
patch?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180519/0b9220cf/attachment.sig>

[U-Boot] [RFC][PATCH] block: Enable block cache by default

[Marek Vasut]

On 05/19/2018 10:50 PM, Tom Rini wrote:
> On Sat, May 19, 2018 at 08:20:30PM +0200, Marek Vasut wrote:
>> On 05/19/2018 04:36 PM, Simon Glass wrote:
>>> On 18 May 2018 at 03:22, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> The recent ext4 cache discussion would indicate that the block cache
>>>> is a desired feature, yet hidden and not enabled most of the time.
>>>> Enable the block cache by default since it provides significant block
>>>> device access performance improvement and if there are some users who
>>>> cannot enable it ie. due to size limitations, those should disable it
>>>> explicitly in their board config.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>> Cc: Tom Rini <trini@konsulko.com>
>>>> ---
>>>>  drivers/block/Kconfig | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> I was hoping to get some feedback ?
> 
> So, as I was asking on IRC, can you show the code paths where this gets
> used outside of CONFIG_BLK and then really ext4/fat/btrfs as I do in my
> patch?

Can you summarize that discussion for everyone who was not on IRC at
that point ?

-- 
Best regards,
Marek Vasut

[U-Boot] [RFC][PATCH] block: Enable block cache by default

[Tom Rini]

On Sat, May 19, 2018 at 11:39:38PM +0200, Marek Vasut wrote:
> On 05/19/2018 10:50 PM, Tom Rini wrote:
> > On Sat, May 19, 2018 at 08:20:30PM +0200, Marek Vasut wrote:
> >> On 05/19/2018 04:36 PM, Simon Glass wrote:
> >>> On 18 May 2018 at 03:22, Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> The recent ext4 cache discussion would indicate that the block cache
> >>>> is a desired feature, yet hidden and not enabled most of the time.
> >>>> Enable the block cache by default since it provides significant block
> >>>> device access performance improvement and if there are some users who
> >>>> cannot enable it ie. due to size limitations, those should disable it
> >>>> explicitly in their board config.
> >>>>
> >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>> Cc: Simon Glass <sjg@chromium.org>
> >>>> Cc: Michal Simek <michal.simek@xilinx.com>
> >>>> Cc: Tom Rini <trini@konsulko.com>
> >>>> ---
> >>>>  drivers/block/Kconfig | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>
> >> I was hoping to get some feedback ?
> > 
> > So, as I was asking on IRC, can you show the code paths where this gets
> > used outside of CONFIG_BLK and then really ext4/fat/btrfs as I do in my
> > patch?
> 
> Can you summarize that discussion for everyone who was not on IRC at
> that point ?

So I posted https://patchwork.ozlabs.org/patch/913266/ and it depends on
BLK, as without BLK it does compile but isn't useful, but also isn't
wholly discarded (due to the invalidate call in disk/part.c).  AFIACT
from a quick read of the code, block cache is only useful on filesystems
that reside on block devices.  It won't help with "just" MMC reads for
example.  So we should only enable it by default in the case of
filesystems that are usually on block devices being enabled.

But I didn't dig through the code hard enough to see if it would be
useful on say UBIFS or if I'm wrong about it not being in the code path
of things like say NAND.

But I also don't think just default y in all cases is right as that's
adding non-trivial mounts of code on all of the platforms that don't /
won't make use of it.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180519/05771c57/attachment.sig>

[U-Boot] [RFC][PATCH] block: Enable block cache by default

[Marek Vasut]

On 05/20/2018 12:29 AM, Tom Rini wrote:
> On Sat, May 19, 2018 at 11:39:38PM +0200, Marek Vasut wrote:
>> On 05/19/2018 10:50 PM, Tom Rini wrote:
>>> On Sat, May 19, 2018 at 08:20:30PM +0200, Marek Vasut wrote:
>>>> On 05/19/2018 04:36 PM, Simon Glass wrote:
>>>>> On 18 May 2018 at 03:22, Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> The recent ext4 cache discussion would indicate that the block cache
>>>>>> is a desired feature, yet hidden and not enabled most of the time.
>>>>>> Enable the block cache by default since it provides significant block
>>>>>> device access performance improvement and if there are some users who
>>>>>> cannot enable it ie. due to size limitations, those should disable it
>>>>>> explicitly in their board config.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>> ---
>>>>>>  drivers/block/Kconfig | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>
>>>> I was hoping to get some feedback ?
>>>
>>> So, as I was asking on IRC, can you show the code paths where this gets
>>> used outside of CONFIG_BLK and then really ext4/fat/btrfs as I do in my
>>> patch?
>>
>> Can you summarize that discussion for everyone who was not on IRC at
>> that point ?
> 
> So I posted https://patchwork.ozlabs.org/patch/913266/ and it depends on
> BLK, as without BLK it does compile but isn't useful, but also isn't
> wholly discarded (due to the invalidate call in disk/part.c).

Maybe that is what needs fixing and then this patch can be applied ?

> AFIACT
> from a quick read of the code, block cache is only useful on filesystems
> that reside on block devices.  It won't help with "just" MMC reads for
> example.  So we should only enable it by default in the case of
> filesystems that are usually on block devices being enabled.

I wonder if this not helping with raw block reads is fine or not.
Thoughts ?

> But I didn't dig through the code hard enough to see if it would be
> useful on say UBIFS or if I'm wrong about it not being in the code path
> of things like say NAND.

I don't see why this won't be useful on UBI/UBIFS . It is probably just
not implemented yet.

> But I also don't think just default y in all cases is right as that's
> adding non-trivial mounts of code on all of the platforms that don't /
> won't make use of it.

So it should be discarded if there are no users ?

-- 
Best regards,
Marek Vasut

[U-Boot] [RFC][PATCH] block: Enable block cache by default

[Tom Rini]

On Sun, May 20, 2018 at 01:39:07AM +0200, Marek Vasut wrote:
> On 05/20/2018 12:29 AM, Tom Rini wrote:
> > On Sat, May 19, 2018 at 11:39:38PM +0200, Marek Vasut wrote:
> >> On 05/19/2018 10:50 PM, Tom Rini wrote:
> >>> On Sat, May 19, 2018 at 08:20:30PM +0200, Marek Vasut wrote:
> >>>> On 05/19/2018 04:36 PM, Simon Glass wrote:
> >>>>> On 18 May 2018 at 03:22, Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>> The recent ext4 cache discussion would indicate that the block cache
> >>>>>> is a desired feature, yet hidden and not enabled most of the time.
> >>>>>> Enable the block cache by default since it provides significant block
> >>>>>> device access performance improvement and if there are some users who
> >>>>>> cannot enable it ie. due to size limitations, those should disable it
> >>>>>> explicitly in their board config.
> >>>>>>
> >>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>> Cc: Simon Glass <sjg@chromium.org>
> >>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
> >>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>> ---
> >>>>>>  drivers/block/Kconfig | 2 +-
> >>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>>
> >>>> I was hoping to get some feedback ?
> >>>
> >>> So, as I was asking on IRC, can you show the code paths where this gets
> >>> used outside of CONFIG_BLK and then really ext4/fat/btrfs as I do in my
> >>> patch?
> >>
> >> Can you summarize that discussion for everyone who was not on IRC at
> >> that point ?
> > 
> > So I posted https://patchwork.ozlabs.org/patch/913266/ and it depends on
> > BLK, as without BLK it does compile but isn't useful, but also isn't
> > wholly discarded (due to the invalidate call in disk/part.c).
> 
> Maybe that is what needs fixing and then this patch can be applied ?

No, that's just the nature of the functionality.  We have an option for
block cache for the DM based block class.

> > AFIACT
> > from a quick read of the code, block cache is only useful on filesystems
> > that reside on block devices.  It won't help with "just" MMC reads for
> > example.  So we should only enable it by default in the case of
> > filesystems that are usually on block devices being enabled.
> 
> I wonder if this not helping with raw block reads is fine or not.
> Thoughts ?

So you got me to look at the code and it should be more widely enabled
that I suggested as it is used on block devices even on raw reads.

> > But I didn't dig through the code hard enough to see if it would be
> > useful on say UBIFS or if I'm wrong about it not being in the code path
> > of things like say NAND.
> 
> I don't see why this won't be useful on UBI/UBIFS . It is probably just
> not implemented yet.

It's not useful outside of "block" devices, of which NAND (and SPI and
NOR and so forth) are not.

> > But I also don't think just default y in all cases is right as that's
> > adding non-trivial mounts of code on all of the platforms that don't /
> > won't make use of it.
> 
> So it should be discarded if there are no users ?

Yes, it is.  Based on confirming it is used on raw block reads, I don't
think the cases I saw with growth, when we have BLK enabled, were a
problem.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180520/b07ecae6/attachment.sig>

[U-Boot] [RFC][PATCH] block: Enable block cache by default

[Marek Vasut]

On 05/21/2018 03:48 AM, Tom Rini wrote:
> On Sun, May 20, 2018 at 01:39:07AM +0200, Marek Vasut wrote:
>> On 05/20/2018 12:29 AM, Tom Rini wrote:
>>> On Sat, May 19, 2018 at 11:39:38PM +0200, Marek Vasut wrote:
>>>> On 05/19/2018 10:50 PM, Tom Rini wrote:
>>>>> On Sat, May 19, 2018 at 08:20:30PM +0200, Marek Vasut wrote:
>>>>>> On 05/19/2018 04:36 PM, Simon Glass wrote:
>>>>>>> On 18 May 2018 at 03:22, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> The recent ext4 cache discussion would indicate that the block cache
>>>>>>>> is a desired feature, yet hidden and not enabled most of the time.
>>>>>>>> Enable the block cache by default since it provides significant block
>>>>>>>> device access performance improvement and if there are some users who
>>>>>>>> cannot enable it ie. due to size limitations, those should disable it
>>>>>>>> explicitly in their board config.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>> ---
>>>>>>>>  drivers/block/Kconfig | 2 +-
>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>
>>>>>> I was hoping to get some feedback ?
>>>>>
>>>>> So, as I was asking on IRC, can you show the code paths where this gets
>>>>> used outside of CONFIG_BLK and then really ext4/fat/btrfs as I do in my
>>>>> patch?
>>>>
>>>> Can you summarize that discussion for everyone who was not on IRC at
>>>> that point ?
>>>
>>> So I posted https://patchwork.ozlabs.org/patch/913266/ and it depends on
>>> BLK, as without BLK it does compile but isn't useful, but also isn't
>>> wholly discarded (due to the invalidate call in disk/part.c).
>>
>> Maybe that is what needs fixing and then this patch can be applied ?
> 
> No, that's just the nature of the functionality.  We have an option for
> block cache for the DM based block class.

Then it should probably be enabled by default if block class is enabled ? :)

>>> AFIACT
>>> from a quick read of the code, block cache is only useful on filesystems
>>> that reside on block devices.  It won't help with "just" MMC reads for
>>> example.  So we should only enable it by default in the case of
>>> filesystems that are usually on block devices being enabled.
>>
>> I wonder if this not helping with raw block reads is fine or not.
>> Thoughts ?
> 
> So you got me to look at the code and it should be more widely enabled
> that I suggested as it is used on block devices even on raw reads.

Good!

>>> But I didn't dig through the code hard enough to see if it would be
>>> useful on say UBIFS or if I'm wrong about it not being in the code path
>>> of things like say NAND.
>>
>> I don't see why this won't be useful on UBI/UBIFS . It is probably just
>> not implemented yet.
> 
> It's not useful outside of "block" devices, of which NAND (and SPI and
> NOR and so forth) are not.

Don't we have mtdblock or somesuch ? (caching block access to MTD devices) ?

>>> But I also don't think just default y in all cases is right as that's
>>> adding non-trivial mounts of code on all of the platforms that don't /
>>> won't make use of it.
>>
>> So it should be discarded if there are no users ?
> 
> Yes, it is.  Based on confirming it is used on raw block reads, I don't
> think the cases I saw with growth, when we have BLK enabled, were a
> problem.

Errrr, can you rephrase ?

-- 
Best regards,
Marek Vasut