diff mbox

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

Message ID 1462214578-27122-2-git-send-email-toshi.kani@hpe.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kani, Toshi May 2, 2016, 6:42 p.m. UTC 
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(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jan Kara May 3, 2016, 7:58 a.m. UTC | #1 
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;
>  		}
>  	}
Christoph Hellwig May 3, 2016, 8:44 a.m. UTC | #2 
Please come up with a version that doesn't require tons of boilerplate
code in every file system.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara May 3, 2016, 9 a.m. UTC | #3 
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
Ross Zwisler May 3, 2016, 2:43 p.m. UTC | #4 
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;
                }
        }
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kani, Toshi May 3, 2016, 3:41 p.m. UTC | #5 
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


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kani, Toshi May 4, 2016, 3:42 p.m. UTC | #6 
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



--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 		}
 	}