| Message ID | 20190909182722.16783-10-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/19] iomap: better document the IOMAP_F_* flags | expand |
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@lst.de> > --- > 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 >
On Wed, Sep 18, 2019 at 10:17:33AM -0700, Darrick J. Wong wrote: > > /* > > * 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. No idea. This is your old code just moved down to this function. But yes, the comment looks rather confusing and maybe we should just remove it.
On Wed, Sep 18, 2019 at 07:25:45PM +0200, Christoph Hellwig wrote: > On Wed, Sep 18, 2019 at 10:17:33AM -0700, Darrick J. Wong wrote: > > > /* > > > * 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. > > No idea. This is your old code just moved down to this function. But > yes, the comment looks rather confusing and maybe we should just remove > it. I know. I remember testing it way back in 4.9 when we first merged it to make sure it really did work that way, and I think the locking model hasn't changed so it should be fine. But just wanted another set of eyes to make sure things haven't subtlely shifted since then. :) --D
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? :) */ 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;
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@lst.de> --- fs/xfs/xfs_reflink.c | 108 ++++--------------------------------------- 1 file changed, 10 insertions(+), 98 deletions(-)