On Tue, Jan 23, 2018 at 07:05:34AM -0500, Brian Foster wrote:
> On Sat, Jan 20, 2018 at 09:34:03PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Reflink and dedupe operations remap blocks from a source file into a
> > destination file. The destination file needs exclusive locks on all
> > levels because we're updating its block map, but the source file isn't
> > undergoing any block map changes so we can use a shared lock.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_reflink.c | 50 +++++++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 37 insertions(+), 13 deletions(-)
> >
> >
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index ce523dd..5d1ff5a 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -1202,13 +1202,16 @@ xfs_reflink_remap_blocks(
> >
> > /* drange = (destoff, destoff + len); srange = (srcoff, srcoff + len) */
> > while (len) {
> > + uint lock_mode;
> > +
> > trace_xfs_reflink_remap_blocks_loop(src, srcoff, len,
> > dest, destoff);
> > +
> > /* Read extent from the source file */
> > nimaps = 1;
> > - xfs_ilock(src, XFS_ILOCK_EXCL);
> > + lock_mode = xfs_ilock_data_map_shared(src);
> > error = xfs_bmapi_read(src, srcoff, len, &imap, &nimaps, 0);
> > - xfs_iunlock(src, XFS_ILOCK_EXCL);
> > + xfs_iunlock(src, lock_mode);
> > if (error)
> > goto err;
> > ASSERT(nimaps == 1);
> > @@ -1260,7 +1263,7 @@ xfs_iolock_two_inodes_and_break_layout(
> >
> > retry:
> > if (src_first) {
> > - inode_lock(src);
> > + inode_lock_shared(src);
>
> Hm, I guess this could make my comment on the previous patch more
> difficult. Oh well.
Yeah, there's nowhere else in the xfs code where we do this (read lock
one file, write lock another), afaict.
> > inode_lock_nested(dest, I_MUTEX_NONDIR2);
> > } else {
> > inode_lock(dest);
> > @@ -1270,7 +1273,7 @@ xfs_iolock_two_inodes_and_break_layout(
> > if (error == -EWOULDBLOCK) {
> > inode_unlock(dest);
> > if (src_first)
> > - inode_unlock(src);
> > + inode_unlock_shared(src);
> > error = break_layout(dest, true);
> > if (error)
> > return error;
> > @@ -1278,14 +1281,36 @@ xfs_iolock_two_inodes_and_break_layout(
> > } else if (error) {
> > inode_unlock(dest);
> > if (src_first)
> > - inode_unlock(src);
> > + inode_unlock_shared(src);
> > return error;
> > }
> > if (src_last)
> > - inode_lock_nested(src, I_MUTEX_NONDIR2);
> > + down_read_nested(&src->i_rwsem, I_MUTEX_NONDIR2);
> > return 0;
> > }
> >
> > +static void
> > +xfs_reflink_mmaplock_two(
> > + struct xfs_inode *src,
> > + struct xfs_inode *dest)
> > +{
> > + int i = 0;
> > +
> > + if (src->i_ino == dest->i_ino) {
> > + xfs_ilock(src, XFS_MMAPLOCK_EXCL);
> > + return;
> > + }
> > +
> > + if (src->i_ino < dest->i_ino) {
> > + xfs_ilock(src, XFS_MMAPLOCK_SHARED);
> > + i++;
> > + }
> > + xfs_ilock(dest, XFS_MMAPLOCK_EXCL + (i << XFS_MMAPLOCK_SHIFT));
> > + i++;
> > + if (src->i_ino > dest->i_ino)
> > + xfs_ilock(src, XFS_MMAPLOCK_SHARED + (i << XFS_MMAPLOCK_SHIFT));
> > +}
> > +
>
> I am kind of wondering if this one could be replaced with a refactor of
> xfs_lock_two_inodes() to take two sets of lock flags (then create a
> wrapper to preserve the current signature that just passes the same set
> of flags for both inodes).
Yes, I'll do that.
--D
> Brian
>
> > /*
> > * Link a range of blocks from one file to another.
> > */
> > @@ -1319,10 +1344,7 @@ xfs_reflink_remap_range(
> > 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
> > - xfs_lock_two_inodes(src, dest, XFS_MMAPLOCK_EXCL);
> > + xfs_reflink_mmaplock_two(src, dest);
> >
> > /* Check file eligibility and prepare for block sharing. */
> > ret = -EINVAL;
> > @@ -1385,10 +1407,12 @@ xfs_reflink_remap_range(
> > is_dedupe);
> >
> > out_unlock:
> > - xfs_iunlock(src, XFS_MMAPLOCK_EXCL);
> > + xfs_iunlock(dest, XFS_MMAPLOCK_EXCL);
> > + if (!same_inode)
> > + xfs_iunlock(src, XFS_MMAPLOCK_SHARED);
> > + inode_unlock(inode_out);
> > if (!same_inode)
> > - xfs_iunlock(dest, XFS_MMAPLOCK_EXCL);
> > - unlock_two_nondirectories(inode_in, inode_out);
> > + inode_unlock_shared(inode_in);
> > if (ret)
> > trace_xfs_reflink_remap_range_error(dest, ret, _RET_IP_);
> > return ret;
> >
> > --
> > 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
