diff mbox series

[RFC,7/7] xfs: Use dax_is_supported()

Message ID 20240129210631.193493-8-mathieu.desnoyers@efficios.com
State New
Headers
Series Introduce cache_is_aliasing() to fix DAX regression |

Commit Message

Mathieu Desnoyers Jan. 29, 2024, 9:06 p.m. UTC 
  Use dax_is_supported() to validate whether the architecture has
virtually aliased caches at mount time.

This is relevant for architectures which require a dynamic check
to validate whether they have virtually aliased data caches
(ARCH_HAS_CACHE_ALIASING_DYNAMIC=y).

Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing caches")
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Chandan Babu R <chandan.babu@oracle.com>
Cc: Darrick J. Wong <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
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: nvdimm@lists.linux.dev
Cc: linux-cxl@vger.kernel.org
---
 fs/xfs/xfs_super.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

Dave Chinner Jan. 30, 2024, 2:38 a.m. UTC | #1 
On Mon, Jan 29, 2024 at 04:06:31PM -0500, Mathieu Desnoyers wrote:
> Use dax_is_supported() to validate whether the architecture has
> virtually aliased caches at mount time.
> 
> This is relevant for architectures which require a dynamic check
> to validate whether they have virtually aliased data caches
> (ARCH_HAS_CACHE_ALIASING_DYNAMIC=y).

Where's the rest of this patchset? I have no idea what
dax_is_supported() actually does, how it interacts with
CONFIG_FS_DAX, etc.

If you are changing anything to do with FSDAX, the cc-ing the
-entire- patchset to linux-fsdevel is absolutely necessary so the
entire patchset lands in our inboxes and not just a random patch
from the middle of a bigger change.

> Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing caches")
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Chandan Babu R <chandan.babu@oracle.com>
> Cc: Darrick J. Wong <djwong@kernel.org>
> Cc: linux-xfs@vger.kernel.org
> 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: nvdimm@lists.linux.dev
> Cc: linux-cxl@vger.kernel.org
> ---
>  fs/xfs/xfs_super.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 764304595e8b..b27ecb11db66 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1376,14 +1376,22 @@ xfs_fs_parse_param(
>  	case Opt_nodiscard:
>  		parsing_mp->m_features &= ~XFS_FEAT_DISCARD;
>  		return 0;
> -#ifdef CONFIG_FS_DAX
>  	case Opt_dax:
> -		xfs_mount_set_dax_mode(parsing_mp, XFS_DAX_ALWAYS);
> -		return 0;
> +		if (dax_is_supported()) {
> +			xfs_mount_set_dax_mode(parsing_mp, XFS_DAX_ALWAYS);
> +			return 0;
> +		} else {
> +			xfs_warn(parsing_mp, "dax option not supported.");
> +			return -EINVAL;
> +		}
>  	case Opt_dax_enum:
> -		xfs_mount_set_dax_mode(parsing_mp, result.uint_32);
> -		return 0;
> -#endif
> +		if (dax_is_supported()) {
> +			xfs_mount_set_dax_mode(parsing_mp, result.uint_32);
> +			return 0;
> +		} else {
> +			xfs_warn(parsing_mp, "dax option not supported.");
> +			return -EINVAL;
> +		}

Assuming that I understand what dax_is_supported() is doing, this
change isn't right.  We're just setting the DAX configuration flags
from the mount options here, we don't validate them until 
we've parsed all options and eliminated conflicts and rejected
conflicting options. We validate whether the options are
appropriate for the underlying hardware configuration later in the
mount process.

dax=always suitability is check in xfs_setup_dax_always() called
later in the mount process when we have enough context and support
to open storage devices and check them for DAX support. If the
hardware does not support DAX then we simply we turn off DAX
support, we do not reject the mount as this change does.

dax=inode and dax=never are valid options on all configurations,
even those with without FSDAX support or have hardware that is not
capable of using DAX. dax=inode only affects how an inode is
instantiated in cache - if the inode has a flag that says "use DAX"
and dax is suppoortable by the hardware, then the turn on DAX for
that inode. Otherwise we just use the normal non-dax IO paths.

Again, we don't error out the filesystem if DAX is not supported,
we just don't turn it on. This check is done in
xfs_inode_should_enable_dax() and I think all you need to do is
replace the IS_ENABLED(CONFIG_FS_DAX) with a dax_is_supported()
call...

-Dave.
Mathieu Desnoyers Jan. 30, 2024, 3:19 p.m. UTC | #2 
On 2024-01-29 21:38, Dave Chinner wrote:
> On Mon, Jan 29, 2024 at 04:06:31PM -0500, Mathieu Desnoyers wrote:
>> Use dax_is_supported() to validate whether the architecture has
>> virtually aliased caches at mount time.
>>
>> This is relevant for architectures which require a dynamic check
>> to validate whether they have virtually aliased data caches
>> (ARCH_HAS_CACHE_ALIASING_DYNAMIC=y).
> 
> Where's the rest of this patchset? I have no idea what
> dax_is_supported() actually does, how it interacts with
> CONFIG_FS_DAX, etc.
> 
> If you are changing anything to do with FSDAX, the cc-ing the
> -entire- patchset to linux-fsdevel is absolutely necessary so the
> entire patchset lands in our inboxes and not just a random patch
> from the middle of a bigger change.

Sorry, I will Cc linux-fsdevel on all patches for the next round.

Meanwhile you can find the whole series on lore:

https://lore.kernel.org/lkml/20240129210631.193493-1-mathieu.desnoyers@efficios.com/

[...]

> 
> Assuming that I understand what dax_is_supported() is doing, this
> change isn't right.  We're just setting the DAX configuration flags
> from the mount options here, we don't validate them until
> we've parsed all options and eliminated conflicts and rejected
> conflicting options. We validate whether the options are
> appropriate for the underlying hardware configuration later in the
> mount process.
> 
> dax=always suitability is check in xfs_setup_dax_always() called
> later in the mount process when we have enough context and support
> to open storage devices and check them for DAX support. If the
> hardware does not support DAX then we simply we turn off DAX
> support, we do not reject the mount as this change does.
> 
> dax=inode and dax=never are valid options on all configurations,
> even those with without FSDAX support or have hardware that is not
> capable of using DAX. dax=inode only affects how an inode is
> instantiated in cache - if the inode has a flag that says "use DAX"
> and dax is suppoortable by the hardware, then the turn on DAX for
> that inode. Otherwise we just use the normal non-dax IO paths.
> 
> Again, we don't error out the filesystem if DAX is not supported,
> we just don't turn it on. This check is done in
> xfs_inode_should_enable_dax() and I think all you need to do is
> replace the IS_ENABLED(CONFIG_FS_DAX) with a dax_is_supported()
> call...

Thanks a lot for the detailed explanation. You are right, I will
move the dax_is_supported() check to xfs_inode_should_enable_dax().

Mathieu
diff mbox series

Patch

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 764304595e8b..b27ecb11db66 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1376,14 +1376,22 @@  xfs_fs_parse_param(
 	case Opt_nodiscard:
 		parsing_mp->m_features &= ~XFS_FEAT_DISCARD;
 		return 0;
-#ifdef CONFIG_FS_DAX
 	case Opt_dax:
-		xfs_mount_set_dax_mode(parsing_mp, XFS_DAX_ALWAYS);
-		return 0;
+		if (dax_is_supported()) {
+			xfs_mount_set_dax_mode(parsing_mp, XFS_DAX_ALWAYS);
+			return 0;
+		} else {
+			xfs_warn(parsing_mp, "dax option not supported.");
+			return -EINVAL;
+		}
 	case Opt_dax_enum:
-		xfs_mount_set_dax_mode(parsing_mp, result.uint_32);
-		return 0;
-#endif
+		if (dax_is_supported()) {
+			xfs_mount_set_dax_mode(parsing_mp, result.uint_32);
+			return 0;
+		} else {
+			xfs_warn(parsing_mp, "dax option not supported.");
+			return -EINVAL;
+		}
 	/* Following mount options will be removed in September 2025 */
 	case Opt_ikeep:
 		xfs_fs_warn_deprecated(fc, param, XFS_FEAT_IKEEP, true);