On Mon, Sep 09, 2019 at 08:27:12PM +0200, Christoph Hellwig wrote:
> Now that xfs_file_unshare is not completely dumb we can just call it
> directly without iterating the extent and reflink btrees ourselves.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_reflink.c | 108 ++++---------------------------------------
> 1 file changed, 10 insertions(+), 98 deletions(-)
>
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index cadc0456804d..73f8cce4722d 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1381,85 +1381,6 @@ xfs_reflink_remap_prep(
> return ret;
> }
>
> -/*
> - * The user wants to preemptively CoW all shared blocks in this file,
> - * which enables us to turn off the reflink flag. Iterate all
> - * extents which are not prealloc/delalloc to see which ranges are
> - * mentioned in the refcount tree, then read those blocks into the
> - * pagecache, dirty them, fsync them back out, and then we can update
> - * the inode flag. What happens if we run out of memory? :)
> - */
> -STATIC int
> -xfs_reflink_dirty_extents(
> - struct xfs_inode *ip,
> - xfs_fileoff_t fbno,
> - xfs_filblks_t end,
> - xfs_off_t isize)
> -{
> - struct xfs_mount *mp = ip->i_mount;
> - xfs_agnumber_t agno;
> - xfs_agblock_t agbno;
> - xfs_extlen_t aglen;
> - xfs_agblock_t rbno;
> - xfs_extlen_t rlen;
> - xfs_off_t fpos;
> - xfs_off_t flen;
> - struct xfs_bmbt_irec map[2];
> - int nmaps;
> - int error = 0;
> -
> - while (end - fbno > 0) {
> - nmaps = 1;
> - /*
> - * Look for extents in the file. Skip holes, delalloc, or
> - * unwritten extents; they can't be reflinked.
> - */
> - error = xfs_bmapi_read(ip, fbno, end - fbno, map, &nmaps, 0);
> - if (error)
> - goto out;
> - if (nmaps == 0)
> - break;
> - if (!xfs_bmap_is_real_extent(&map[0]))
> - goto next;
> -
> - map[1] = map[0];
> - while (map[1].br_blockcount) {
> - agno = XFS_FSB_TO_AGNO(mp, map[1].br_startblock);
> - agbno = XFS_FSB_TO_AGBNO(mp, map[1].br_startblock);
> - aglen = map[1].br_blockcount;
> -
> - error = xfs_reflink_find_shared(mp, NULL, agno, agbno,
> - aglen, &rbno, &rlen, true);
> - if (error)
> - goto out;
> - if (rbno == NULLAGBLOCK)
> - break;
> -
> - /* Dirty the pages */
> - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> - fpos = XFS_FSB_TO_B(mp, map[1].br_startoff +
> - (rbno - agbno));
> - flen = XFS_FSB_TO_B(mp, rlen);
> - if (fpos + flen > isize)
> - flen = isize - fpos;
> - error = iomap_file_unshare(VFS_I(ip), fpos, flen,
> - &xfs_iomap_ops);
> - xfs_ilock(ip, XFS_ILOCK_EXCL);
> - if (error)
> - goto out;
> -
> - map[1].br_blockcount -= (rbno - agbno + rlen);
> - map[1].br_startoff += (rbno - agbno + rlen);
> - map[1].br_startblock += (rbno - agbno + rlen);
> - }
> -
> -next:
> - fbno = map[0].br_startoff + map[0].br_blockcount;
> - }
> -out:
> - return error;
> -}
> -
> /* Does this inode need the reflink flag? */
> int
> xfs_reflink_inode_has_shared_extents(
> @@ -1589,6 +1510,11 @@ xfs_reflink_try_clear_inode_flag(
> /*
> * Pre-COW all shared blocks within a given byte range of a file and turn off
> * the reflink flag if we unshare all of the file's blocks.
> + *
> + * Let iomap iterate all extents to see which are shared and not unwritten or
> + * delalloc and read them into the page cache, dirty them, fsync them back out,
> + * and then we can update the inode flag. What happens if we run out of
> + * memory? :)
I don't know, what /does/ happen? :)
It /should/ be fine, right? Writeback will start pushing the dirty
cache pages to disk, and since writeback only takes the ILOCK, it should
be able to perform the COW even while the unshare process sits on the
IOLOCK/MMAPLOCK. True, the unshare process and writeback will both be
contending on the ILOCK, but that shouldn't be a problem...
...unless I'm missing something? It sure does look nice to drain all
this other code out.
--D
> */
> int
> xfs_reflink_unshare(
> @@ -1596,10 +1522,7 @@ xfs_reflink_unshare(
> xfs_off_t offset,
> xfs_off_t len)
> {
> - struct xfs_mount *mp = ip->i_mount;
> - xfs_fileoff_t fbno;
> - xfs_filblks_t end;
> - xfs_off_t isize;
> + struct inode *inode = VFS_I(ip);
> int error;
>
> if (!xfs_is_reflink_inode(ip))
> @@ -1607,20 +1530,12 @@ xfs_reflink_unshare(
>
> trace_xfs_reflink_unshare(ip, offset, len);
>
> - inode_dio_wait(VFS_I(ip));
> + inode_dio_wait(inode);
>
> - /* Try to CoW the selected ranges */
> - xfs_ilock(ip, XFS_ILOCK_EXCL);
> - fbno = XFS_B_TO_FSBT(mp, offset);
> - isize = i_size_read(VFS_I(ip));
> - end = XFS_B_TO_FSB(mp, offset + len);
> - error = xfs_reflink_dirty_extents(ip, fbno, end, isize);
> + error = iomap_file_unshare(inode, offset, len, &xfs_iomap_ops);
> if (error)
> - goto out_unlock;
> - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -
> - /* Wait for the IO to finish */
> - error = filemap_write_and_wait(VFS_I(ip)->i_mapping);
> + goto out;
> + error = filemap_write_and_wait(inode->i_mapping);
> if (error)
> goto out;
>
> @@ -1628,11 +1543,8 @@ xfs_reflink_unshare(
> error = xfs_reflink_try_clear_inode_flag(ip);
> if (error)
> goto out;
> -
> return 0;
>
> -out_unlock:
> - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> out:
> trace_xfs_reflink_unshare_error(ip, error, _RET_IP_);
> return error;
> --
> 2.20.1
>
