Add alignment check for DAX mount

blkdev_dax_capable() is similar to bdev_dax_supported(), but needs to remain as a separate interface for checking dax capability of a raw block device.

Rename and relocate blkdev_dax_capable() to keep them maintained consistently, and call bdev_direct_access() for the dax capability check.

There is no change in the behavior.

Link https://lkml.org/lkml/2016/5/9/950

Signed-off-by Toshi Kani <[email protected]>

Cc Alexander Viro <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Andreas Dilger <[email protected]>

Cc Jan Kara <[email protected]>

Cc Dave Chinner <[email protected]>

Cc Dan Williams <[email protected]>

Cc Ross Zwisler <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Boaz Harrosh <[email protected]>

---
 block/ioctl.c          |   30 ------------------------------
 fs/block_dev.c         |   39 +++++++++++++++++++++++++++++++++++++--
 include/linux/blkdev.h |    1 +
 include/linux/fs.h     |    8 --------
 4 files changed, 38 insertions(+), 40 deletions(-)

block/ioctl.c +0 -19

index 4ff1f92..7eeda07 100644
@@ -4,7 +4,6 @@
 #include <linux/gfp.h>
 #include <linux/blkpg.h>
 #include <linux/hdreg.h>
-#include <linux/badblocks.h>
 #include <linux/backing-dev.h>
 #include <linux/fs.h>
 #include <linux/blktrace_api.h>

@@ -407,35 +406,6 @@ static inline int is_unrecognized_ioctl(int ret)
                ret == -ENOIOCTLCMD;
 }
 
-#ifdef CONFIG_FS_DAX
-bool blkdev_dax_capable(struct block_device *bdev)
-{
-       struct gendisk *disk = bdev->bd_disk;
-
-       if (!disk->fops->direct_access)
-               return false;
-
-       /*
-        * If the partition is not aligned on a page boundary, we can't
-        * do dax I/O to it.
-        */
-       if ((bdev->bd_part->start_sect % (PAGE_SIZE / 512))
-                       || (bdev->bd_part->nr_sects % (PAGE_SIZE / 512)))
-               return false;
-
-       /*
-        * If the device has known bad blocks, force all I/O through the

Quoted 10 lines · Toshi Kani

blkdev_dax_capable() is similar to bdev_dax_supported(), but needs to remain as a separate interface for checking dax capability of a raw block device.

Rename and relocate blkdev_dax_capable() to keep them maintained consistently, and call bdev_direct_access() for the dax capability check.

There is no change in the behavior.

Quoted 44 lines

block/ioctl.c +0 -19

index 4ff1f92..7eeda07 100644
@@ -4,7 +4,6 @@
  #include <linux/gfp.h>
  #include <linux/blkpg.h>
  #include <linux/hdreg.h>
-#include <linux/badblocks.h>
  #include <linux/backing-dev.h>
  #include <linux/fs.h>
  #include <linux/blktrace_api.h>

@@ -407,35 +406,6 @@ static inline int is_unrecognized_ioctl(int ret)
                 ret == -ENOIOCTLCMD;
  }
 
-#ifdef CONFIG_FS_DAX
-bool blkdev_dax_capable(struct block_device *bdev)
-{
-       struct gendisk *disk = bdev->bd_disk;
-
-       if (!disk->fops->direct_access)
-               return false;
-
-       /*
-        * If the partition is not aligned on a page boundary, we can't
-        * do dax I/O to it.
-        */
-       if ((bdev->bd_part->start_sect % (PAGE_SIZE / 512))
-                       || (bdev->bd_part->nr_sects % (PAGE_SIZE /
 512)))
-               return false;
-
-       /*
-        * If the device has known bad blocks, force all I/O through

the
-        * driver / page cache.
-        *
-        * TODO: support finer grained dax error handling -        */
-       if (disk->bb && disk->bb->count)
-               return false;
-
-       return true;

block/ioctl.c

Quoted 2 lines

block/ioctl.c +0 -2

-}
-#endif

This will collide with my pending change to revert raw block device dax support, and also with Vishal's DAX error handling changes.  For coordination purposes I'm thining this should all go on top of the branch that Vishal is putting together with the dax zeroing changes from Jan and Christoph as well.

blkdev_dax_capable() is similar to bdev_dax_supported(), but needs to remain as a separate interface for checking dax capability of a raw block device.

Rename and relocate blkdev_dax_capable() to keep them maintained consistently, and call bdev_direct_access() for the dax capability check.

Quoted 8 lines · Toshi Kani

blkdev_dax_capable() is similar to bdev_dax_supported(), but needs to remain as a separate interface for checking dax capability of a raw block device.

Rename and relocate blkdev_dax_capable() to keep them maintained consistently, and call bdev_direct_access() for the dax capability check.

...

Quoted 1 line

fs/block_dev.c int bdev_dax_supported(struct super_block *sb, int blocksize) · line 570

Quoted 9 lines

fs/block_dev.c +9 -0

+bool bdev_dax_capable(struct block_device *bdev)
+{
+	struct gendisk *disk = bdev->bd_disk;
+	struct blk_dax_ctl dax = {
+		.size = PAGE_SIZE,
+	};
+
+	if (!IS_ENABLED(CONFIG_FS_DAX))
+		return false;

Frankly, I prefer the #ifdef CONFIG_FS_DAX and just compile the code out when DAX is not enabled (like it was with blkdev_dax_capable()). That way we don't grow the kernel for people who don't care about DAX.

Quoted 11 lines · Jan Kara

Quoted 8 lines · Toshi Kani

blkdev_dax_capable() is similar to bdev_dax_supported(), but needs to remain as a separate interface for checking dax capability of a raw block device.

Rename and relocate blkdev_dax_capable() to keep them maintained consistently, and call bdev_direct_access() for the dax capability check.

...

Quoted 1 line

Quoted 13 lines

fs/block_dev.c int bdev_dax_supported(struct super_block *sb, int blocksize) · line 570

Quoted 9 lines

fs/block_dev.c +9 -0

+bool bdev_dax_capable(struct block_device *bdev)
+{
+	struct gendisk *disk = bdev->bd_disk;
+	struct blk_dax_ctl dax = {
+		.size = PAGE_SIZE,
+	};
+
+	if (!IS_ENABLED(CONFIG_FS_DAX))
+		return false;

Frankly, I prefer the #ifdef CONFIG_FS_DAX and just compile the code out when DAX is not enabled (like it was with blkdev_dax_capable()). That way we don't grow the kernel for people who don't care about DAX.

When CONFIG_FS_DAX is not set, the rest of the code is optimized out. �So, I think the code size is the same.

(gdb) disas bdev_dax_capable
Dump of assembler code for function bdev_dax_capable:
���0xffffffff81260d20 <+0>:�����callq��0xffffffff81813c30 <__fentry__> ���0xffffffff81260d25 <+5>:�����push���%rbp ���0xffffffff81260d26 <+6>:�����xor����%eax,%eax ���0xffffffff81260d28 <+8>:�����mov����%rsp,%rbp ���0xffffffff81260d2b <+11>:����pop����%rbp ���0xffffffff81260d2c <+12>:����retq���
End of assembler dump.

blkdev_dax_capable() is similar to bdev_dax_supported(), but needs to remain as a separate interface for checking dax capability of a raw block device.

Rename and relocate blkdev_dax_capable() to keep them maintained consistently, and call bdev_direct_access() for the dax capability check.

There is no change in the behavior.

Link https://lkml.org/lkml/2016/5/9/950

Signed-off-by Toshi Kani <[email protected]>

Cc Alexander Viro <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Andreas Dilger <[email protected]>

Cc Jan Kara <[email protected]>

Cc Dave Chinner <[email protected]>

Cc Dan Williams <[email protected]>

Cc Ross Zwisler <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Boaz Harrosh <[email protected]>

---
 block/ioctl.c          |   30 ------------------------------
 fs/block_dev.c         |   39 +++++++++++++++++++++++++++++++++++++--
 include/linux/blkdev.h |    1 +
 include/linux/fs.h     |    8 --------
 4 files changed, 38 insertions(+), 40 deletions(-)

block/ioctl.c +0 -30

index 4ff1f92..7eeda07 100644
@@ -4,7 +4,6 @@
 #include <linux/gfp.h>
 #include <linux/blkpg.h>
 #include <linux/hdreg.h>
-#include <linux/badblocks.h>
 #include <linux/backing-dev.h>
 #include <linux/fs.h>
 #include <linux/blktrace_api.h>

@@ -407,35 +406,6 @@ static inline int is_unrecognized_ioctl(int ret)
                ret == -ENOIOCTLCMD;
 }
 
-#ifdef CONFIG_FS_DAX
-bool blkdev_dax_capable(struct block_device *bdev)
-{
-       struct gendisk *disk = bdev->bd_disk;
-
-       if (!disk->fops->direct_access)
-               return false;
-
-       /*
-        * If the partition is not aligned on a page boundary, we can't
-        * do dax I/O to it.
-        */
-       if ((bdev->bd_part->start_sect % (PAGE_SIZE / 512))
-                       || (bdev->bd_part->nr_sects % (PAGE_SIZE / 512)))
-               return false;
-
-       /*
-        * If the device has known bad blocks, force all I/O through the
-        * driver / page cache.
-        *
-        * TODO: support finer grained dax error handling
-        */
-       if (disk->bb && disk->bb->count)
-               return false;
-
-       return true;
-}
-#endif
-
 static int blkdev_flushbuf(struct block_device *bdev, fmode_t mode,
                unsigned cmd, unsigned long arg)
 {

fs/block_dev.c +12 -0

index db55638..5d79093 100644
@@ -29,6 +29,7 @@
 #include <linux/log2.h>
 #include <linux/cleancache.h>
 #include <linux/dax.h>
+#include <linux/badblocks.h>
 #include <asm/uaccess.h>
 #include "internal.h"
 

@@ -554,6 +555,40 @@ int bdev_dax_supported(struct super_block *sb, int blocksize)
 }
 EXPORT_SYMBOL_GPL(bdev_dax_supported);
 
+/**
+ * bdev_dax_capable() - Return if the raw device is capable for dax
+ * @bdev: The device for raw block device access
+ */
+bool bdev_dax_capable(struct block_device *bdev)
+{
+       struct gendisk *disk = bdev->bd_disk;
+       struct blk_dax_ctl dax = {
+               .size = PAGE_SIZE,
+       };
+

Quoted 5 lines · Toshi Kani

blkdev_dax_capable() is similar to bdev_dax_supported(), but needs to remain as a separate interface for checking dax capability of a raw block device.

Quoted 6 lines

Rename and relocate blkdev_dax_capable() to keep them maintained consistently, and call bdev_direct_access() for the dax capability check.

There is no change in the behavior.

Quoted 11 lines

fs/block_dev.c +11 -0

+/**
+ * bdev_dax_capable() - Return if the raw device is capable for dax
+ * @bdev: The device for raw block device access
+ */
+bool bdev_dax_capable(struct block_device *bdev)
+{
+       struct gendisk *disk = bdev->bd_disk;
+       struct blk_dax_ctl dax = {
+               .size = PAGE_SIZE,
+       };
+

fs/block_dev.c int bdev_dax_supported(struct super_block *sb, int blocksize) · line 578

Quoted 10 lines

fs/block_dev.c +10 -0

+       if (!IS_ENABLED(CONFIG_FS_DAX))
+               return false;
+
+       dax.sector = 0;
+       if (bdev_direct_access(bdev, &dax) < 0)
+               return false;
+
+       dax.sector = bdev->bd_part->nr_sects - (PAGE_SIZE / 512);
+       if (bdev_direct_access(bdev, &dax) < 0)
+               return false;

So I just noticed that this new implementation of bdev_dax_capable() prevents us from enabling DAX in the presence of errors, which was the goal of Vishal's pending patches for 4.7.

I like the goal of centralizing checks, but this collides the base DAX capability with the ability to perform an actual accesses at a given address.

All the immediate solutions that come to mind right now are pretty ugly, like an ignore errors flag...

Hmm, what about explicitly returning -EBADBLOCK and updating size to return the offset of the error?  That might be useful information to have in general...

The change from blkdev_dax_capable() to bdev_dax_capable() removed the only user of the former, so we now get a build warning:

fs/block_dev.c:1244:13: error: 'blkdev_dax_capable' defined but not used [-Werror=unused-function]
 static bool blkdev_dax_capable(struct block_device *bdev)

This removes the now-unused function.

Signed-off-by Arnd Bergmann <[email protected]>

Fixes a8078b1fc616 ("block: Update blkdev_dax_capable() for

consistency")

It's not entirely from the patch description what the intention was here in keeping two slightly different implementations of the same function in one file, my best guess is that it was not intentional and we should just remove this.

The change from blkdev_dax_capable() to bdev_dax_capable() removed the only user of the former, so we now get a build warning:

fs/block_dev.c:1244:13: error: 'blkdev_dax_capable' defined but not used [-Werror=unused-function]
static bool blkdev_dax_capable(struct block_device *bdev)

This removes the now-unused function.

Signed-off-by Arnd Bergmann <[email protected]>

Fixes a8078b1fc616 ("block: Update blkdev_dax_capable() for consistency")

---

Quoted 2 lines · On Tuesday 10 May 2016 10:23:57 Toshi Kani wrote:

(quoted diff) +0 -0

@@ -1295,7 +1330,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 

Review static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part) · line 1335

Quoted 6 lines

(quoted diff) +1 -1

                        if (!ret) {
                                bd_set_size(bdev,(loff_t)get_capacity(disk)<<9);
-                               if (!blkdev_dax_capable(bdev))
+                               if (!bdev_dax_capable(bdev))
                                        bdev->bd_inode->i_flags &= ~S_DAX;
 

It's not entirely from the patch description what the intention was here in keeping two slightly different implementations of the same function in one file, my best guess is that it was not intentional and we should just remove this.

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.

- Patch 1-2 add bdev_dax_supported() which performs all the checks necessary for dax mount.
- Patch 3-5 change fillesystems to call bdev_dax_supported().
- Patch 6 is a cleanup to keep dax capability checks consistent.

v4:
- blkdev_dax_capable() is similar to bdev_dax_supported(). Manage them consistently. (Dan Williams, Dave Chinner)

v3:
- Remove boilerplate code from filesytems (Christoph Hellwig)
- Add a helper function to perform all checks (Dave Chinner)

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 (6):
1/6 block: Add vfs_msg() interface
2/6 block: Add bdev_dax_supported() for dax mount checks 3/6 ext4: Add alignment check for DAX mount
4/6 ext2: Add alignment check for DAX mount
5/6 xfs: Add alignment check for DAX mount
6/6 block: Update blkdev_dax_capable() for consistency

---
 block/ioctl.c          | 30 ----------------
 fs/block_dev.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/ext2/super.c        | 11 ++----
 fs/ext4/super.c        | 11 ++----
 fs/xfs/xfs_super.c     | 12 +++----
 include/linux/blkdev.h | 13 +++++++
 include/linux/fs.h     |  8 -----
 7 files changed, 116 insertions(+), 65 deletions(-)

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

When a partition is not aligned by 4KB, mount -o dax succeeds,