| Message ID | 20190403104018.23947-6-pagupta@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | virtio pmem driver | expand |
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@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.... Cheers, Dave.
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@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@fromorbit.com
Hi Dave, > > 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@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; > } Sure, this is better. > > > 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.... o.k. Will add one common helper function for both ext4 & xfs filesystems. Thanks for the suggestion. Best regards, Pankaj > > Cheers, > > Dave. > -- > Dave Chinner > david@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@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. Agree. Thanks, Pankaj > > --D > > > > > Cheers, > > > > Dave. > > -- > > Dave Chinner > > david@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@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@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 > > >
On Thu, Apr 04, 2019 at 02:12:30AM -0400, Pankaj Gupta wrote: > > 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; > > } > > Sure, this is better. > > 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.... > > o.k. Will add one common helper function for both ext4 & xfs filesystems. Note this pending patch for Goldwyn Rodrigues' patchset for btrfs: https://lore.kernel.org/linux-btrfs/20190328102418.5466-1-kilobyte@angband.pl/ We might want to coordinate. Meow!
> 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@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@suse.com> > SUSE Labs, CR > >
> > On Thu, Apr 04, 2019 at 02:12:30AM -0400, Pankaj Gupta wrote: > > > 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; > > > } > > > > Sure, this is better. > > > > 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.... > > > > o.k. Will add one common helper function for both ext4 & xfs filesystems. > > Note this pending patch for Goldwyn Rodrigues' patchset for btrfs: > > https://lore.kernel.org/linux-btrfs/20190328102418.5466-1-kilobyte@angband.pl/ > > We might want to coordinate. Sure. Good to know. Thanks for the pointer. Will have a look. Best regards, Pankaj > > > Meow! > -- > ⢀⣴⠾⠻⢶⣦⠀ > ⣾⠁⢠⠒⠀⣿⡁ Did ya know that typing "test -j8" instead of "ctest -j8" > ⢿⡄⠘⠷⠚⠋⠀ will make your testsuite pass much faster, and fix bugs? > ⠈⠳⣄⠀⠀⠀⠀ >
On Thu, Apr 04, 2019 at 06:08:44AM -0400, Pankaj Gupta wrote: > > > 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@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, Should this be static inline if you're putting it in the header file? A comment ought to be added to describe what this predicate function does. > > 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. Jan's approach (modulo that one comment) looks good to me. --D > 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@suse.com> > > SUSE Labs, CR > > > >
> > > > > > So I would put the helper in include/linux/dax.h and have it like: > > > > > > bool daxdev_mapping_supported(struct vm_area_struct *vma, > > Should this be static inline if you're putting it in the header file? yes. Thanks. > > A comment ought to be added to describe what this predicate function > does. Sure, will add a comment describing the function. > > > > 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. > > Jan's approach (modulo that one comment) looks good to me. Sure. Thank you. Best regards, Pankaj > > --D >
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)))
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@redhat.com> --- fs/xfs/xfs_file.c | 8 ++++++++ 1 file changed, 8 insertions(+)