On 2024-01-31 16:02, Dan Williams wrote:
Mathieu Desnoyers wrote:
Replace the following fs/Kconfig:FS_DAX dependency:
depends on !(ARM || MIPS || SPARC)
By a runtime check within alloc_dax().
This is done in preparation for its use by each filesystem supporting
the "dax" mount option to validate whether DAX is indeed supported.
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@xxxxxxxxxxxx>
Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Cc: linux-mm@xxxxxxxxx
Cc: linux-arch@xxxxxxxxxxxxxxx
Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
Cc: Vishal Verma <vishal.l.verma@xxxxxxxxx>
Cc: Dave Jiang <dave.jiang@xxxxxxxxx>
Cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
Cc: Arnd Bergmann <arnd@xxxxxxxx>
Cc: Russell King <linux@xxxxxxxxxxxxxxx>
Cc: nvdimm@xxxxxxxxxxxxxxx
Cc: linux-cxl@xxxxxxxxxxxxxxx
Cc: linux-fsdevel@xxxxxxxxxxxxxxx
Cc: dm-devel@xxxxxxxxxxxxxxx
---
drivers/dax/super.c | 6 ++++++
fs/Kconfig | 1 -
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index 0da9232ea175..e9f397b8a5a3 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -445,6 +445,12 @@ 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. */
+ if (IS_ENABLED(CONFIG_ARM) ||
+ IS_ENABLED(CONFIG_MIPS) ||
+ IS_ENABLED(CONFIG_SPARC))
+ return NULL;
This function returns ERR_PTR(), not NULL on failure.
Except that it returns NULL in the CONFIG_DAX=n case as you
noticed below.
...and I notice this mistake is also made in include/linux/dax.h in the
CONFIG_DAX=n case. That function also mentions:
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;
}
...and none of the callers validate the result, but now runtime
validation is necessary. I.e. it is not enough to check
IS_ENABLED(CONFIG_DAX) it also needs to check cpu_dcache_is_aliasing().
If the callers select DAX in their Kconfig, then they don't have to
explicitly check for IS_ENABLED(CONFIG_DAX). Things change for the
introduced runtime check though.
With that, there are a few more fixup places needed, pmem_attach_disk(),
dcssblk_add_store(), and virtio_fs_setup_dax().
Which approach should we take then ? Should we:
A) Keep returning NULL from alloc_dax() for both
cpu_dcache_is_aliasing() and CONFIG_DAX=n, and use IS_ERR_OR_NULL()
in the caller. If we do this, then the callers need to somehow
translate this NULL into a negative error value, or
B) Replace this NULL return value in both cases by a ERR_PTR() (which
error value should we return ?).
I would favor approach B) which appears more robust and introduces
fewer changes. If we go for that approach do we still need to change
the callers ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
