On Tue, Jan 23, 2018 at 07:05:20AM -0500, Brian Foster wrote:
> On Sat, Jan 20, 2018 at 09:33:55PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Before we share blocks between files, we need to break the pnfs leases
> > on the layout before we start slicing and dicing the block map.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_reflink.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 45 insertions(+), 1 deletion(-)
> >
> >
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index 47aea2e..ce523dd 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -1245,6 +1245,48 @@ xfs_reflink_remap_blocks(
> > }
> >
> > /*
> > + * Grab the exclusive iolock for a data copy from src to dest, making
> > + * sure to abide vfs locking order (lowest pointer value goes first) and
> > + * breaking the pnfs layout leases on dest before proceeding.
> > + */
> > +static int
> > +xfs_iolock_two_inodes_and_break_layout(
> > + struct inode *src,
> > + struct inode *dest)
> > +{
> > + bool src_first = src < dest;
> > + bool src_last = src > dest;
> > + int error;
> > +
> > +retry:
> > + if (src_first) {
> > + inode_lock(src);
> > + inode_lock_nested(dest, I_MUTEX_NONDIR2);
> > + } else {
> > + inode_lock(dest);
> > + }
> > +
> > + error = break_layout(dest, false);
> > + if (error == -EWOULDBLOCK) {
> > + inode_unlock(dest);
> > + if (src_first)
> > + inode_unlock(src);
> > + error = break_layout(dest, true);
> > + if (error)
> > + return error;
> > + goto retry;
> > + } else if (error) {
> > + inode_unlock(dest);
> > + if (src_first)
> > + inode_unlock(src);
> > + return error;
> > + }
> > + if (src_last)
> > + inode_lock_nested(src, I_MUTEX_NONDIR2);
> > + return 0;
> > +}
> > +
>
> I'm not really familiar with the lease code so it's not clear to me, for
> example, why the loop is necessary here and whatnot.
Christoph objected to the original version of this patch grabbing
IOLOCK_EXCL (or any lock) and then spinning on a break-unlock-break-lock
loop trying to break the leases on dest. I didn't see anything obvious
that could stall in break_layout for a long time, but the fact that it
has a 'wait' parameter strongly implies that something in that path
could sleep.
> But it seems the above could be factored into a slightly more generic
> helper patterned after xfs_break_layouts(). For example, create an
> xfs_break_two_nondir_layouts() in xfs_pnfs.c with the exact same
> semantics/structure as the former, just replace the iolock calls with
> the lock/unlock_two_nondir() calls..?
The awkward part of this patch is that we really only need a function
like this to do things like reflink (read from one file, write to
another) so that's why it's in xfs_reflink.c... but I'll move on to the
next patch for more of a reply. :)
--D
> Brian
>
> > +/*
> > * Link a range of blocks from one file to another.
> > */
> > int
> > @@ -1274,7 +1316,9 @@ xfs_reflink_remap_range(
> > return -EIO;
> >
> > /* Lock both files against IO */
> > - lock_two_nondirectories(inode_in, inode_out);
> > + ret = xfs_iolock_two_inodes_and_break_layout(inode_in, inode_out);
> > + if (ret)
> > + return ret;
> > if (same_inode)
> > xfs_ilock(src, XFS_MMAPLOCK_EXCL);
> > else
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
