| Message ID | 20240129210631.193493-1-mathieu.desnoyers@efficios.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce cache_is_aliasing() to fix DAX regression | expand |
Mathieu Desnoyers wrote: > This commit introduced in v5.13 prevents building FS_DAX on 32-bit ARM, > even on ARMv7 which does not have virtually aliased dcaches: > > commit d92576f1167c ("dax: does not work correctly with virtual aliasing caches") > > It used to work fine before: I have customers using dax over pmem on > ARMv7, but this regression will likely prevent them from upgrading their > kernel. > > The root of the issue here is the fact that DAX was never designed to > handle virtually aliased dcache (VIVT and VIPT with aliased dcache). It > touches the pages through their linear mapping, which is not consistent > with the userspace mappings on virtually aliased dcaches. > > This patch series introduces cache_is_aliasing() with new Kconfig > options: > > * ARCH_HAS_CACHE_ALIASING > * ARCH_HAS_CACHE_ALIASING_DYNAMIC > > and implements it for all architectures. The "DYNAMIC" implementation > implements cache_is_aliasing() as a runtime check, which is what is > needed on architectures like 32-bit ARMV6 and ARMV6K. > > With this we can basically narrow down the list of architectures which > are unsupported by DAX to those which are really affected. > > Feedback is welcome, Hi Mathieu, this looks good overall, just some quibbling about the ordering. I would introduce dax_is_supported() with the current overly broad interpretation of "!(ARM || MIPS || SPARC)" using IS_ENABLED(), then fixup the filesystems to use the new helper, and finally go back and convert dax_is_supported() to use cache_is_aliasing() internally. Separately, it is not clear to me why ARCH_HAS_CACHE_ALIASING_DYNAMIC needs to exist. As long as all paths that care are calling cache_is_aliasing() then whether it is dynamic or not is something only the compiler cares about. If those dynamic archs do not want to pay the .text size increase they can always do CONFIG_FS_DAX=n, right?
On 2024-01-29 16:22, Dan Williams wrote: > Mathieu Desnoyers wrote: >> This commit introduced in v5.13 prevents building FS_DAX on 32-bit ARM, >> even on ARMv7 which does not have virtually aliased dcaches: >> >> commit d92576f1167c ("dax: does not work correctly with virtual aliasing caches") >> >> It used to work fine before: I have customers using dax over pmem on >> ARMv7, but this regression will likely prevent them from upgrading their >> kernel. >> >> The root of the issue here is the fact that DAX was never designed to >> handle virtually aliased dcache (VIVT and VIPT with aliased dcache). It >> touches the pages through their linear mapping, which is not consistent >> with the userspace mappings on virtually aliased dcaches. >> >> This patch series introduces cache_is_aliasing() with new Kconfig >> options: >> >> * ARCH_HAS_CACHE_ALIASING >> * ARCH_HAS_CACHE_ALIASING_DYNAMIC >> >> and implements it for all architectures. The "DYNAMIC" implementation >> implements cache_is_aliasing() as a runtime check, which is what is >> needed on architectures like 32-bit ARMV6 and ARMV6K. >> >> With this we can basically narrow down the list of architectures which >> are unsupported by DAX to those which are really affected. >> >> Feedback is welcome, > > Hi Mathieu, this looks good overall, just some quibbling about the > ordering. Thanks for having a look ! > > I would introduce dax_is_supported() with the current overly broad > interpretation of "!(ARM || MIPS || SPARC)" using IS_ENABLED(), then > fixup the filesystems to use the new helper, and finally go back and > convert dax_is_supported() to use cache_is_aliasing() internally. Will do. > > Separately, it is not clear to me why ARCH_HAS_CACHE_ALIASING_DYNAMIC > needs to exist. As long as all paths that care are calling > cache_is_aliasing() then whether it is dynamic or not is something only > the compiler cares about. If those dynamic archs do not want to pay the > .text size increase they can always do CONFIG_FS_DAX=n, right? Good point. It will help reduce complexity and improve test coverage. I also intend to rename "cache_is_aliasing()" to "dcache_is_aliasing()", so if we introduce an "icache_is_aliasing()" in the future, it won't be confusing. Having aliasing icache-dcache but not dcache-dcache seems to be fairly common. So basically: If an arch selects ARCH_HAS_CACHE_ALIASING, it implements dcache_is_aliasing() (for now), and eventually we can implement icache_is_aliasing() as well. Thanks, Mathieu
This commit introduced in v5.13 prevents building FS_DAX on 32-bit ARM, even on ARMv7 which does not have virtually aliased dcaches: commit d92576f1167c ("dax: does not work correctly with virtual aliasing caches") It used to work fine before: I have customers using dax over pmem on ARMv7, but this regression will likely prevent them from upgrading their kernel. The root of the issue here is the fact that DAX was never designed to handle virtually aliased dcache (VIVT and VIPT with aliased dcache). It touches the pages through their linear mapping, which is not consistent with the userspace mappings on virtually aliased dcaches. This patch series introduces cache_is_aliasing() with new Kconfig options: * ARCH_HAS_CACHE_ALIASING * ARCH_HAS_CACHE_ALIASING_DYNAMIC and implements it for all architectures. The "DYNAMIC" implementation implements cache_is_aliasing() as a runtime check, which is what is needed on architectures like 32-bit ARMV6 and ARMV6K. With this we can basically narrow down the list of architectures which are unsupported by DAX to those which are really affected. Feedback is welcome, Thanks, Mathieu Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: linux-mm@kvack.org Cc: linux-arch@vger.kernel.org Cc: Dan Williams <dan.j.williams@intel.com> Cc: Vishal Verma <vishal.l.verma@intel.com> Cc: Dave Jiang <dave.jiang@intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: linux-cxl@vger.kernel.org Cc: nvdimm@lists.linux.dev Mathieu Desnoyers (7): Introduce cache_is_aliasing() across all architectures dax: Fix incorrect list of cache aliasing architectures erofs: Use dax_is_supported() ext2: Use dax_is_supported() ext4: Use dax_is_supported() fuse: Introduce fuse_dax_is_supported() xfs: Use dax_is_supported() arch/arc/Kconfig | 1 + arch/arm/include/asm/cachetype.h | 3 ++ arch/arm/mm/Kconfig | 2 ++ arch/csky/Kconfig | 1 + arch/m68k/Kconfig | 1 + arch/mips/Kconfig | 1 + arch/mips/include/asm/cachetype.h | 9 +++++ arch/nios2/Kconfig | 1 + arch/nios2/include/asm/cachetype.h | 10 ++++++ arch/parisc/Kconfig | 1 + arch/sh/Kconfig | 1 + arch/sparc/Kconfig | 1 + arch/sparc/include/asm/cachetype.h | 14 ++++++++ arch/xtensa/Kconfig | 1 + arch/xtensa/include/asm/cachetype.h | 10 ++++++ fs/Kconfig | 2 +- fs/erofs/super.c | 10 +++--- fs/ext2/super.c | 14 ++++---- fs/ext4/super.c | 52 ++++++++++++++--------------- fs/fuse/file.c | 2 +- fs/fuse/fuse_i.h | 36 +++++++++++++++++++- fs/fuse/inode.c | 47 +++++++++++++------------- fs/fuse/virtio_fs.c | 4 +-- fs/xfs/xfs_super.c | 20 +++++++---- include/linux/cacheinfo.h | 8 +++++ include/linux/dax.h | 9 +++++ mm/Kconfig | 10 ++++++ 27 files changed, 198 insertions(+), 73 deletions(-) create mode 100644 arch/mips/include/asm/cachetype.h create mode 100644 arch/nios2/include/asm/cachetype.h create mode 100644 arch/sparc/include/asm/cachetype.h create mode 100644 arch/xtensa/include/asm/cachetype.h