Linux Virtualization - Apr 2019 - [PATCH v4 5/5] xfs: disable map_sync for async flush

On Thu, Apr 04, 2019 at 09:09:12AM +1100, Dave Chinner
wrote:
> On Wed, Apr 03, 2019 at 04:10:18PM +0530, Pankaj Gupta wrote:
> > Virtio pmem provides asynchronous host page cache flush
> > mechanism. we don't support 'MAP_SYNC' with virtio pmem 
> > and xfs.
> > 
> > Signed-off-by: Pankaj Gupta <pagupta at redhat.com>
> > ---
> >  fs/xfs/xfs_file.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index 1f2e2845eb76..dced2eb8c91a 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1203,6 +1203,14 @@ xfs_file_mmap(
> >  	if (!IS_DAX(file_inode(filp)) && (vma->vm_flags &
VM_SYNC))
> >  		return -EOPNOTSUPP;
> >  
> > +	/* We don't support synchronous mappings with DAX files if
> > +	 * dax_device is not synchronous.
> > +	 */
> > +	if (IS_DAX(file_inode(filp)) && !dax_synchronous(
> > +		xfs_find_daxdev_for_inode(file_inode(filp))) &&
> > +					(vma->vm_flags & VM_SYNC))
> > +		return -EOPNOTSUPP;
> > +
> >  	file_accessed(filp);
> >  	vma->vm_ops = &xfs_file_vm_ops;
> >  	if (IS_DAX(file_inode(filp)))
> 
> All this ad hoc IS_DAX conditional logic is getting pretty nasty.
> 
> xfs_file_mmap(
> ....
> {
> 	struct inode	*inode = file_inode(filp);
> 
> 	if (vma->vm_flags & VM_SYNC) {
> 		if (!IS_DAX(inode))
> 			return -EOPNOTSUPP;
> 		if (!dax_synchronous(xfs_find_daxdev_for_inode(inode))
> 			return -EOPNOTSUPP;
> 	}
> 
> 	file_accessed(filp);
> 	vma->vm_ops = &xfs_file_vm_ops;
> 	if (IS_DAX(inode))
> 		vma->vm_flags |= VM_HUGEPAGE;
> 	return 0;
> }
> 
> 
> Even better, factor out all the "MAP_SYNC supported" checks into
a
> helper so that the filesystem code just doesn't have to care about
> the details of checking for DAX+MAP_SYNC support....
Seconded, since ext4 has nearly the same flag validation logic.

--D
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david at fromorbit.com

> > On Thu, Apr 04, 2019 at 09:09:12AM +1100, Dave Chinner wrote:
> > > On Wed, Apr 03, 2019 at 04:10:18PM +0530, Pankaj Gupta wrote:
> > > > Virtio pmem provides asynchronous host page cache flush
> > > > mechanism. we don't support 'MAP_SYNC' with
virtio pmem
> > > > and xfs.
> > > > 
> > > > Signed-off-by: Pankaj Gupta <pagupta at redhat.com>
> > > > ---
> > > >  fs/xfs/xfs_file.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > > index 1f2e2845eb76..dced2eb8c91a 100644
> > > > --- a/fs/xfs/xfs_file.c
> > > > +++ b/fs/xfs/xfs_file.c
> > > > @@ -1203,6 +1203,14 @@ xfs_file_mmap(
> > > >  	if (!IS_DAX(file_inode(filp)) && (vma->vm_flags
& VM_SYNC))
> > > >  		return -EOPNOTSUPP;
> > > >  
> > > > +	/* We don't support synchronous mappings with DAX
files if
> > > > +	 * dax_device is not synchronous.
> > > > +	 */
> > > > +	if (IS_DAX(file_inode(filp)) && !dax_synchronous(
> > > > +		xfs_find_daxdev_for_inode(file_inode(filp))) &&
> > > > +					(vma->vm_flags & VM_SYNC))
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > >  	file_accessed(filp);
> > > >  	vma->vm_ops = &xfs_file_vm_ops;
> > > >  	if (IS_DAX(file_inode(filp)))
> > > 
> > > All this ad hoc IS_DAX conditional logic is getting pretty nasty.
> > > 
> > > xfs_file_mmap(
> > > ....
> > > {
> > > 	struct inode	*inode = file_inode(filp);
> > > 
> > > 	if (vma->vm_flags & VM_SYNC) {
> > > 		if (!IS_DAX(inode))
> > > 			return -EOPNOTSUPP;
> > > 		if (!dax_synchronous(xfs_find_daxdev_for_inode(inode))
> > > 			return -EOPNOTSUPP;
> > > 	}
> > > 
> > > 	file_accessed(filp);
> > > 	vma->vm_ops = &xfs_file_vm_ops;
> > > 	if (IS_DAX(inode))
> > > 		vma->vm_flags |= VM_HUGEPAGE;
> > > 	return 0;
> > > }
> > > 
> > > 
> > > Even better, factor out all the "MAP_SYNC supported"
checks into a
> > > helper so that the filesystem code just doesn't have to care
about
> > > the details of checking for DAX+MAP_SYNC support....
> > 
> > Seconded, since ext4 has nearly the same flag validation logic.
> 
Only issue with this I see is we need the helper function only for supported
filesystems ext4 & xfs (right now). If I create the function in
"fs.h" it
will be compiled for every filesystem, even for those don't need it.  

Sample patch below, does below patch is near to what you have in mind?

================
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 1f2e2845eb76..614995170cac 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1196,12 +1196,17 @@ xfs_file_mmap(
        struct file     *filp,
        struct vm_area_struct *vma)
 {
+       struct dax_device *dax_dev =
xfs_find_daxdev_for_inode(file_inode(filp));
+
        /*
-        * We don't support synchronous mappings for non-DAX files. At least
-        * until someone comes with a sensible use case.
+        * We don't support synchronous mappings for non-DAX files and
+        * for DAX files if underneath dax_device is not synchronous.
         */
-       if (!IS_DAX(file_inode(filp)) && (vma->vm_flags &
VM_SYNC))
-               return -EOPNOTSUPP;
+       if (vma->vm_flags & VM_SYNC) {
+               int err = is_synchronous(filp, dax_dev);
+               if (err)
+                       return err;
+       }
 
        file_accessed(filp);
        vma->vm_ops = &xfs_file_vm_ops;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 8b42df09b04c..add017de3dd7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2162,6 +2162,20 @@ static inline void file_accessed(struct file *file)
                touch_atime(&file->f_path);
 }
 
+struct dax_device;
+extern bool dax_synchronous(struct dax_device *dax_dev);
+static inline int is_synchronous(struct file *filp, struct dax_device *dax_dev)
+{
+       struct inode *inode = file_inode(filp);
+
+       if (!IS_DAX(inode))
+               return -EOPNOTSUPP;
+       if (!dax_synchronous(dax_dev))
+               return -EOPNOTSUPP;
+
+       return 0;
+}
+
 int sync_inode(struct inode *inode, struct writeback_control *wbc);
 int sync_inode_metadata(struct inode *inode, int wait);
 
---------

Thanks,
Pankaj