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

[Williams, Dan J]

On Thu, 2015-10-22 at 11:35 +0200, Jan Kara wrote:
> On Thu 22-10-15 02:42:11, 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: Jan Kara <jack@xxxxxxx>
> > Cc: Jeff Moyer <jmoyer@xxxxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxx>
> > Cc: Dave Chinner <david@xxxxxxxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > ---
> >  fs/block_dev.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/block_dev.c b/fs/block_dev.c
> > index 3255dcec96b4..c27cd1a21a13 100644
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -1687,13 +1687,65 @@ static const struct address_space_operations def_blk_aops = {
> >  	.is_dirty_writeback = buffer_check_dirty_writeback,
> >  };
> >  
> > +#ifdef CONFIG_FS_DAX
> > +/*
> > + * In the raw block case we do not need to contend with truncation nor
> > + * unwritten file extents.  Without those concerns there is no need for
> > + * additional locking beyond the mmap_sem context that these routines
> > + * are already executing under.
> > + *
> > + * Note, there is no protection if the block device is dynamically
> > + * resized (partition grow/shrink) during a fault. A stable block device
> > + * size is already not enforced in the blkdev_direct_IO path.
> > + *
> > + * For DAX, it is the responsibility of the block device driver to
> > + * ensure the whole-disk device size is stable while requests are in
> > + * flight.
> > + *
> > + * Finally, these paths do not synchronize against freezing
> > + * (sb_start_pagefault(), etc...) since bdev_sops does not support
> > + * freezing.
> 
> Well, for devices freezing is handled directly in the block layer code
> (blk_stop_queue()) since there's no need to put some metadata structures
> into a consistent state. So the comment about bdev_sops is somewhat
> strange.

This text was aimed at the request from Ross to document the differences
vs the generic_file_mmap() path.  Is the following incremental change
more clear?

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 840acd4380d4..4ae8fa55bd1e 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1702,9 +1702,15 @@ static const struct address_space_operations def_blk_aops = {
  * ensure the whole-disk device size is stable while requests are in
  * flight.
  *
- * Finally, these paths do not synchronize against freezing
- * (sb_start_pagefault(), etc...) since bdev_sops does not support
- * freezing.
+ * Finally, in contrast to the generic_file_mmap() path, there are no
+ * calls to sb_start_pagefault().  That is meant to synchronize write
+ * faults against requests to freeze the contents of the filesystem
+ * hosting vma->vm_file.  However, in the case of a block device special
+ * file, it is a 0-sized device node usually hosted on devtmpfs, i.e.
+ * nothing to do with the super_block for bdev_file_inode(vma->vm_file).
+ * We could call get_super() in this path to retrieve the right
+ * super_block, but the generic_file_mmap() path does not do this for
+ * the CONFIG_FS_DAX=n case.
  */
 static int blkdev_dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {