Messages in this thread | | | Date | Wed, 31 Jan 2024 16:39:57 -0500 | Subject | Re: [RFC PATCH v3 2/4] dax: Check for data cache aliasing at runtime | From | Mathieu Desnoyers <> |
| |
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@efficios.com> >> 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: Arnd Bergmann <arnd@arndb.de> >> Cc: Russell King <linux@armlinux.org.uk> >> Cc: nvdimm@lists.linux.dev >> Cc: linux-cxl@vger.kernel.org >> Cc: linux-fsdevel@vger.kernel.org >> Cc: dm-devel@lists.linux.dev >> --- >> 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
| |