| Message ID | 20210408120432.1063608-8-ruansy.fnst@fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fsdax,xfs: Add reflink&dedupe support for fsdax | expand |
On Thu 08 Apr 2021 at 20:04, Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > Add xfs_break_two_dax_layouts() to break layout for tow dax > files. Then > call compare range function only when files are both DAX or not. > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> > Not family with xfs code but reading code make my sleep better :) See bellow. > --- > fs/xfs/xfs_file.c | 20 ++++++++++++++++++++ > fs/xfs/xfs_inode.c | 8 +++++++- > fs/xfs/xfs_inode.h | 1 + > fs/xfs/xfs_reflink.c | 5 +++-- > 4 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 5795d5d6f869..1fd457167c12 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -842,6 +842,26 @@ xfs_break_dax_layouts( > 0, 0, xfs_wait_dax_page(inode)); > } > > +int > +xfs_break_two_dax_layouts( > + struct inode *src, > + struct inode *dest) > +{ > + int error; > + bool retry = false; > + > +retry: > 'retry = false;' ? since xfs_break_dax_layouts() won't set retry to false if there is no busy page in inode->i_mapping. Dead loop will happen if retry is true once. > + error = xfs_break_dax_layouts(src, &retry); > + if (error || retry) > + goto retry; > + > + error = xfs_break_dax_layouts(dest, &retry); > + if (error || retry) > + goto retry; > + > + return error; > +} > + > int > xfs_break_layouts( > struct inode *inode, > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index f93370bd7b1e..c01786917eef 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -3713,8 +3713,10 @@ xfs_ilock2_io_mmap( > struct xfs_inode *ip2) > { > int ret; > + struct inode *inode1 = VFS_I(ip1); > + struct inode *inode2 = VFS_I(ip2); > > - ret = xfs_iolock_two_inodes_and_break_layout(VFS_I(ip1), > VFS_I(ip2)); > + ret = xfs_iolock_two_inodes_and_break_layout(inode1, inode2); > if (ret) > return ret; > if (ip1 == ip2) > @@ -3722,6 +3724,10 @@ xfs_ilock2_io_mmap( > else > xfs_lock_two_inodes(ip1, XFS_MMAPLOCK_EXCL, > ip2, XFS_MMAPLOCK_EXCL); > + > + if (IS_DAX(inode1) && IS_DAX(inode2)) > + ret = xfs_break_two_dax_layouts(inode1, inode2); > + ret is ignored here. -- Su > return 0; > } > > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index 88ee4c3930ae..5ef21924dddc 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -435,6 +435,7 @@ enum xfs_prealloc_flags { > > int xfs_update_prealloc_flags(struct xfs_inode *ip, > enum xfs_prealloc_flags flags); > +int xfs_break_two_dax_layouts(struct inode *inode1, struct > inode *inode2); > int xfs_break_layouts(struct inode *inode, uint *iolock, > enum layout_break_reason reason); > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c > index a4cd6e8a7aa0..4426bcc8a985 100644 > --- a/fs/xfs/xfs_reflink.c > +++ b/fs/xfs/xfs_reflink.c > @@ -29,6 +29,7 @@ > #include "xfs_iomap.h" > #include "xfs_sb.h" > #include "xfs_ag_resv.h" > +#include <linux/dax.h> > > /* > * Copy on Write of Shared Blocks > @@ -1324,8 +1325,8 @@ xfs_reflink_remap_prep( > if (XFS_IS_REALTIME_INODE(src) || XFS_IS_REALTIME_INODE(dest)) > goto out_unlock; > > - /* Don't share DAX file data for now. */ > - if (IS_DAX(inode_in) || IS_DAX(inode_out)) > + /* Don't share DAX file data with non-DAX file. */ > + if (IS_DAX(inode_in) != IS_DAX(inode_out)) > goto out_unlock; > > if (!IS_DAX(inode_in))
On Thu, Apr 08, 2021 at 08:04:32PM +0800, Shiyang Ruan wrote: > Add xfs_break_two_dax_layouts() to break layout for tow dax files. Then > call compare range function only when files are both DAX or not. > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> > --- > fs/xfs/xfs_file.c | 20 ++++++++++++++++++++ > fs/xfs/xfs_inode.c | 8 +++++++- > fs/xfs/xfs_inode.h | 1 + > fs/xfs/xfs_reflink.c | 5 +++-- > 4 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 5795d5d6f869..1fd457167c12 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -842,6 +842,26 @@ xfs_break_dax_layouts( > 0, 0, xfs_wait_dax_page(inode)); > } > > +int > +xfs_break_two_dax_layouts( > + struct inode *src, > + struct inode *dest) > +{ > + int error; > + bool retry = false; > + > +retry: > + error = xfs_break_dax_layouts(src, &retry); > + if (error || retry) > + goto retry; > + > + error = xfs_break_dax_layouts(dest, &retry); > + if (error || retry) > + goto retry; > + > + return error; > +} > + > int > xfs_break_layouts( > struct inode *inode, > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index f93370bd7b1e..c01786917eef 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -3713,8 +3713,10 @@ xfs_ilock2_io_mmap( > struct xfs_inode *ip2) > { > int ret; > + struct inode *inode1 = VFS_I(ip1); > + struct inode *inode2 = VFS_I(ip2); > > - ret = xfs_iolock_two_inodes_and_break_layout(VFS_I(ip1), VFS_I(ip2)); > + ret = xfs_iolock_two_inodes_and_break_layout(inode1, inode2); > if (ret) > return ret; > if (ip1 == ip2) > @@ -3722,6 +3724,10 @@ xfs_ilock2_io_mmap( > else > xfs_lock_two_inodes(ip1, XFS_MMAPLOCK_EXCL, > ip2, XFS_MMAPLOCK_EXCL); > + > + if (IS_DAX(inode1) && IS_DAX(inode2)) > + ret = xfs_break_two_dax_layouts(inode1, inode2); This is wrong on many levels. The first problem is that xfs_break_two_dax_layouts calls xfs_break_dax_layouts twice even if inode1 == inode2, which is unnecessary. The second problem is that xfs_break_dax_layouts can cycle the MMAPLOCK on the inode that it's processing. Since there are two inodes in play here, you must be /very/ careful about maintaining correct locking order, which for the MMAPLOCK is increasing order of xfs_inode.i_ino. If you drop the MMAPLOCK for the lower-numbered inode for any reason, you have to drop both MMAPLOCKs and try again. In other words, you have to replace all that nice MMAPLOCK code with a new xfs_mmaplock_two_inodes_and_break_dax_layouts function that is structured similarly to what xfs_iolock_two_inodes_and_break_layout does for the IOLOCK and PNFS layouts. > + > return 0; > } > > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index 88ee4c3930ae..5ef21924dddc 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -435,6 +435,7 @@ enum xfs_prealloc_flags { > > int xfs_update_prealloc_flags(struct xfs_inode *ip, > enum xfs_prealloc_flags flags); > +int xfs_break_two_dax_layouts(struct inode *inode1, struct inode *inode2); > int xfs_break_layouts(struct inode *inode, uint *iolock, > enum layout_break_reason reason); > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c > index a4cd6e8a7aa0..4426bcc8a985 100644 > --- a/fs/xfs/xfs_reflink.c > +++ b/fs/xfs/xfs_reflink.c > @@ -29,6 +29,7 @@ > #include "xfs_iomap.h" > #include "xfs_sb.h" > #include "xfs_ag_resv.h" > +#include <linux/dax.h> Why is this necessary? --D > > /* > * Copy on Write of Shared Blocks > @@ -1324,8 +1325,8 @@ xfs_reflink_remap_prep( > if (XFS_IS_REALTIME_INODE(src) || XFS_IS_REALTIME_INODE(dest)) > goto out_unlock; > > - /* Don't share DAX file data for now. */ > - if (IS_DAX(inode_in) || IS_DAX(inode_out)) > + /* Don't share DAX file data with non-DAX file. */ > + if (IS_DAX(inode_in) != IS_DAX(inode_out)) > goto out_unlock; > > if (!IS_DAX(inode_in)) > -- > 2.31.0 > > >
> -----Original Message----- > From: Su Yue <l@damenly.su> > Subject: Re: [PATCH v4 7/7] fs/xfs: Add dedupe support for fsdax > > > On Thu 08 Apr 2021 at 20:04, Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > > > Add xfs_break_two_dax_layouts() to break layout for tow dax files. > > Then call compare range function only when files are both DAX or not. > > > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> > > > Not family with xfs code but reading code make my sleep better :) See bellow. > > > --- > > fs/xfs/xfs_file.c | 20 ++++++++++++++++++++ > > fs/xfs/xfs_inode.c | 8 +++++++- > > fs/xfs/xfs_inode.h | 1 + > > fs/xfs/xfs_reflink.c | 5 +++-- > > 4 files changed, 31 insertions(+), 3 deletions(-) > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index > > 5795d5d6f869..1fd457167c12 100644 > > --- a/fs/xfs/xfs_file.c > > +++ b/fs/xfs/xfs_file.c > > @@ -842,6 +842,26 @@ xfs_break_dax_layouts( > > 0, 0, xfs_wait_dax_page(inode)); > > } > > > > +int > > +xfs_break_two_dax_layouts( > > + struct inode *src, > > + struct inode *dest) > > +{ > > + int error; > > + bool retry = false; > > + > > +retry: > > > 'retry = false;' ? since xfs_break_dax_layouts() won't set retry to false if there is > no busy page in inode->i_mapping. > Dead loop will happen if retry is true once. Yes, I should move 'retry=false;' under the retry label. > > > + error = xfs_break_dax_layouts(src, &retry); > > + if (error || retry) > > + goto retry; > > + > > + error = xfs_break_dax_layouts(dest, &retry); > > + if (error || retry) > > + goto retry; > > + > > + return error; > > +} > > + > > int > > xfs_break_layouts( > > struct inode *inode, > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index > > f93370bd7b1e..c01786917eef 100644 > > --- a/fs/xfs/xfs_inode.c > > +++ b/fs/xfs/xfs_inode.c > > @@ -3713,8 +3713,10 @@ xfs_ilock2_io_mmap( > > struct xfs_inode *ip2) > > { > > int ret; > > + struct inode *inode1 = VFS_I(ip1); > > + struct inode *inode2 = VFS_I(ip2); > > > > - ret = xfs_iolock_two_inodes_and_break_layout(VFS_I(ip1), > > VFS_I(ip2)); > > + ret = xfs_iolock_two_inodes_and_break_layout(inode1, inode2); > > if (ret) > > return ret; > > if (ip1 == ip2) > > @@ -3722,6 +3724,10 @@ xfs_ilock2_io_mmap( > > else > > xfs_lock_two_inodes(ip1, XFS_MMAPLOCK_EXCL, > > ip2, XFS_MMAPLOCK_EXCL); > > + > > + if (IS_DAX(inode1) && IS_DAX(inode2)) > > + ret = xfs_break_two_dax_layouts(inode1, inode2); > > + > ret is ignored here. Thanks, it's my mistake. -- Thanks, Ruan Shiyang. > > -- > Su > > return 0; > > } > > > > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index > > 88ee4c3930ae..5ef21924dddc 100644 > > --- a/fs/xfs/xfs_inode.h > > +++ b/fs/xfs/xfs_inode.h > > @@ -435,6 +435,7 @@ enum xfs_prealloc_flags { > > > > int xfs_update_prealloc_flags(struct xfs_inode *ip, > > enum xfs_prealloc_flags flags); > > +int xfs_break_two_dax_layouts(struct inode *inode1, struct > > inode *inode2); > > int xfs_break_layouts(struct inode *inode, uint *iolock, > > enum layout_break_reason reason); > > > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c index > > a4cd6e8a7aa0..4426bcc8a985 100644 > > --- a/fs/xfs/xfs_reflink.c > > +++ b/fs/xfs/xfs_reflink.c > > @@ -29,6 +29,7 @@ > > #include "xfs_iomap.h" > > #include "xfs_sb.h" > > #include "xfs_ag_resv.h" > > +#include <linux/dax.h> > > > > /* > > * Copy on Write of Shared Blocks > > @@ -1324,8 +1325,8 @@ xfs_reflink_remap_prep( > > if (XFS_IS_REALTIME_INODE(src) || XFS_IS_REALTIME_INODE(dest)) > > goto out_unlock; > > > > - /* Don't share DAX file data for now. */ > > - if (IS_DAX(inode_in) || IS_DAX(inode_out)) > > + /* Don't share DAX file data with non-DAX file. */ > > + if (IS_DAX(inode_in) != IS_DAX(inode_out)) > > goto out_unlock; > > > > if (!IS_DAX(inode_in))
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 5795d5d6f869..1fd457167c12 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -842,6 +842,26 @@ xfs_break_dax_layouts( 0, 0, xfs_wait_dax_page(inode)); } +int +xfs_break_two_dax_layouts( + struct inode *src, + struct inode *dest) +{ + int error; + bool retry = false; + +retry: + error = xfs_break_dax_layouts(src, &retry); + if (error || retry) + goto retry; + + error = xfs_break_dax_layouts(dest, &retry); + if (error || retry) + goto retry; + + return error; +} + int xfs_break_layouts( struct inode *inode, diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index f93370bd7b1e..c01786917eef 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -3713,8 +3713,10 @@ xfs_ilock2_io_mmap( struct xfs_inode *ip2) { int ret; + struct inode *inode1 = VFS_I(ip1); + struct inode *inode2 = VFS_I(ip2); - ret = xfs_iolock_two_inodes_and_break_layout(VFS_I(ip1), VFS_I(ip2)); + ret = xfs_iolock_two_inodes_and_break_layout(inode1, inode2); if (ret) return ret; if (ip1 == ip2) @@ -3722,6 +3724,10 @@ xfs_ilock2_io_mmap( else xfs_lock_two_inodes(ip1, XFS_MMAPLOCK_EXCL, ip2, XFS_MMAPLOCK_EXCL); + + if (IS_DAX(inode1) && IS_DAX(inode2)) + ret = xfs_break_two_dax_layouts(inode1, inode2); + return 0; } diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index 88ee4c3930ae..5ef21924dddc 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -435,6 +435,7 @@ enum xfs_prealloc_flags { int xfs_update_prealloc_flags(struct xfs_inode *ip, enum xfs_prealloc_flags flags); +int xfs_break_two_dax_layouts(struct inode *inode1, struct inode *inode2); int xfs_break_layouts(struct inode *inode, uint *iolock, enum layout_break_reason reason); diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c index a4cd6e8a7aa0..4426bcc8a985 100644 --- a/fs/xfs/xfs_reflink.c +++ b/fs/xfs/xfs_reflink.c @@ -29,6 +29,7 @@ #include "xfs_iomap.h" #include "xfs_sb.h" #include "xfs_ag_resv.h" +#include <linux/dax.h> /* * Copy on Write of Shared Blocks @@ -1324,8 +1325,8 @@ xfs_reflink_remap_prep( if (XFS_IS_REALTIME_INODE(src) || XFS_IS_REALTIME_INODE(dest)) goto out_unlock; - /* Don't share DAX file data for now. */ - if (IS_DAX(inode_in) || IS_DAX(inode_out)) + /* Don't share DAX file data with non-DAX file. */ + if (IS_DAX(inode_in) != IS_DAX(inode_out)) goto out_unlock; if (!IS_DAX(inode_in))
Add xfs_break_two_dax_layouts() to break layout for tow dax files. Then call compare range function only when files are both DAX or not. Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> --- fs/xfs/xfs_file.c | 20 ++++++++++++++++++++ fs/xfs/xfs_inode.c | 8 +++++++- fs/xfs/xfs_inode.h | 1 + fs/xfs/xfs_reflink.c | 5 +++-- 4 files changed, 31 insertions(+), 3 deletions(-)