diff mbox series

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

Message ID 20240208184913.484340-7-mathieu.desnoyers@efficios.com
State New
Headers
Series Introduce cpu_dcache_is_aliasing() to fix DAX regression |

Commit Message

Mathieu Desnoyers Feb. 8, 2024, 6:49 p.m. UTC 
  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(-)

Comments

Dan Williams Feb. 8, 2024, 9:39 p.m. UTC | #1 
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.
Mathieu Desnoyers Feb. 8, 2024, 10:10 p.m. UTC | #2 
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
Dan Williams Feb. 8, 2024, 10:37 p.m. UTC | #3 
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.
Mathieu Desnoyers Feb. 8, 2024, 10:42 p.m. UTC | #4 
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
Dan Williams Feb. 9, 2024, 1:01 a.m. UTC | #5 
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 mbox series

Patch

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)
 {