thr3ads.net - Linux Virtualization - [Qemu-devel] [PATCH v4 5/5] xfs: disable map

If this information is useful, please help other people find it:
Share via:

Pankaj Gupta

2019-Apr-04 09:09 UTC

[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.
> 
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

Jan Kara

2019-Apr-04 09:40 UTC

head link

[PATCH v4 5/5] xfs: disable map_sync for async flush

On Thu 04-04-19 05:09:10, Pankaj Gupta wrote:> 
> > > 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?
So I would put the helper in include/linux/dax.h and have it like:

bool daxdev_mapping_supported(struct vm_area_struct *vma,
			      struct dax_device *dax_dev)
{
	if (!(vma->vm_flags & VM_SYNC))
		return true;
	if (!IS_DAX(file_inode(vma->vm_file)))
		return false;
	return dax_synchronous(dax_dev);
}

								Honza> 
> ================> 
> 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
> 
> 
> -- 
Jan Kara <jack at suse.com>
SUSE Labs, CR

Pankaj Gupta

2019-Apr-04 10:08 UTC

head link

[Qemu-devel] [PATCH v4 5/5] xfs: disable map_sync for async flush

> On Thu 04-04-19 05:09:10, Pankaj Gupta wrote:
> > 
> > > > 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?
> 
> So I would put the helper in include/linux/dax.h and have it like:
> 
> bool daxdev_mapping_supported(struct vm_area_struct *vma,
> 			      struct dax_device *dax_dev)
> {
> 	if (!(vma->vm_flags & VM_SYNC))
> 		return true;
> 	if (!IS_DAX(file_inode(vma->vm_file)))
> 		return false;
> 	return dax_synchronous(dax_dev);
> }
Sure. This is much better. I was also not sure what to name the helper function.
I will go ahead with this unless 'Dave' & 'Darrick' have
anything to add.

Thank you very much.

Best regards,
Pankaj 
> 
> 								Honza
> > 
> > ================> > 
> > 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
> > 
> > 
> > 
> --
> Jan Kara <jack at suse.com>
> SUSE Labs, CR
> 
>

Reasonably Related Threads

Search for more seemingly similar threads

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

[PATCH v4 5/5] xfs: disable map_sync for async flush

[PATCH v4 5/5] xfs: disable map_sync for async flush

[Qemu-devel] [PATCH v4 5/5] xfs: disable map_sync for async flush

Reasonably Related Threads