Re: [RFC PATCH 2/2] block: enable dax for raw block devices

[Ross Zwisler <ross.zwisler@linux.intel.com>]

On Fri, Oct 16, 2015 at 08:49:41PM -0400, Dan Williams wrote:
> If an application wants exclusive access to all of the persistent memory
> provided by an NVDIMM namespace it can use this raw-block-dax facility
> to forgo establishing a filesystem.  This capability is targeted
> primarily to hypervisors wanting to provision persistent memory for
> guests.
> 
> Cc: Jeff Moyer <jmoyer@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> 
> Only lighted tested so far, but seems to work, is the shortest path to a
> DAX mapping, and makes it easier to trigger the pmd_fault path (no
> fs-block-allocator interactions).
> 
>  fs/block_dev.c |   84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 5277dd83d254..498b71455570 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1687,13 +1687,95 @@ static const struct address_space_operations def_blk_aops = {
>  	.is_dirty_writeback = buffer_check_dirty_writeback,
>  };
>  
> +#ifdef CONFIG_FS_DAX
> +static int blkdev_dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> +{
> +	struct inode *bd_inode = file_bd_inode(vma->vm_file);
> +	struct block_device *bdev = I_BDEV(bd_inode);
> +	int ret;
> +
> +	mutex_lock(&bdev->bd_mutex);
> +	ret = __dax_fault(vma, vmf, blkdev_get_block, NULL);
> +	mutex_unlock(&bdev->bd_mutex);
> +
> +	return ret;
> +}

This all looks very straightforward.  The one comment I have is that this
code is missing the calls to sb_[start|end]_pagefault(), and to
file_update_time() that are found in ext[24]/xfs and the generic fault code.

The previous version of this code used the generic fault implementation, and
was calling these functions via filemap_page_mkwrite().

It is possible that they were omitted for a reason - does protection from
filesystem freezing still make sense when talking with a raw block device?
For example, if that block device *has* a mounted filesystem on it that is
frozen, does sb_start_pagefault() prevent against page faults on the raw
device that try and make something writable?  

In any case, the presence of them in filemap_page_mkwrite() tells me that they
at least aren't harmful, and I wanted to make sure they weren't needed before
leaving them out.  If the omission was intentional, should we add a comment to
explain why they are missing?