> > 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
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
[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 > >
Apparently Analagous Threads
- [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
- [PATCH v4 5/5] xfs: disable map_sync for async flush
- [PATCH v4 5/5] xfs: disable map_sync for async flush