Re: [PATCH v4 6/6] block: Update blkdev_dax_capable() for consistency

[Dan Williams]

On Tue, May 10, 2016 at 9:23 AM, Toshi Kani <toshi.kani@xxxxxxx> wrote:
> 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 <toshi.kani@xxxxxxx>
> Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: Jens Axboe <axboe@xxxxxx>
> Cc: Andreas Dilger <adilger.kernel@xxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> Cc: Dave Chinner <david@xxxxxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Cc: Boaz Harrosh <boaz@xxxxxxxxxxxxx>
> ---
>  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(-)
>
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 4ff1f92..7eeda07 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -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)
>  {
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index db55638..5d79093 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -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,
> +       };
> +
> +       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...