Re: [RFC PATCH v2 8/8] dax: Fix incorrect list of dcache aliasing architectures
On 2024-01-30 22:13, Dan Williams wrote: Dave Chinner wrote: On Tue, Jan 30, 2024 at 11:52:55AM -0500, Mathieu Desnoyers wrote: commit d92576f1167c ("dax: does not work correctly with virtual aliasing caches") prevents DAX from building on architectures with virtually aliased dcache with: depends on !(ARM || MIPS || SPARC) This check is too broad (e.g. recent ARMv7 don't have virtually aliased dcaches), and also misses many other architectures with virtually aliased dcache. This is a regression introduced in the v5.13 Linux kernel where the dax mount option is removed for 32-bit ARMv7 boards which have no dcache aliasing, and therefore should work fine with FS_DAX. This was turned into the following implementation of dax_is_supported() by a preparatory change: return !IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_MIPS) && !IS_ENABLED(CONFIG_SPARC); Use dcache_is_aliasing() instead to figure out whether the environment has aliasing dcaches. Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing caches") Signed-off-by: Mathieu Desnoyers Cc: Andrew Morton Cc: Linus Torvalds Cc: linux...@kvack.org Cc: linux-a...@vger.kernel.org Cc: Dan Williams Cc: Vishal Verma Cc: Dave Jiang Cc: Matthew Wilcox Cc: Arnd Bergmann Cc: Russell King Cc: nvd...@lists.linux.dev Cc: linux-...@vger.kernel.org Cc: linux-fsde...@vger.kernel.org --- include/linux/dax.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/include/linux/dax.h b/include/linux/dax.h index cfc8cd4a3eae..f59e604662e4 100644 --- a/include/linux/dax.h +++ b/include/linux/dax.h @@ -5,6 +5,7 @@ #include #include #include +#include typedef unsigned long dax_entry_t; @@ -80,9 +81,7 @@ static inline bool daxdev_mapping_supported(struct vm_area_struct *vma, } static inline bool dax_is_supported(void) { - return !IS_ENABLED(CONFIG_ARM) && - !IS_ENABLED(CONFIG_MIPS) && - !IS_ENABLED(CONFIG_SPARC); + return !dcache_is_aliasing(); Yeah, if this is just a one liner should go into fs_dax_get_by_bdev(), similar to the blk_queue_dax() check at the start of the function. I also noticed that device mapper uses fs_dax_get_by_bdev() to determine if it can support DAX, but this patch set does not address that case. Hence it really seems to me like fs_dax_get_by_bdev() is the right place to put this check. Oh, good catch. Yes, I agree this can definitely be pushed down, but then I think it should be pushed down all the way to make alloc_dax() fail. That will need some additional fixups like: diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 8dcabf84d866..a35e60e62440 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -2126,12 +2126,12 @@ static struct mapped_device *alloc_dev(int minor) md->dax_dev = alloc_dax(md, &dm_dax_ops); if (IS_ERR(md->dax_dev)) { md->dax_dev = NULL; - goto bad; + } else { + set_dax_nocache(md->dax_dev); + set_dax_nomc(md->dax_dev); + if (dax_add_host(md->dax_dev, md->disk)) + goto bad; } - set_dax_nocache(md->dax_dev); - set_dax_nomc(md->dax_dev); - if (dax_add_host(md->dax_dev, md->disk)) - goto bad; } format_dev_t(md->name, MKDEV(_major, minor)); ...to make it not fatal to fail to register the dax_dev. I've had a quick look at other users of alloc_dax() and alloc_dax_region(), and so far I figure that all of those really want to bail out on alloc_dax failure. Is dm.c the only special-case we need to fix to make it non-fatal ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com
Re: [RFC PATCH v2 8/8] dax: Fix incorrect list of dcache aliasing architectures
Dave Chinner wrote: > On Tue, Jan 30, 2024 at 11:52:55AM -0500, Mathieu Desnoyers wrote: > > commit d92576f1167c ("dax: does not work correctly with virtual aliasing > > caches") > > prevents DAX from building on architectures with virtually aliased > > dcache with: > > > > depends on !(ARM || MIPS || SPARC) > > > > This check is too broad (e.g. recent ARMv7 don't have virtually aliased > > dcaches), and also misses many other architectures with virtually > > aliased dcache. > > > > This is a regression introduced in the v5.13 Linux kernel where the > > dax mount option is removed for 32-bit ARMv7 boards which have no dcache > > aliasing, and therefore should work fine with FS_DAX. > > > > This was turned into the following implementation of dax_is_supported() > > by a preparatory change: > > > > return !IS_ENABLED(CONFIG_ARM) && > >!IS_ENABLED(CONFIG_MIPS) && > >!IS_ENABLED(CONFIG_SPARC); > > > > Use dcache_is_aliasing() instead to figure out whether the environment > > has aliasing dcaches. > > > > Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing > > caches") > > Signed-off-by: Mathieu Desnoyers > > Cc: Andrew Morton > > Cc: Linus Torvalds > > Cc: linux...@kvack.org > > Cc: linux-a...@vger.kernel.org > > Cc: Dan Williams > > Cc: Vishal Verma > > Cc: Dave Jiang > > Cc: Matthew Wilcox > > Cc: Arnd Bergmann > > Cc: Russell King > > Cc: nvd...@lists.linux.dev > > Cc: linux-...@vger.kernel.org > > Cc: linux-fsde...@vger.kernel.org > > --- > > include/linux/dax.h | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/dax.h b/include/linux/dax.h > > index cfc8cd4a3eae..f59e604662e4 100644 > > --- a/include/linux/dax.h > > +++ b/include/linux/dax.h > > @@ -5,6 +5,7 @@ > > #include > > #include > > #include > > +#include > > > > typedef unsigned long dax_entry_t; > > > > @@ -80,9 +81,7 @@ static inline bool daxdev_mapping_supported(struct > > vm_area_struct *vma, > > } > > static inline bool dax_is_supported(void) > > { > > - return !IS_ENABLED(CONFIG_ARM) && > > - !IS_ENABLED(CONFIG_MIPS) && > > - !IS_ENABLED(CONFIG_SPARC); > > + return !dcache_is_aliasing(); > > Yeah, if this is just a one liner should go into > fs_dax_get_by_bdev(), similar to the blk_queue_dax() check at the > start of the function. > > I also noticed that device mapper uses fs_dax_get_by_bdev() to > determine if it can support DAX, but this patch set does not address > that case. Hence it really seems to me like fs_dax_get_by_bdev() is > the right place to put this check. Oh, good catch. Yes, I agree this can definitely be pushed down, but then I think it should be pushed down all the way to make alloc_dax() fail. That will need some additional fixups like: diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 8dcabf84d866..a35e60e62440 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -2126,12 +2126,12 @@ static struct mapped_device *alloc_dev(int minor) md->dax_dev = alloc_dax(md, &dm_dax_ops); if (IS_ERR(md->dax_dev)) { md->dax_dev = NULL; - goto bad; + } else { + set_dax_nocache(md->dax_dev); + set_dax_nomc(md->dax_dev); + if (dax_add_host(md->dax_dev, md->disk)) + goto bad; } - set_dax_nocache(md->dax_dev); - set_dax_nomc(md->dax_dev); - if (dax_add_host(md->dax_dev, md->disk)) - goto bad; } format_dev_t(md->name, MKDEV(_major, minor)); ...to make it not fatal to fail to register the dax_dev.
Re: [RFC PATCH v2 8/8] dax: Fix incorrect list of dcache aliasing architectures
On Tue, Jan 30, 2024 at 11:52:55AM -0500, Mathieu Desnoyers wrote: > commit d92576f1167c ("dax: does not work correctly with virtual aliasing > caches") > prevents DAX from building on architectures with virtually aliased > dcache with: > > depends on !(ARM || MIPS || SPARC) > > This check is too broad (e.g. recent ARMv7 don't have virtually aliased > dcaches), and also misses many other architectures with virtually > aliased dcache. > > This is a regression introduced in the v5.13 Linux kernel where the > dax mount option is removed for 32-bit ARMv7 boards which have no dcache > aliasing, and therefore should work fine with FS_DAX. > > This was turned into the following implementation of dax_is_supported() > by a preparatory change: > > return !IS_ENABLED(CONFIG_ARM) && >!IS_ENABLED(CONFIG_MIPS) && >!IS_ENABLED(CONFIG_SPARC); > > Use dcache_is_aliasing() instead to figure out whether the environment > has aliasing dcaches. > > Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing > caches") > Signed-off-by: Mathieu Desnoyers > Cc: Andrew Morton > Cc: Linus Torvalds > Cc: linux...@kvack.org > Cc: linux-a...@vger.kernel.org > Cc: Dan Williams > Cc: Vishal Verma > Cc: Dave Jiang > Cc: Matthew Wilcox > Cc: Arnd Bergmann > Cc: Russell King > Cc: nvd...@lists.linux.dev > Cc: linux-...@vger.kernel.org > Cc: linux-fsde...@vger.kernel.org > --- > include/linux/dax.h | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/include/linux/dax.h b/include/linux/dax.h > index cfc8cd4a3eae..f59e604662e4 100644 > --- a/include/linux/dax.h > +++ b/include/linux/dax.h > @@ -5,6 +5,7 @@ > #include > #include > #include > +#include > > typedef unsigned long dax_entry_t; > > @@ -80,9 +81,7 @@ static inline bool daxdev_mapping_supported(struct > vm_area_struct *vma, > } > static inline bool dax_is_supported(void) > { > - return !IS_ENABLED(CONFIG_ARM) && > -!IS_ENABLED(CONFIG_MIPS) && > -!IS_ENABLED(CONFIG_SPARC); > + return !dcache_is_aliasing(); Yeah, if this is just a one liner should go into fs_dax_get_by_bdev(), similar to the blk_queue_dax() check at the start of the function. I also noticed that device mapper uses fs_dax_get_by_bdev() to determine if it can support DAX, but this patch set does not address that case. Hence it really seems to me like fs_dax_get_by_bdev() is the right place to put this check. -Dave. -- Dave Chinner da...@fromorbit.com
[RFC PATCH v2 8/8] dax: Fix incorrect list of dcache aliasing architectures
commit d92576f1167c ("dax: does not work correctly with virtual aliasing caches") prevents DAX from building on architectures with virtually aliased dcache with: depends on !(ARM || MIPS || SPARC) This check is too broad (e.g. recent ARMv7 don't have virtually aliased dcaches), and also misses many other architectures with virtually aliased dcache. This is a regression introduced in the v5.13 Linux kernel where the dax mount option is removed for 32-bit ARMv7 boards which have no dcache aliasing, and therefore should work fine with FS_DAX. This was turned into the following implementation of dax_is_supported() by a preparatory change: return !IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_MIPS) && !IS_ENABLED(CONFIG_SPARC); Use dcache_is_aliasing() instead to figure out whether the environment has aliasing dcaches. Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing caches") Signed-off-by: Mathieu Desnoyers Cc: Andrew Morton Cc: Linus Torvalds Cc: linux...@kvack.org Cc: linux-a...@vger.kernel.org Cc: Dan Williams Cc: Vishal Verma Cc: Dave Jiang Cc: Matthew Wilcox Cc: Arnd Bergmann Cc: Russell King Cc: nvd...@lists.linux.dev Cc: linux-...@vger.kernel.org Cc: linux-fsde...@vger.kernel.org --- include/linux/dax.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/include/linux/dax.h b/include/linux/dax.h index cfc8cd4a3eae..f59e604662e4 100644 --- a/include/linux/dax.h +++ b/include/linux/dax.h @@ -5,6 +5,7 @@ #include #include #include +#include typedef unsigned long dax_entry_t; @@ -80,9 +81,7 @@ static inline bool daxdev_mapping_supported(struct vm_area_struct *vma, } static inline bool dax_is_supported(void) { - return !IS_ENABLED(CONFIG_ARM) && - !IS_ENABLED(CONFIG_MIPS) && - !IS_ENABLED(CONFIG_SPARC); + return !dcache_is_aliasing(); } #else static inline void *dax_holder(struct dax_device *dax_dev) -- 2.39.2