| Message ID | 20180518092253.24255-1-marex@denx.de |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,RFC] block: Enable block cache by default | expand |
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>
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 ?
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?
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 ?
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.
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 ?
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.
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 ?
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
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(-)