[PATCH v2 0/3] Add alignment check for DAX mount

[PATCH v2 0/3] Add alignment check for DAX mount

[Toshi Kani]

When a partition is not aligned by 4KB, mount -o dax succeeds,
but any read/write access to the filesystem fails, except for
metadata update.

Add alignment check to ext4, ext2, and xfs.

v2:
 - Use a helper function via ->direct_access for the check.
   (Christoph Hellwig)
 - Call bdev_direct_access() with sector 0 for the check.
   (Boaz Harrosh)

---
Toshi Kani (3):
 1/3 ext4: Add alignment check for DAX mount
 2/3 ext2: Add alignment check for DAX mount
 3/3 xfs: Add alignment check for DAX mount

---
 fs/ext2/super.c    | 21 +++++++++++++++++++--
 fs/ext4/super.c    | 20 ++++++++++++++++++--
 fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
 3 files changed, 56 insertions(+), 8 deletions(-)

[PATCH v2 1/3] ext4: Add alignment check for DAX mount

[Toshi Kani]

When a partition is not aligned by 4KB, mount -o dax succeeds,
but any read/write access to the filesystem fails, except for
metadata update.

Call bdev_direct_access to check the alignment when -o dax is
specified.

Reported-by: Micah Parrish <micah.parrish@hpe.com>
Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: Jan Kara <jack@suse.cz>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Boaz Harrosh <boaz@plexistor.com>
---
 fs/ext4/super.c |   20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 304c712..51ac78e 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3416,14 +3416,30 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	if (sbi->s_mount_opt & EXT4_MOUNT_DAX) {
+		struct blk_dax_ctl dax = {
+			.sector = 0,
+			.size = PAGE_SIZE,
+		};
 		if (blocksize != PAGE_SIZE) {
 			ext4_msg(sb, KERN_ERR,
 					"error: unsupported blocksize for dax");
 			goto failed_mount;
 		}
-		if (!sb->s_bdev->bd_disk->fops->direct_access) {
-			ext4_msg(sb, KERN_ERR,
+		err = bdev_direct_access(sb->s_bdev, &dax);
+		if (err < 0) {
+			switch (err) {
+			case -EOPNOTSUPP:
+				ext4_msg(sb, KERN_ERR,
 					"error: device does not support dax");
+				break;
+			case -EINVAL:
+				ext4_msg(sb, KERN_ERR,
+					"error: unaligned partition for dax");
+				break;
+			default:
+				ext4_msg(sb, KERN_ERR,
+					"error: dax access failed (%d)", err);
+			}
 			goto failed_mount;
 		}
 	}

[PATCH v2 2/3] ext2: Add alignment check for DAX mount

[Toshi Kani]

When a partition is not aligned by 4KB, mount -o dax succeeds,
but any read/write access to the filesystem fails, except for
metadata update.

Call bdev_direct_access to check the alignment when -o dax is
specified.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Boaz Harrosh <boaz@plexistor.com>
---
 fs/ext2/super.c |   21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index b78caf2..e636b56 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -922,14 +922,31 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
 	blocksize = BLOCK_SIZE << le32_to_cpu(sbi->s_es->s_log_block_size);
 
 	if (sbi->s_mount_opt & EXT2_MOUNT_DAX) {
+		struct blk_dax_ctl dax = {
+			.sector = 0,
+			.size = PAGE_SIZE,
+		};
 		if (blocksize != PAGE_SIZE) {
 			ext2_msg(sb, KERN_ERR,
 					"error: unsupported blocksize for dax");
 			goto failed_mount;
 		}
-		if (!sb->s_bdev->bd_disk->fops->direct_access) {
-			ext2_msg(sb, KERN_ERR,
+		err = bdev_direct_access(sb->s_bdev, &dax);
+		if (err < 0) {
+			switch (err) {
+			case -EOPNOTSUPP:
+				ext2_msg(sb, KERN_ERR,
 					"error: device does not support dax");
+				break;
+			case -EINVAL:
+				ext2_msg(sb, KERN_ERR,
+					"error: unaligned partition for dax");
+				break;
+			default:
+				ext2_msg(sb, KERN_ERR,
+					"error: dax access failed (%d)", err);
+				break;
+			}
 			goto failed_mount;
 		}
 	}

[PATCH v2 3/3] xfs: Add alignment check for DAX mount

[Toshi Kani]

When a partition is not aligned by 4KB, mount -o dax succeeds,
but any read/write access to the filesystem fails, except for
metadata update.

Call bdev_direct_access to check the alignment when -o dax is
specified.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Dave Chinner <david@fromorbit.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Boaz Harrosh <boaz@plexistor.com>
---
 fs/xfs/xfs_super.c |   23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 187e14b..b7ee323 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1557,15 +1557,30 @@ xfs_fs_fill_super(
 		sb->s_flags |= MS_I_VERSION;
 
 	if (mp->m_flags & XFS_MOUNT_DAX) {
+		struct blk_dax_ctl dax = {
+			.sector = 0,
+			.size = PAGE_SIZE,
+		};
 		xfs_warn(mp,
-	"DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
+		"DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
 		if (sb->s_blocksize != PAGE_SIZE) {
 			xfs_alert(mp,
 		"Filesystem block size invalid for DAX Turning DAX off.");
 			mp->m_flags &= ~XFS_MOUNT_DAX;
-		} else if (!sb->s_bdev->bd_disk->fops->direct_access) {
-			xfs_alert(mp,
-		"Block device does not support DAX Turning DAX off.");
+		} else if ((error = bdev_direct_access(sb->s_bdev, &dax)) < 0) {
+			switch (error) {
+			case -EOPNOTSUPP:
+				xfs_alert(mp,
+			"Block device does not support DAX Turning DAX off.");
+				break;
+			case -EINVAL:
+				xfs_alert(mp,
+			"Partition alignment invalid for DAX Turning DAX off.");
+				break;
+			default:
+				xfs_alert(mp,
+			"DAX access failed (%d) DAX Turning DAX off.", error);
+			}
 			mp->m_flags &= ~XFS_MOUNT_DAX;
 		}
 	}

Re: [PATCH v2 0/3] Add alignment check for DAX mount

[Boaz Harrosh]

On 05/02/2016 09:42 PM, Toshi Kani wrote:
> When a partition is not aligned by 4KB, mount -o dax succeeds,
> but any read/write access to the filesystem fails, except for
> metadata update.
> 
> Add alignment check to ext4, ext2, and xfs.
> 
> v2:
>  - Use a helper function via ->direct_access for the check.
>    (Christoph Hellwig)
>  - Call bdev_direct_access() with sector 0 for the check.
>    (Boaz Harrosh)
> 
> ---
> Toshi Kani (3):
>  1/3 ext4: Add alignment check for DAX mount
>  2/3 ext2: Add alignment check for DAX mount
>  3/3 xfs: Add alignment check for DAX mount
> 

All patches look very good to me, and keep the
internals internal. Thanks Toshi

Review-by: Boaz Harrosh <boaz@plexistor.com>

> ---
>  fs/ext2/super.c    | 21 +++++++++++++++++++--
>  fs/ext4/super.c    | 20 ++++++++++++++++++--
>  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
>  3 files changed, 56 insertions(+), 8 deletions(-)
> 

Re: [PATCH v2 3/3] xfs: Add alignment check for DAX mount

[Ross Zwisler]

On Mon, May 02, 2016 at 12:42:58PM -0600, Toshi Kani wrote:
> When a partition is not aligned by 4KB, mount -o dax succeeds,
> but any read/write access to the filesystem fails, except for
> metadata update.
> 
> Call bdev_direct_access to check the alignment when -o dax is
> specified.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Dave Chinner <david@fromorbit.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Boaz Harrosh <boaz@plexistor.com>
> ---
>  fs/xfs/xfs_super.c |   23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 187e14b..b7ee323 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1557,15 +1557,30 @@ xfs_fs_fill_super(
>  		sb->s_flags |= MS_I_VERSION;
>  
>  	if (mp->m_flags & XFS_MOUNT_DAX) {
> +		struct blk_dax_ctl dax = {
> +			.sector = 0,
> +			.size = PAGE_SIZE,
> +		};
>  		xfs_warn(mp,
> -	"DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
> +		"DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
>  		if (sb->s_blocksize != PAGE_SIZE) {
>  			xfs_alert(mp,
>  		"Filesystem block size invalid for DAX Turning DAX off.");
>  			mp->m_flags &= ~XFS_MOUNT_DAX;
> -		} else if (!sb->s_bdev->bd_disk->fops->direct_access) {
> -			xfs_alert(mp,
> -		"Block device does not support DAX Turning DAX off.");
> +		} else if ((error = bdev_direct_access(sb->s_bdev, &dax)) < 0) {
> +			switch (error) {
> +			case -EOPNOTSUPP:
> +				xfs_alert(mp,
> +			"Block device does not support DAX Turning DAX off.");

Since you're already in here editing all the strings, can you add a period to
make it more readable?   Applies to all strings.

> +			"Block device does not support DAX. Turning DAX off.");
							  ^

> +				break;
> +			case -EINVAL:
> +				xfs_alert(mp,
> +			"Partition alignment invalid for DAX Turning DAX off.");
> +				break;
> +			default:
> +				xfs_alert(mp,
> +			"DAX access failed (%d) DAX Turning DAX off.", error);

I DAX think you might DAX have too many DAXes in here. :)

> +			}
>  			mp->m_flags &= ~XFS_MOUNT_DAX;
>  		}
>  	}

Other than the nit-picking about the strings, this seems fine.

You can add this for the series:
Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>

Re: [PATCH v2 3/3] xfs: Add alignment check for DAX mount

[Toshi Kani]

On Mon, 2016-05-02 at 13:50 -0600, Ross Zwisler wrote:
> On Mon, May 02, 2016 at 12:42:58PM -0600, Toshi Kani wrote:
 :
> >  		xfs_warn(mp,
> > -	"DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
> > +		"DAX enabled. Warning: EXPERIMENTAL, use at your own
> > risk");
> >  		if (sb->s_blocksize != PAGE_SIZE) {
> >  			xfs_alert(mp,
> >  		"Filesystem block size invalid for DAX Turning DAX
> > off.");
> >  			mp->m_flags &= ~XFS_MOUNT_DAX;
> > -		} else if (!sb->s_bdev->bd_disk->fops->direct_access)
> > {
> > -			xfs_alert(mp,
> > -		"Block device does not support DAX Turning DAX off.");
> > +		} else if ((error = bdev_direct_access(sb->s_bdev,
> > &dax)) < 0) {
> > +			switch (error) {
> > +			case -EOPNOTSUPP:
> > +				xfs_alert(mp,
> > +			"Block device does not support DAX Turning DAX
> > off.");
>
> Since you're already in here editing all the strings, can you add a
> period to make it more readable?   Applies to all strings.

Right. Will do.

> > 
> > +			"Block device does not support DAX. Turning
> > DAX off.");
> 							  ^
> 
> > 
> > +				break;
> > +			case -EINVAL:
> > +				xfs_alert(mp,
> > +			"Partition alignment invalid for DAX Turning
> > DAX off.");
> > +				break;
> > +			default:
> > +				xfs_alert(mp,
> > +			"DAX access failed (%d) DAX Turning DAX off.",
> > error);
>
> I DAX think you might DAX have too many DAXes in here. :)

Oops! :)

> > 
> > +			}
> >  			mp->m_flags &= ~XFS_MOUNT_DAX;
> >  		}
> >  	}
>
> Other than the nit-picking about the strings, this seems fine.
> 
> You can add this for the series:
> Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>

Thanks!
-Toshi

Re: [PATCH v2 1/3] ext4: Add alignment check for DAX mount

[Jan Kara]

On Mon 02-05-16 12:42:56, Toshi Kani wrote:
> When a partition is not aligned by 4KB, mount -o dax succeeds,
> but any read/write access to the filesystem fails, except for
> metadata update.
> 
> Call bdev_direct_access to check the alignment when -o dax is
> specified.
> 
> Reported-by: Micah Parrish <micah.parrish@hpe.com>
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Boaz Harrosh <boaz@plexistor.com>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  fs/ext4/super.c |   20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 304c712..51ac78e 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3416,14 +3416,30 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  	}
>  
>  	if (sbi->s_mount_opt & EXT4_MOUNT_DAX) {
> +		struct blk_dax_ctl dax = {
> +			.sector = 0,
> +			.size = PAGE_SIZE,
> +		};
>  		if (blocksize != PAGE_SIZE) {
>  			ext4_msg(sb, KERN_ERR,
>  					"error: unsupported blocksize for dax");
>  			goto failed_mount;
>  		}
> -		if (!sb->s_bdev->bd_disk->fops->direct_access) {
> -			ext4_msg(sb, KERN_ERR,
> +		err = bdev_direct_access(sb->s_bdev, &dax);
> +		if (err < 0) {
> +			switch (err) {
> +			case -EOPNOTSUPP:
> +				ext4_msg(sb, KERN_ERR,
>  					"error: device does not support dax");
> +				break;
> +			case -EINVAL:
> +				ext4_msg(sb, KERN_ERR,
> +					"error: unaligned partition for dax");
> +				break;
> +			default:
> +				ext4_msg(sb, KERN_ERR,
> +					"error: dax access failed (%d)", err);
> +			}
>  			goto failed_mount;
>  		}
>  	}
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 2/3] ext2: Add alignment check for DAX mount

[Jan Kara]

On Mon 02-05-16 12:42:57, Toshi Kani wrote:
> When a partition is not aligned by 4KB, mount -o dax succeeds,
> but any read/write access to the filesystem fails, except for
> metadata update.
> 
> Call bdev_direct_access to check the alignment when -o dax is
> specified.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Boaz Harrosh <boaz@plexistor.com>

The patch looks good. I've added it to my tree and will push it to Linus in
the coming merge window.

								Honza

> ---
>  fs/ext2/super.c |   21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index b78caf2..e636b56 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -922,14 +922,31 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
>  	blocksize = BLOCK_SIZE << le32_to_cpu(sbi->s_es->s_log_block_size);
>  
>  	if (sbi->s_mount_opt & EXT2_MOUNT_DAX) {
> +		struct blk_dax_ctl dax = {
> +			.sector = 0,
> +			.size = PAGE_SIZE,
> +		};
>  		if (blocksize != PAGE_SIZE) {
>  			ext2_msg(sb, KERN_ERR,
>  					"error: unsupported blocksize for dax");
>  			goto failed_mount;
>  		}
> -		if (!sb->s_bdev->bd_disk->fops->direct_access) {
> -			ext2_msg(sb, KERN_ERR,
> +		err = bdev_direct_access(sb->s_bdev, &dax);
> +		if (err < 0) {
> +			switch (err) {
> +			case -EOPNOTSUPP:
> +				ext2_msg(sb, KERN_ERR,
>  					"error: device does not support dax");
> +				break;
> +			case -EINVAL:
> +				ext2_msg(sb, KERN_ERR,
> +					"error: unaligned partition for dax");
> +				break;
> +			default:
> +				ext2_msg(sb, KERN_ERR,
> +					"error: dax access failed (%d)", err);
> +				break;
> +			}
>  			goto failed_mount;
>  		}
>  	}
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 1/3] ext4: Add alignment check for DAX mount

[Christoph Hellwig]

Please come up with a version that doesn't require tons of boilerplate
code in every file system.

Re: [PATCH v2 1/3] ext4: Add alignment check for DAX mount

[Jan Kara]

On Tue 03-05-16 01:44:10, Christoph Hellwig wrote:
> Please come up with a version that doesn't require tons of boilerplate
> code in every file system.

Well, I was thinking about some helper as well but we could save ~4 lines
with that and that didn't seem significant to me. Most of the lines is
actually reporting appropriate mount error in dmesg and that is
fs-dependent so it needs to stay in the filesystem... So what do you have
in mind?

							Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 1/3] ext4: Add alignment check for DAX mount

[Ross Zwisler]

On Tue, May 03, 2016 at 11:00:21AM +0200, Jan Kara wrote:
> On Tue 03-05-16 01:44:10, Christoph Hellwig wrote:
> > Please come up with a version that doesn't require tons of boilerplate
> > code in every file system.
> 
> Well, I was thinking about some helper as well but we could save ~4 lines
> with that and that didn't seem significant to me. Most of the lines is
> actually reporting appropriate mount error in dmesg and that is
> fs-dependent so it needs to stay in the filesystem... So what do you have
> in mind?

I guess if you wanted to reduce the code needed in each filesystem, you could
avoid having different error messages for each of the failure conditions, and
just print the error value.  All the error cases caught by the current code are
unique, so we aren't losing any information.  The resulting patch for ext4
would look like this:

@@ -3416,14 +3416,19 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
        }
 
        if (sbi->s_mount_opt & EXT4_MOUNT_DAX) {
+               struct blk_dax_ctl dax = {
+                       .sector = 0,
+                       .size = PAGE_SIZE,
+               };
                if (blocksize != PAGE_SIZE) {
                        ext4_msg(sb, KERN_ERR,
                                        "error: unsupported blocksize for dax");
                        goto failed_mount;
                }
-               if (!sb->s_bdev->bd_disk->fops->direct_access) {
+               err = bdev_direct_access(sb->s_bdev, &dax);
+               if (err < 0) {
                        ext4_msg(sb, KERN_ERR,
-                                       "error: device does not support dax");
+                                       "error: dax access failed (%d)", err);
                        goto failed_mount;
                }
        }

Re: [PATCH v2 1/3] ext4: Add alignment check for DAX mount

[Toshi Kani]

On Tue, 2016-05-03 at 08:43 -0600, Ross Zwisler wrote:
> On Tue, May 03, 2016 at 11:00:21AM +0200, Jan Kara wrote:
> > 
> > On Tue 03-05-16 01:44:10, Christoph Hellwig wrote:
> > > 
> > > Please come up with a version that doesn't require tons of
> > > boilerplate code in every file system.
> >
> > Well, I was thinking about some helper as well but we could save ~4
> > lines with that and that didn't seem significant to me. Most of the
> > lines is actually reporting appropriate mount error in dmesg and that
> > is fs-dependent so it needs to stay in the filesystem... So what do you
> > have in mind?
>
> I guess if you wanted to reduce the code needed in each filesystem, you
> could avoid having different error messages for each of the failure
> conditions, and just print the error value.  All the error cases caught
> by the current code are unique, so we aren't losing any information.  The
> resulting patch for ext4 would look like this:

I'd prefer to keep the "error: device does not support dax" and "error:
unaligned partition for dax" messages since they clarify the problem as a
user error. The former case is especially common with BTT. The "error: dax
access failed (%d)" message is helpful when we need to look into the case.

If XFS is OK to use the messages similar to ext2/4, then we can do: 

 * Add a new helper function, say bdev_check_dax_mount(), which logs common
error messages with pr_err(), such as:
       "VFS (pmem0): error: device does not support dax"
 * When bdev_check_dax_mount() returns with a negative value:
   - XFS logs an additional message below via xfs_alert() and proceeds
without dax option.
       "XFS (pmem0): Turning DAX off."
   - ext2/4 fails the mount.

Thanks,
-Toshi

Re: [PATCH v2 1/3] ext4: Add alignment check for DAX mount

[Toshi Kani]

On Tue, 2016-05-03 at 15:41 +0000, Kani, Toshimitsu wrote:
> On Tue, 2016-05-03 at 08:43 -0600, Ross Zwisler wrote:
> > On Tue, May 03, 2016 at 11:00:21AM +0200, Jan Kara wrote:
> > > On Tue 03-05-16 01:44:10, Christoph Hellwig wrote:
> > > > 
> > > > Please come up with a version that doesn't require tons of
> > > > boilerplate code in every file system.
> > >
> > > Well, I was thinking about some helper as well but we could save ~4
> > > lines with that and that didn't seem significant to me. Most of the
> > > lines is actually reporting appropriate mount error in dmesg and that
> > > is fs-dependent so it needs to stay in the filesystem... So what do
> > > you have in mind?
> >
> > I guess if you wanted to reduce the code needed in each filesystem, you
> > could avoid having different error messages for each of the failure
> > conditions, and just print the error value.  All the error cases caught
> > by the current code are unique, so we aren't losing any
> > information.  The resulting patch for ext4 would look like this:
>
> I'd prefer to keep the "error: device does not support dax" and "error:
> unaligned partition for dax" messages since they clarify the problem as a
> user error. The former case is especially common with BTT. The "error:
> dax access failed (%d)" message is helpful when we need to look into the
> case.
> 
> If XFS is OK to use the messages similar to ext2/4, then we can do: 
> 
>  * Add a new helper function, say bdev_check_dax_mount(), which logs
> common error messages with pr_err(), such as:
>        "VFS (pmem0): error: device does not support dax"
>  * When bdev_check_dax_mount() returns with a negative value:
>    - XFS logs an additional message below via xfs_alert() and proceeds
> without dax option.
>        "XFS (pmem0): Turning DAX off."
>    - ext2/4 fails the mount.

Since v2 got multiple reviews, and Jan has added 2/3 into his tree
(thanks!), I will send "[PATCH v2-UPDATE 3/3]" which addresses Ross's
comments on v2 patch 3/3.  This keeps the v2 series.

Christoph, let me know if you'd still like to see v3 with the above changes
to share the same error messages among ext2/4 and xfs.

Thanks,
-Toshi

Re: [PATCH v2 3/3] xfs: Add alignment check for DAX mount

[Dave Chinner]

On Mon, May 02, 2016 at 12:42:58PM -0600, Toshi Kani wrote:
> When a partition is not aligned by 4KB, mount -o dax succeeds,
> but any read/write access to the filesystem fails, except for
> metadata update.
> 
> Call bdev_direct_access to check the alignment when -o dax is
> specified.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Dave Chinner <david@fromorbit.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Boaz Harrosh <boaz@plexistor.com>
> ---
>  fs/xfs/xfs_super.c |   23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 187e14b..b7ee323 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1557,15 +1557,30 @@ xfs_fs_fill_super(
>  		sb->s_flags |= MS_I_VERSION;
>  
>  	if (mp->m_flags & XFS_MOUNT_DAX) {
> +		struct blk_dax_ctl dax = {
> +			.sector = 0,
> +			.size = PAGE_SIZE,
> +		};
>  		xfs_warn(mp,
> -	"DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
> +		"DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
>  		if (sb->s_blocksize != PAGE_SIZE) {
>  			xfs_alert(mp,
>  		"Filesystem block size invalid for DAX Turning DAX off.");
>  			mp->m_flags &= ~XFS_MOUNT_DAX;
> -		} else if (!sb->s_bdev->bd_disk->fops->direct_access) {
> -			xfs_alert(mp,
> -		"Block device does not support DAX Turning DAX off.");
> +		} else if ((error = bdev_direct_access(sb->s_bdev, &dax)) < 0) {
> +			switch (error) {
> +			case -EOPNOTSUPP:
> +				xfs_alert(mp,
> +			"Block device does not support DAX Turning DAX off.");
> +				break;
> +			case -EINVAL:
> +				xfs_alert(mp,
> +			"Partition alignment invalid for DAX Turning DAX off.");
> +				break;
> +			default:
> +				xfs_alert(mp,
> +			"DAX access failed (%d) DAX Turning DAX off.", error);
> +			}

Please write a helper along the lines of:

	error = blkdev_supports_dax(sb->s_bdev, sb->s_blocksize);

and encapsulate all this, including the specific error messages in the
helper (i.e. "Block device %s does not support DAX."). Then the rest
of the filesystem code looks something like this:

	if (mp->m_flags & XFS_MOUNT_DAX) {
		error = blkdev_supports_dax(sb->s_bdev, sb->s_blocksize);
		if (error) {
			xfs_alert(mp,
		"DAX unsupported by block device. Turning off DAX.");
			mp->m_flags &= ~XFS_MOUNT_DAX;
		}
	}

And each filesystem can choose to do what it wants with the error
without having to care exactly why DAX is not supported.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 3/3] xfs: Add alignment check for DAX mount

[Toshi Kani]

On Thu, 2016-05-05 at 09:18 +1000, Dave Chinner wrote:
> On Mon, May 02, 2016 at 12:42:58PM -0600, Toshi Kani wrote:
> > 
:
> Please write a helper along the lines of:
> 
> 	error = blkdev_supports_dax(sb->s_bdev, sb->s_blocksize);
> 
> and encapsulate all this, including the specific error messages in the
> helper (i.e. "Block device %s does not support DAX."). Then the rest
> of the filesystem code looks something like this:
> 
> 	if (mp->m_flags & XFS_MOUNT_DAX) {
> 		error = blkdev_supports_dax(sb->s_bdev, sb->s_blocksize);
> 		if (error) {
> 			xfs_alert(mp,
> 		"DAX unsupported by block device. Turning off DAX.");
> 			mp->m_flags &= ~XFS_MOUNT_DAX;
> 		}
> 	}
> 
> And each filesystem can choose to do what it wants with the error
> without having to care exactly why DAX is not supported.

Yes, I had this change in mind and was wondering if you are OK with it
since I am incline to keep the ext2/4 message style as majority rule. :)
https://lkml.org/lkml/2016/5/3/543

Assuming that's OK, I will make this change.

Thanks!
-Toshi

Re: [PATCH v2 3/3] xfs: Add alignment check for DAX mount

[Jan Kara]

On Wed 04-05-16 17:41:26, Toshi Kani wrote:
> On Thu, 2016-05-05 at 09:18 +1000, Dave Chinner wrote:
> > On Mon, May 02, 2016 at 12:42:58PM -0600, Toshi Kani wrote:
> > > 
> :
> > Please write a helper along the lines of:
> > 
> > 	error = blkdev_supports_dax(sb->s_bdev, sb->s_blocksize);
> > 
> > and encapsulate all this, including the specific error messages in the
> > helper (i.e. "Block device %s does not support DAX."). Then the rest
> > of the filesystem code looks something like this:
> > 
> > 	if (mp->m_flags & XFS_MOUNT_DAX) {
> > 		error = blkdev_supports_dax(sb->s_bdev, sb->s_blocksize);
> > 		if (error) {
> > 			xfs_alert(mp,
> > 		"DAX unsupported by block device. Turning off DAX.");
> > 			mp->m_flags &= ~XFS_MOUNT_DAX;
> > 		}
> > 	}
> > 
> > And each filesystem can choose to do what it wants with the error
> > without having to care exactly why DAX is not supported.
> 
> Yes, I had this change in mind and was wondering if you are OK with it
> since I am incline to keep the ext2/4 message style as majority rule. :)
> https://lkml.org/lkml/2016/5/3/543
> 
> Assuming that's OK, I will make this change.

Yeah, just send me ext2 changes on top of your v2 and I can pull it to my
tree.


								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR