RE: [PATCH v4 06/12] dax: Check for data cache aliasing at runtime

Dan Williams
2024-02-08T21:39:37Z

Mathieu Desnoyers wrote:
> Replace the following fs/Kconfig:FS_DAX dependency:
>
> depends on !(ARM || MIPS || SPARC)
>
> By a runtime check within alloc_dax(). This runtime check returns
> ERR_PTR(-EOPNOTSUPP) if the @ops parameter is non-NULL (which means
> the kernel is using an aliased mapping) on an architecture which
> has data cache aliasing.
>
> Change the return value from NULL to PTR_ERR(-EOPNOTSUPP) for
> CONFIG_DAX=n for consistency.
>
> This is done in preparation for using cpu_dcache_is_aliasing() in a
> following change which will properly support architectures which detect
> data cache aliasing at runtime.
>
> Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing caches")
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.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: Arnd Bergmann <arnd@arndb.de>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: linux-arch@vger.kernel.org
> Cc: linux-cxl@vger.kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-xfs@vger.kernel.org
> Cc: dm-devel@lists.linux.dev
> Cc: nvdimm@lists.linux.dev
> ---
> drivers/dax/super.c | 15 +++++++++++++++
> fs/Kconfig | 1 -
> include/linux/dax.h | 6 +-----
> 3 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 0da9232ea175..ce5bffa86bba 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -319,6 +319,11 @@ EXPORT_SYMBOL_GPL(dax_alive);
> * that any fault handlers or operations that might have seen
> * dax_alive(), have completed. Any operations that start after
> * synchronize_srcu() has run will abort upon seeing !dax_alive().
> + *
> + * Note, because alloc_dax() returns an ERR_PTR() on error, callers
> + * typically store its result into a local variable in order to check
> + * the result. Therefore, care must be taken to populate the struct
> + * device dax_dev field make sure the dax_dev is not leaked.
> */
> void kill_dax(struct dax_device *dax_dev)
> {
> @@ -445,6 +450,16 @@ struct dax_device *alloc_dax(void *private, const struct dax_operations *ops)
> dev_t devt;
> int minor;
>
> + /*
> + * Unavailable on architectures with virtually aliased data caches,
> + * except for device-dax (NULL operations pointer), which does
> + * not use aliased mappings from the kernel.
> + */
> + if (ops && (IS_ENABLED(CONFIG_ARM) ||
> + IS_ENABLED(CONFIG_MIPS) ||
> + IS_ENABLED(CONFIG_SPARC)))
> + return ERR_PTR(-EOPNOTSUPP);
> +
> if (WARN_ON_ONCE(ops && !ops->zero_page_range))
> return ERR_PTR(-EINVAL);
>
> diff --git a/fs/Kconfig b/fs/Kconfig
> index 42837617a55b..e5efdb3b276b 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -56,7 +56,6 @@ endif # BLOCK
> config FS_DAX
> bool "File system based Direct Access (DAX) support"
> depends on MMU
> - depends on !(ARM || MIPS || SPARC)
> depends on ZONE_DEVICE || FS_DAX_LIMITED
> select FS_IOMAP
> select DAX
> diff --git a/include/linux/dax.h b/include/linux/dax.h
> index b463502b16e1..df2d52b8a245 100644
> --- a/include/linux/dax.h
> +++ b/include/linux/dax.h
> @@ -86,11 +86,7 @@ static inline void *dax_holder(struct dax_device *dax_dev)
> static inline struct dax_device *alloc_dax(void *private,
> const struct dax_operations *ops)
> {
> - /*
> - * Callers should check IS_ENABLED(CONFIG_DAX) to know if this
> - * NULL is an error or expected.
> - */
> - return NULL;
> + return ERR_PTR(-EOPNOTSUPP);
> }

So per other feedback on earlier patches, I think this hunk deserves to
be moved to its own patch earlier in the series as a standalone fixup.

Rest of this patch looks good to me.