From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 08/15] xfs: inline xfs_shift_file_space into callers
Date: Fri, 20 Oct 2017 17:07:20 -0700 [thread overview]
Message-ID: <20171021000720.GB4755@magnolia> (raw)
In-Reply-To: <20171019065942.18813-9-hch@lst.de>
On Thu, Oct 19, 2017 at 08:59:35AM +0200, Christoph Hellwig wrote:
> The code is sufficiently different for the insert vs collapse cases both
> in xfs_shift_file_space itself and the callers that untangling them will
> make life a lot easier down the road.
>
> We still keep a common helper for flushing all data and COW state to get
> the inode into the right shape for shifting the extents around.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_bmap_util.c | 192 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 102 insertions(+), 90 deletions(-)
>
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 0543423651ff..47b53c88de7c 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1260,53 +1260,12 @@ xfs_zero_file_space(
>
> }
>
> -/*
> - * @next_fsb will keep track of the extent currently undergoing shift.
> - * @stop_fsb will keep track of the extent at which we have to stop.
> - * If we are shifting left, we will start with block (offset + len) and
> - * shift each extent till last extent.
> - * If we are shifting right, we will start with last extent inside file space
> - * and continue until we reach the block corresponding to offset.
> - */
> static int
> -xfs_shift_file_space(
> - struct xfs_inode *ip,
> - xfs_off_t offset,
> - xfs_off_t len,
> - enum shift_direction direction)
> +xfs_prepare_shift(
> + struct xfs_inode *ip,
> + loff_t offset)
> {
> - int done = 0;
> - struct xfs_mount *mp = ip->i_mount;
> - struct xfs_trans *tp;
> int error;
> - struct xfs_defer_ops dfops;
> - xfs_fsblock_t first_block;
> - xfs_fileoff_t stop_fsb;
> - xfs_fileoff_t next_fsb;
> - xfs_fileoff_t shift_fsb;
> - uint resblks;
> -
> - ASSERT(direction == SHIFT_LEFT || direction == SHIFT_RIGHT);
> -
> - if (direction == SHIFT_LEFT) {
> - /*
> - * Reserve blocks to cover potential extent merges after left
> - * shift operations.
> - */
> - resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
> - next_fsb = XFS_B_TO_FSB(mp, offset + len);
> - stop_fsb = XFS_B_TO_FSB(mp, VFS_I(ip)->i_size);
> - } else {
> - /*
> - * If right shift, delegate the work of initialization of
> - * next_fsb to xfs_bmap_shift_extent as it has ilock held.
> - */
> - resblks = 0;
> - next_fsb = NULLFSBLOCK;
> - stop_fsb = XFS_B_TO_FSB(mp, offset);
> - }
> -
> - shift_fsb = XFS_B_TO_FSB(mp, len);
>
> /*
> * Trim eofblocks to avoid shifting uninitialized post-eof preallocation
> @@ -1322,8 +1281,7 @@ xfs_shift_file_space(
> * Writeback and invalidate cache for the remainder of the file as we're
> * about to shift down every extent from offset to EOF.
> */
> - error = filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
> - offset, -1);
> + error = filemap_write_and_wait_range(VFS_I(ip)->i_mapping, offset, -1);
> if (error)
> return error;
> error = invalidate_inode_pages2_range(VFS_I(ip)->i_mapping,
> @@ -1343,16 +1301,48 @@ xfs_shift_file_space(
> return error;
> }
>
> - /*
> - * The extent shifting code works on extent granularity. So, if
> - * stop_fsb is not the starting block of extent, we need to split
> - * the extent at stop_fsb.
> - */
> - if (direction == SHIFT_RIGHT) {
> - error = xfs_bmap_split_extent(ip, stop_fsb);
> - if (error)
> - return error;
> - }
> + return 0;
> +}
> +
> +/*
> + * xfs_collapse_file_space()
> + * This routine frees disk space and shift extent for the given file.
> + * The first thing we do is to free data blocks in the specified range
> + * by calling xfs_free_file_space(). It would also sync dirty data
> + * and invalidate page cache over the region on which collapse range
> + * is working. And Shift extent records to the left to cover a hole.
> + * RETURNS:
> + * 0 on success
> + * errno on error
> + *
> + */
> +int
> +xfs_collapse_file_space(
> + struct xfs_inode *ip,
> + xfs_off_t offset,
> + xfs_off_t len)
> +{
> + int done = 0;
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_trans *tp;
> + int error;
> + struct xfs_defer_ops dfops;
> + xfs_fsblock_t first_block;
> + xfs_fileoff_t stop_fsb = XFS_B_TO_FSB(mp, VFS_I(ip)->i_size);
> + xfs_fileoff_t next_fsb = XFS_B_TO_FSB(mp, offset + len);
> + xfs_fileoff_t shift_fsb = XFS_B_TO_FSB(mp, len);
> + uint resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
> +
> + ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
So it took me a while of wondering "don't we have to have the
MMAPLOCK_EXCL too?" before realizing that yes, the caller actually does
grab that too. I wonder if it's worth checking here, since you're
asserting the lock status at all?
Aside from that,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> + trace_xfs_collapse_file_space(ip);
> +
> + error = xfs_free_file_space(ip, offset, len);
> + if (error)
> + return error;
> +
> + error = xfs_prepare_shift(ip, offset);
> + if (error)
> + return error;
>
> while (!error && !done) {
> error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0,
> @@ -1366,7 +1356,6 @@ xfs_shift_file_space(
> XFS_QMOPT_RES_REGBLKS);
> if (error)
> goto out_trans_cancel;
> -
> xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>
> xfs_defer_init(&dfops, &first_block);
> @@ -1377,14 +1366,13 @@ xfs_shift_file_space(
> */
> error = xfs_bmap_shift_extents(tp, ip, &next_fsb, shift_fsb,
> &done, stop_fsb, &first_block, &dfops,
> - direction, XFS_BMAP_MAX_SHIFT_EXTENTS);
> + SHIFT_LEFT, XFS_BMAP_MAX_SHIFT_EXTENTS);
> if (error)
> goto out_bmap_cancel;
>
> error = xfs_defer_finish(&tp, &dfops);
> if (error)
> goto out_bmap_cancel;
> -
> error = xfs_trans_commit(tp);
> }
>
> @@ -1397,36 +1385,6 @@ xfs_shift_file_space(
> return error;
> }
>
> -/*
> - * xfs_collapse_file_space()
> - * This routine frees disk space and shift extent for the given file.
> - * The first thing we do is to free data blocks in the specified range
> - * by calling xfs_free_file_space(). It would also sync dirty data
> - * and invalidate page cache over the region on which collapse range
> - * is working. And Shift extent records to the left to cover a hole.
> - * RETURNS:
> - * 0 on success
> - * errno on error
> - *
> - */
> -int
> -xfs_collapse_file_space(
> - struct xfs_inode *ip,
> - xfs_off_t offset,
> - xfs_off_t len)
> -{
> - int error;
> -
> - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> - trace_xfs_collapse_file_space(ip);
> -
> - error = xfs_free_file_space(ip, offset, len);
> - if (error)
> - return error;
> -
> - return xfs_shift_file_space(ip, offset, len, SHIFT_LEFT);
> -}
> -
> /*
> * xfs_insert_file_space()
> * This routine create hole space by shifting extents for the given file.
> @@ -1445,10 +1403,64 @@ xfs_insert_file_space(
> loff_t offset,
> loff_t len)
> {
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_trans *tp;
> + int error;
> + struct xfs_defer_ops dfops;
> + xfs_fsblock_t first_block;
> + xfs_fileoff_t stop_fsb = XFS_B_TO_FSB(mp, offset);
> + xfs_fileoff_t next_fsb = NULLFSBLOCK;
> + xfs_fileoff_t shift_fsb = XFS_B_TO_FSB(mp, len);
> + int done = 0;
> +
> ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> trace_xfs_insert_file_space(ip);
>
> - return xfs_shift_file_space(ip, offset, len, SHIFT_RIGHT);
> + error = xfs_prepare_shift(ip, offset);
> + if (error)
> + return error;
> +
> + /*
> + * The extent shifting code works on extent granularity. So, if stop_fsb
> + * is not the starting block of extent, we need to split the extent at
> + * stop_fsb.
> + */
> + error = xfs_bmap_split_extent(ip, stop_fsb);
> + if (error)
> + return error;
> +
> + while (!error && !done) {
> + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 0, 0, 0,
> + &tp);
> + if (error)
> + break;
> +
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> + xfs_defer_init(&dfops, &first_block);
> +
> + /*
> + * We are using the write transaction in which max 2 bmbt
> + * updates are allowed
> + */
> + error = xfs_bmap_shift_extents(tp, ip, &next_fsb, shift_fsb,
> + &done, stop_fsb, &first_block, &dfops,
> + SHIFT_RIGHT, XFS_BMAP_MAX_SHIFT_EXTENTS);
> + if (error)
> + goto out_bmap_cancel;
> +
> + error = xfs_defer_finish(&tp, &dfops);
> + if (error)
> + goto out_bmap_cancel;
> + error = xfs_trans_commit(tp);
> + }
> +
> + return error;
> +
> +out_bmap_cancel:
> + xfs_defer_cancel(&dfops);
> + xfs_trans_cancel(tp);
> + return error;
> }
>
> /*
> --
> 2.14.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-10-21 0:07 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-19 6:59 more extent mapping cleanups Christoph Hellwig
2017-10-19 6:59 ` [PATCH 01/15] xfs: add a xfs_bmap_fork_to_state helper Christoph Hellwig
2017-10-19 22:48 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 02/15] xfs: make better use of the 'state' variable in xfs_bmap_del_extent_real Christoph Hellwig
2017-10-19 22:49 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 03/15] xfs: remove post-bmap tracing in xfs_bmap_local_to_extents Christoph Hellwig
2017-10-19 22:49 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 04/15] xfs: move pre/post-bmap tracing into xfs_iext_update_extent Christoph Hellwig
2017-10-19 22:50 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 05/15] xfs: remove XFS_BMAP_TRACE_EXLIST Christoph Hellwig
2017-10-19 22:50 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 06/15] xfs: remove the never fully implemented UUID fork format Christoph Hellwig
2017-10-19 22:48 ` Darrick J. Wong
2017-10-20 7:02 ` Christoph Hellwig
2017-10-20 16:52 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 07/15] xfs: remove if_rdev Christoph Hellwig
2017-10-19 22:52 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 08/15] xfs: inline xfs_shift_file_space into callers Christoph Hellwig
2017-10-21 0:07 ` Darrick J. Wong [this message]
2017-10-21 8:13 ` Christoph Hellwig
2017-10-21 18:06 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 09/15] xfs: remove XFS_BMAP_MAX_SHIFT_EXTENTS Christoph Hellwig
2017-10-21 0:10 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 10/15] xfs: split xfs_bmap_shift_extents Christoph Hellwig
2017-10-21 0:22 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 11/15] xfs: remove xfs_bmse_shift_one Christoph Hellwig
2017-10-21 0:25 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 12/15] xfs: update got in xfs_bmap_shift_update_extent Christoph Hellwig
2017-10-21 0:25 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 13/15] xfs: don't rely on extent indices in xfs_bmap_collapse_extents Christoph Hellwig
2017-10-21 0:26 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 14/15] xfs: don't rely on extent indices in xfs_bmap_insert_extents Christoph Hellwig
2017-10-21 0:27 ` Darrick J. Wong
2017-10-19 6:59 ` [PATCH 15/15] xfs: rewrite xfs_bmap_first_unused to make better use of xfs_iext_get_extent Christoph Hellwig
2017-10-21 0:27 ` Darrick J. Wong
2017-10-19 20:04 ` more extent mapping cleanups Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171021000720.GB4755@magnolia \
--to=darrick.wong@oracle.com \
--cc=hch@lst.de \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).