From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 10/15] xfs: split xfs_bmap_shift_extents
Date: Fri, 20 Oct 2017 17:22:40 -0700 [thread overview]
Message-ID: <20171021002240.GD4755@magnolia> (raw)
In-Reply-To: <20171019065942.18813-11-hch@lst.de>
On Thu, Oct 19, 2017 at 08:59:37AM +0200, Christoph Hellwig wrote:
> Have a separate helper for insert vs collapse, as this prepares us for
> simplifying the code in the next patches.
>
> Also changed the done output argument to a bool intead of int for both
> new functions.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 197 ++++++++++++++++++++++++++++++++---------------
> fs/xfs/libxfs/xfs_bmap.h | 10 ++-
> fs/xfs/xfs_bmap_util.c | 14 ++--
> 3 files changed, 148 insertions(+), 73 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 89ea8a5235c0..7d3a38e69d28 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5689,57 +5689,151 @@ xfs_bmse_shift_one(
> return xfs_rmap_map_extent(mp, dfops, ip, whichfork, &new);
> }
>
> -/*
> - * Shift extent records to the left/right to cover/create a hole.
> - *
> - * @stop_fsb specifies the file offset at which to stop shift and the
> - * file offset where we've left off is returned in @next_fsb. @offset_shift_fsb
> - * is the length by which each extent is shifted. If there is no hole to shift
> - * the extents into, this will be considered invalid operation and we abort
> - * immediately.
> - */
> int
> -xfs_bmap_shift_extents(
> +xfs_bmap_collapse_extents(
> struct xfs_trans *tp,
> struct xfs_inode *ip,
> xfs_fileoff_t *next_fsb,
> xfs_fileoff_t offset_shift_fsb,
> - int *done,
> + bool *done,
> xfs_fileoff_t stop_fsb,
> xfs_fsblock_t *firstblock,
> - struct xfs_defer_ops *dfops,
> - enum shift_direction direction)
> + struct xfs_defer_ops *dfops)
> {
> - struct xfs_btree_cur *cur = NULL;
> - struct xfs_bmbt_irec got;
> - struct xfs_mount *mp = ip->i_mount;
> - struct xfs_ifork *ifp;
> - xfs_extnum_t current_ext;
> - xfs_extnum_t total_extents;
> - xfs_extnum_t stop_extent;
> - int error = 0;
> - int whichfork = XFS_DATA_FORK;
> - int logflags = 0;
> + int whichfork = XFS_DATA_FORK;
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
> + struct xfs_btree_cur *cur = NULL;
> + struct xfs_bmbt_irec got;
> + xfs_extnum_t current_ext;
> + xfs_extnum_t total_extents;
> + xfs_extnum_t stop_extent;
> + int error = 0;
> + int logflags = 0;
>
> if (unlikely(XFS_TEST_ERROR(
> (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
> mp, XFS_ERRTAG_BMAPIFORMAT))) {
> - XFS_ERROR_REPORT("xfs_bmap_shift_extents",
> - XFS_ERRLEVEL_LOW, mp);
> + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> return -EFSCORRUPTED;
> }
>
> if (XFS_FORCED_SHUTDOWN(mp))
> return -EIO;
>
> - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> - ASSERT(direction == SHIFT_LEFT || direction == SHIFT_RIGHT);
> + ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
> +
> + if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> + error = xfs_iread_extents(tp, ip, whichfork);
> + if (error)
> + return error;
> + }
> +
> + if (ifp->if_flags & XFS_IFBROOT) {
> + cur = xfs_bmbt_init_cursor(mp, tp, ip, whichfork);
> + cur->bc_private.b.firstblock = *firstblock;
> + cur->bc_private.b.dfops = dfops;
> + cur->bc_private.b.flags = 0;
> + }
> +
> + /*
> + * There may be delalloc extents in the data fork before the range we
> + * are collapsing out, so we cannot use the count of real extents here.
> + * Instead we have to calculate it from the incore fork.
> + */
> + total_extents = xfs_iext_count(ifp);
> + if (total_extents == 0) {
> + *done = true;
> + goto del_cursor;
> + }
> +
> + /*
> + * Look up the extent index for the fsb where we start shifting. We can
> + * henceforth iterate with current_ext as extent list changes are locked
> + * out via ilock.
> + *
> + * If next_fsb lies in a hole beyond which there are no extents we are
> + * done.
> + */
> + if (!xfs_iext_lookup_extent(ip, ifp, *next_fsb, ¤t_ext, &got)) {
> + *done = true;
> + goto del_cursor;
> + }
> +
> + stop_extent = total_extents;
> + if (current_ext >= stop_extent) {
> + error = -EIO;
> + goto del_cursor;
> + }
> +
> + error = xfs_bmse_shift_one(ip, whichfork, offset_shift_fsb,
> + ¤t_ext, &got, cur, &logflags,
> + SHIFT_LEFT, dfops);
> + if (error)
> + goto del_cursor;
> + /*
> + * If there was an extent merge during the shift, the extent
> + * count can change. Update the total and grade the next record.
> + */
> + total_extents = xfs_iext_count(ifp);
> + stop_extent = total_extents;
> + if (current_ext == stop_extent) {
> + *done = true;
> + goto del_cursor;
> + }
> + xfs_iext_get_extent(ifp, current_ext, &got);
> +
> + if (!*done)
> + *next_fsb = got.br_startoff;
> +
> +del_cursor:
> + if (cur)
> + xfs_btree_del_cursor(cur,
> + error ? XFS_BTREE_ERROR : XFS_BTREE_NOERROR);
> +
> + if (logflags)
> + xfs_trans_log_inode(tp, ip, logflags);
> +
> + return error;
> +}
> +
> +int
> +xfs_bmap_insert_extents(
> + struct xfs_trans *tp,
> + struct xfs_inode *ip,
> + xfs_fileoff_t *next_fsb,
> + xfs_fileoff_t offset_shift_fsb,
> + bool *done,
> + xfs_fileoff_t stop_fsb,
> + xfs_fsblock_t *firstblock,
> + struct xfs_defer_ops *dfops)
> +{
> + int whichfork = XFS_DATA_FORK;
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
> + struct xfs_btree_cur *cur = NULL;
> + struct xfs_bmbt_irec got, s;
> + xfs_extnum_t current_ext;
> + xfs_extnum_t total_extents;
> + xfs_extnum_t stop_extent;
> + int error = 0;
> + int logflags = 0;
> +
> + if (unlikely(XFS_TEST_ERROR(
> + (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> + XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
> + mp, XFS_ERRTAG_BMAPIFORMAT))) {
> + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> + return -EFSCORRUPTED;
> + }
> +
> + if (XFS_FORCED_SHUTDOWN(mp))
> + return -EIO;
> +
> + ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
>
> - ifp = XFS_IFORK_PTR(ip, whichfork);
> if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> - /* Read in all the extents */
> error = xfs_iread_extents(tp, ip, whichfork);
> if (error)
> return error;
> @@ -5759,7 +5853,7 @@ xfs_bmap_shift_extents(
> */
> total_extents = xfs_iext_count(ifp);
> if (total_extents == 0) {
> - *done = 1;
> + *done = true;
> goto del_cursor;
> }
>
> @@ -5767,12 +5861,10 @@ xfs_bmap_shift_extents(
> * In case of first right shift, we need to initialize next_fsb
> */
> if (*next_fsb == NULLFSBLOCK) {
> - ASSERT(direction == SHIFT_RIGHT);
> -
> current_ext = total_extents - 1;
> xfs_iext_get_extent(ifp, current_ext, &got);
> if (stop_fsb > got.br_startoff) {
> - *done = 1;
> + *done = true;
> goto del_cursor;
> }
> *next_fsb = got.br_startoff;
> @@ -5787,46 +5879,27 @@ xfs_bmap_shift_extents(
> */
> if (!xfs_iext_lookup_extent(ip, ifp, *next_fsb, ¤t_ext,
> &got)) {
> - *done = 1;
> + *done = true;
> goto del_cursor;
> }
> }
>
> /* Lookup the extent index at which we have to stop */
> - if (direction == SHIFT_RIGHT) {
> - struct xfs_bmbt_irec s;
> -
> - xfs_iext_lookup_extent(ip, ifp, stop_fsb, &stop_extent, &s);
> - /* Make stop_extent exclusive of shift range */
> - stop_extent--;
> - if (current_ext <= stop_extent) {
> - error = -EIO;
> - goto del_cursor;
> - }
> - } else {
> - stop_extent = total_extents;
> - if (current_ext >= stop_extent) {
> - error = -EIO;
> - goto del_cursor;
> - }
> + xfs_iext_lookup_extent(ip, ifp, stop_fsb, &stop_extent, &s);
> + /* Make stop_extent exclusive of shift range */
> + stop_extent--;
> + if (current_ext <= stop_extent) {
> + error = -EIO;
> + goto del_cursor;
> }
>
> error = xfs_bmse_shift_one(ip, whichfork, offset_shift_fsb,
> ¤t_ext, &got, cur, &logflags,
> - direction, dfops);
> + SHIFT_RIGHT, dfops);
> if (error)
> goto del_cursor;
> - /*
> - * If there was an extent merge during the shift, the extent
> - * count can change. Update the total and grade the next record.
> - */
> - if (direction == SHIFT_LEFT) {
> - total_extents = xfs_iext_count(ifp);
> - stop_extent = total_extents;
> - }
> -
> if (current_ext == stop_extent) {
> - *done = 1;
> + *done = true;
> goto del_cursor;
> }
> xfs_iext_get_extent(ifp, current_ext, &got);
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 985b1c26566a..f7ccf2de1a8c 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -227,10 +227,14 @@ int xfs_bmap_del_extent_delay(struct xfs_inode *ip, int whichfork,
> void xfs_bmap_del_extent_cow(struct xfs_inode *ip, xfs_extnum_t *idx,
> struct xfs_bmbt_irec *got, struct xfs_bmbt_irec *del);
> uint xfs_default_attroffset(struct xfs_inode *ip);
> -int xfs_bmap_shift_extents(struct xfs_trans *tp, struct xfs_inode *ip,
> +int xfs_bmap_collapse_extents(struct xfs_trans *tp, struct xfs_inode *ip,
> xfs_fileoff_t *next_fsb, xfs_fileoff_t offset_shift_fsb,
> - int *done, xfs_fileoff_t stop_fsb, xfs_fsblock_t *firstblock,
> - struct xfs_defer_ops *dfops, enum shift_direction direction);
> + bool *done, xfs_fileoff_t stop_fsb, xfs_fsblock_t *firstblock,
> + struct xfs_defer_ops *dfops);
> +int xfs_bmap_insert_extents(struct xfs_trans *tp, struct xfs_inode *ip,
> + xfs_fileoff_t *next_fsb, xfs_fileoff_t offset_shift_fsb,
> + bool *done, xfs_fileoff_t stop_fsb, xfs_fsblock_t *firstblock,
> + struct xfs_defer_ops *dfops);
> int xfs_bmap_split_extent(struct xfs_inode *ip, xfs_fileoff_t split_offset);
> int xfs_bmapi_reserve_delalloc(struct xfs_inode *ip, int whichfork,
> xfs_fileoff_t off, xfs_filblks_t len, xfs_filblks_t prealloc,
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 3273f083c496..034f3429ca8c 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1322,7 +1322,6 @@ xfs_collapse_file_space(
> xfs_off_t offset,
> xfs_off_t len)
> {
> - int done = 0;
> struct xfs_mount *mp = ip->i_mount;
> struct xfs_trans *tp;
> int error;
> @@ -1332,6 +1331,7 @@ xfs_collapse_file_space(
> 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);
> + bool done = false;
>
> ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> trace_xfs_collapse_file_space(ip);
> @@ -1359,9 +1359,8 @@ xfs_collapse_file_space(
> xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>
> xfs_defer_init(&dfops, &first_block);
> - error = xfs_bmap_shift_extents(tp, ip, &next_fsb, shift_fsb,
> - &done, stop_fsb, &first_block, &dfops,
> - SHIFT_LEFT);
> + error = xfs_bmap_collapse_extents(tp, ip, &next_fsb, shift_fsb,
> + &done, stop_fsb, &first_block, &dfops);
> if (error)
> goto out_bmap_cancel;
>
> @@ -1406,7 +1405,7 @@ xfs_insert_file_space(
> 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;
> + bool done = false;
>
> ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> trace_xfs_insert_file_space(ip);
> @@ -1433,9 +1432,8 @@ xfs_insert_file_space(
> xfs_ilock(ip, XFS_ILOCK_EXCL);
> xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> xfs_defer_init(&dfops, &first_block);
> - error = xfs_bmap_shift_extents(tp, ip, &next_fsb, shift_fsb,
> - &done, stop_fsb, &first_block, &dfops,
> - SHIFT_RIGHT);
> + error = xfs_bmap_insert_extents(tp, ip, &next_fsb, shift_fsb,
> + &done, stop_fsb, &first_block, &dfops);
> if (error)
> goto out_bmap_cancel;
>
> --
> 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:22 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
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 [this message]
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=20171021002240.GD4755@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.