| Message ID | 20240208184913.484340-7-mathieu.desnoyers@efficios.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce cpu_dcache_is_aliasing() to fix DAX regression | expand |
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.
On 2024-02-08 16:39, Dan Williams wrote: [...] > > 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. Done. > > Rest of this patch looks good to me. Adding your Acked-by to what is left of this patch if OK with you. Thanks, Mathieu
Mathieu Desnoyers wrote: > On 2024-02-08 16:39, Dan Williams wrote: > [...] > > > > 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. > > Done. > > > > > Rest of this patch looks good to me. > > Adding your Acked-by to what is left of this patch if OK with you. You can add: Reviewed-by: Dan Williams <dan.j.williams@intel.com> ...after that re-org.
On 2024-02-08 17:37, Dan Williams wrote: > Mathieu Desnoyers wrote: >> On 2024-02-08 16:39, Dan Williams wrote: >> [...] >>> >>> 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. >> >> Done. >> >>> >>> Rest of this patch looks good to me. >> >> Adding your Acked-by to what is left of this patch if OK with you. > > You can add: > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > ...after that re-org. Just to make sure: are you OK with me adding your Reviewed-by only for what is left of this patch, or also to the other driver patches after integrating your requested changes ? Thanks, Mathieu
Mathieu Desnoyers wrote: > On 2024-02-08 17:37, Dan Williams wrote: > > Mathieu Desnoyers wrote: > >> On 2024-02-08 16:39, Dan Williams wrote: > >> [...] > >>> > >>> 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. > >> > >> Done. > >> > >>> > >>> Rest of this patch looks good to me. > >> > >> Adding your Acked-by to what is left of this patch if OK with you. > > > > You can add: > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > > ...after that re-org. > > Just to make sure: are you OK with me adding your Reviewed-by > only for what is left of this patch, or also to the other driver > patches after integrating your requested changes ? Sure, if you make all those changes go ahead and propagate my Reviewed-by across the set.
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); } static inline void put_dax(struct dax_device *dax_dev) {
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(-)