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

[PATCH 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.

---
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    | 6 ++++++
 fs/ext4/super.c    | 6 ++++++
 fs/xfs/xfs_super.c | 6 ++++++
 3 files changed, 18 insertions(+)

[PATCH 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.

Add alignment check to ext4_fill_super() 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>
---
 fs/ext4/super.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 304c712..90a8670 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3421,6 +3421,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 					"error: unsupported blocksize for dax");
 			goto failed_mount;
 		}
+		if (sb->s_bdev->bd_part->start_sect % (PAGE_SIZE / 512) ||
+		    sb->s_bdev->bd_part->nr_sects % (PAGE_SIZE / 512)) {
+			ext4_msg(sb, KERN_ERR,
+					"error: unaligned partition for dax");
+			goto failed_mount;
+		}
 		if (!sb->s_bdev->bd_disk->fops->direct_access) {
 			ext4_msg(sb, KERN_ERR,
 					"error: device does not support dax");

[PATCH 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.

Add alignment check to ext2_fill_super() 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>
---
 fs/ext2/super.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index b78caf2..34e32d9 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -927,6 +927,12 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
 					"error: unsupported blocksize for dax");
 			goto failed_mount;
 		}
+		if (sb->s_bdev->bd_part->start_sect % (PAGE_SIZE / 512) ||
+		    sb->s_bdev->bd_part->nr_sects % (PAGE_SIZE / 512)) {
+			ext2_msg(sb, KERN_ERR,
+					"error: unaligned partition for dax");
+			goto failed_mount;
+		}
 		if (!sb->s_bdev->bd_disk->fops->direct_access) {
 			ext2_msg(sb, KERN_ERR,
 					"error: device does not support dax");

[PATCH 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.

Add alignment check to xfs_fs_fill_super() when -o dax is
specified.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Dave Chinner <david@fromorbit.com> (supporter:XFS FILESYSTEM)
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 fs/xfs/xfs_super.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 187e14b..a17aebc 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1563,6 +1563,11 @@ xfs_fs_fill_super(
 			xfs_alert(mp,
 		"Filesystem block size invalid for DAX Turning DAX off.");
 			mp->m_flags &= ~XFS_MOUNT_DAX;
+		} else if (sb->s_bdev->bd_part->start_sect % (PAGE_SIZE / 512)
+		   || sb->s_bdev->bd_part->nr_sects % (PAGE_SIZE / 512)) {
+			xfs_alert(mp,
+		"Partition alignment 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.");

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

[Ross Zwisler]

On Fri, Apr 29, 2016 at 02:39:32PM -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.
> 
> Add alignment check to ext4, ext2, and xfs.
> 
> ---
> 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    | 6 ++++++
>  fs/ext4/super.c    | 6 ++++++
>  fs/xfs/xfs_super.c | 6 ++++++
>  3 files changed, 18 insertions(+)

For the series:
Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>

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

[Boaz Harrosh]

On 04/29/2016 11:39 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_fill_super() 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>
> ---
>  fs/ext4/super.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 304c712..90a8670 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3421,6 +3421,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  					"error: unsupported blocksize for dax");
>  			goto failed_mount;
>  		}
> +		if (sb->s_bdev->bd_part->start_sect % (PAGE_SIZE / 512) ||
> +		    sb->s_bdev->bd_part->nr_sects % (PAGE_SIZE / 512)) {

Can you please not do this like this? For me is a layering violation only
the device should know what are its limits.

I would prefer if you just try to bdev_direct_access() the 0 sector and if
it fails then fail here. This way you let the device decide what it needs
and if/how to support unaligned partitions.
(For example it could by shrinking its size)

Thanks
Boaz

> +			ext4_msg(sb, KERN_ERR,
> +					"error: unaligned partition for dax");
> +			goto failed_mount;
> +		}
>  		if (!sb->s_bdev->bd_disk->fops->direct_access) {
>  			ext4_msg(sb, KERN_ERR,
>  					"error: device does not support dax");
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
> 

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

[Christoph Hellwig]

On Fri, Apr 29, 2016 at 02:39:33PM -0600, Toshi Kani wrote:
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 304c712..90a8670 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3421,6 +3421,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  					"error: unsupported blocksize for dax");
>  			goto failed_mount;
>  		}
> +		if (sb->s_bdev->bd_part->start_sect % (PAGE_SIZE / 512) ||
> +		    sb->s_bdev->bd_part->nr_sects % (PAGE_SIZE / 512)) {
> +			ext4_msg(sb, KERN_ERR,
> +					"error: unaligned partition for dax");
> +			goto failed_mount;
> +		}
>  		if (!sb->s_bdev->bd_disk->fops->direct_access) {
>  			ext4_msg(sb, KERN_ERR,
>  					"error: device does not support dax");

Factor your new checks and the ->direct_access into a new helper. It
should take the block device as file systems might have multiple
underlying devices.

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

[Boaz Harrosh]

On 05/01/2016 08:31 PM, Christoph Hellwig wrote:
> On Fri, Apr 29, 2016 at 02:39:33PM -0600, Toshi Kani wrote:
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 304c712..90a8670 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -3421,6 +3421,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>>  					"error: unsupported blocksize for dax");
>>  			goto failed_mount;
>>  		}
>> +		if (sb->s_bdev->bd_part->start_sect % (PAGE_SIZE / 512) ||
>> +		    sb->s_bdev->bd_part->nr_sects % (PAGE_SIZE / 512)) {
>> +			ext4_msg(sb, KERN_ERR,
>> +					"error: unaligned partition for dax");
>> +			goto failed_mount;
>> +		}
>>  		if (!sb->s_bdev->bd_disk->fops->direct_access) {
>>  			ext4_msg(sb, KERN_ERR,
>>  					"error: device does not support dax");
> 
> Factor your new checks and the ->direct_access into a new helper. It
> should take the block device as file systems might have multiple
> underlying devices.

This already exists as part of bdev_direct_access()

All the code needs to do is call bdev_direct_access() on sector ZERO and
see if it is successful. If the alignment is broken it will fail.

Infact the FS does not even need the second if (!...bd_disk->fops->direct_access)
check either because it is also done inside bdev_direct_access(). The only check
needed is calling bdev_direct_access() and the generic layer does all the
checks needed already.

Cheers
Boaz

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

[Toshi Kani]

On Mon, 2016-05-02 at 12:49 +0300, Boaz Harrosh wrote:
> On 05/01/2016 08:31 PM, Christoph Hellwig wrote:
> > 
> > On Fri, Apr 29, 2016 at 02:39:33PM -0600, Toshi Kani wrote:
> > > 
> > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > > index 304c712..90a8670 100644
> > > --- a/fs/ext4/super.c
> > > +++ b/fs/ext4/super.c
> > > @@ -3421,6 +3421,12 @@ static int ext4_fill_super(struct super_block
> > > *sb, void *data, int silent)
> > >  					"error: unsupported
> > > blocksize for dax");
> > >  			goto failed_mount;
> > >  		}
> > > +		if (sb->s_bdev->bd_part->start_sect % (PAGE_SIZE /
> > > 512) ||
> > > +		    sb->s_bdev->bd_part->nr_sects % (PAGE_SIZE /
> > > 512)) {
> > > +			ext4_msg(sb, KERN_ERR,
> > > +					"error: unaligned partition
> > > for dax");
> > > +			goto failed_mount;
> > > +		}
> > >  		if (!sb->s_bdev->bd_disk->fops->direct_access) {
> > >  			ext4_msg(sb, KERN_ERR,
> > >  					"error: device does not
> > > support dax");
> >
> > Factor your new checks and the ->direct_access into a new helper. It
> > should take the block device as file systems might have multiple
> > underlying devices.
>
> This already exists as part of bdev_direct_access()
> 
> All the code needs to do is call bdev_direct_access() on sector ZERO and
> see if it is successful. If the alignment is broken it will fail.
> 
> Infact the FS does not even need the second if (!...bd_disk->fops-
> >direct_access) check either because it is also done inside
> bdev_direct_access(). The only check needed is calling
> bdev_direct_access() and the generic layer does all the
> checks needed already.

Sounds good. I will call bdev_direct_access() for this check. I assume we
do not need to check the alignment of 'bd_part->nr_sects' since the rest of
unaligned sectors will be ignored with block size 4KB. 

Thanks!
-Toshi