From: Brian Foster <bfoster@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org, darrick.wong@oracle.com
Subject: Re: [PATCH 6/9] xfs: refactor xfs_bunmapi_cow
Date: Wed, 12 Oct 2016 10:13:08 -0400 [thread overview]
Message-ID: <20161012141308.GB56019@bfoster.bfoster> (raw)
In-Reply-To: <1476106685-29048-7-git-send-email-hch@lst.de>
On Mon, Oct 10, 2016 at 03:38:02PM +0200, Christoph Hellwig wrote:
> Split out two helpers for deleting delayed or real extents from the COW fork.
> This allows to call them directly from xfs_reflink_cow_end_io once that
> function is refactored to iterate the extent tree. It will also allow
> to reuse the delalloc deletion from xfs_bunmapi in the future.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 374 ++++++++++++++++++++++++++++-------------------
> fs/xfs/libxfs/xfs_bmap.h | 5 +
> 2 files changed, 225 insertions(+), 154 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 016dacc..814980d 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4862,6 +4862,219 @@ xfs_bmap_split_indlen(
> return stolen;
> }
>
> +int
> +xfs_bmap_del_extent_delay(
> + struct xfs_inode *ip,
> + int whichfork,
> + xfs_extnum_t *idx,
> + struct xfs_bmbt_irec *got,
> + struct xfs_bmbt_irec *del)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
> + struct xfs_bmbt_irec new;
> + int64_t da_old, da_new, da_diff = 0;
> + xfs_fileoff_t del_endoff, got_endoff;
> + xfs_filblks_t got_indlen, new_indlen, stolen;
> + int error = 0, state = 0;
> + bool isrt;
> +
> + XFS_STATS_INC(mp, xs_del_exlist);
> +
> + isrt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
> + del_endoff = del->br_startoff + del->br_blockcount;
> + got_endoff = got->br_startoff + got->br_blockcount;
> + da_old = startblockval(got->br_startblock);
> + da_new = 0;
> +
> + ASSERT(*idx >= 0);
> + ASSERT(*idx < ifp->if_bytes / sizeof(struct xfs_bmbt_rec));
> + ASSERT(del->br_blockcount > 0);
> + ASSERT(got->br_startoff <= del->br_startoff);
> + ASSERT(got_endoff >= del_endoff);
> +
> + if (isrt) {
> + int64_t rtexts = XFS_FSB_TO_B(mp, del->br_blockcount);
> +
> + do_div(rtexts, mp->m_sb.sb_rextsize);
> + xfs_mod_frextents(mp, rtexts);
> + }
> +
> + /*
> + * Update the inode delalloc counter now and wait to update the
> + * sb counters as we might have to borrow some blocks for the
> + * indirect block accounting.
> + */
> + xfs_trans_reserve_quota_nblks(NULL, ip, -((long)del->br_blockcount), 0,
> + isrt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
> + ip->i_delayed_blks -= del->br_blockcount;
> +
This appears to be fixed up later, but i_delayed_blks is accounted twice
as of this patch. It would be nice if we could avoid known breakage,
even if transient.
> + if (whichfork == XFS_COW_FORK)
> + state |= BMAP_COWFORK;
> +
> + if (got->br_startoff == del->br_startoff)
> + state |= BMAP_LEFT_CONTIG;
> + if (got_endoff == del_endoff)
> + state |= BMAP_RIGHT_CONTIG;
> +
> + switch (state & (BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG)) {
> + case BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG:
> + /*
> + * Matches the whole extent. Delete the entry.
> + */
> + xfs_iext_remove(ip, *idx, 1, state);
> + --*idx;
> + break;
> + case BMAP_LEFT_CONTIG:
> + /*
> + * Deleting the first part of the extent.
> + */
> + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> + got->br_startoff = del_endoff;
> + got->br_blockcount -= del->br_blockcount;
> + da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
> + got->br_blockcount), da_old);
> + got->br_startblock = nullstartblock((int)da_new);
> + xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> + trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> + break;
> + case BMAP_RIGHT_CONTIG:
> + /*
> + * Deleting the last part of the extent.
> + */
> + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> + got->br_blockcount = got->br_blockcount - del->br_blockcount;
> + da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
> + got->br_blockcount), da_old);
> + got->br_startblock = nullstartblock((int)da_new);
> + xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> + trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> + break;
> + case 0:
> + /*
> + * Deleting the middle of the extent.
> + *
> + * Distribute the original indlen reservation across the two
> + * new extents. Steal blocks from the deleted extent if
> + * necessary. Stealing blocks simply fudges the fdblocks
> + * accounting in xfs_bunmapi().
> + */
> + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> + got->br_blockcount = del->br_startoff - got->br_startoff;
> +
> + got_indlen = xfs_bmap_worst_indlen(ip, got->br_blockcount);
> + new_indlen = xfs_bmap_worst_indlen(ip, new.br_blockcount);
Doesn't look like new.br_blockcount is set until a few lines below.
Brian
> + stolen = xfs_bmap_split_indlen(da_old, &got_indlen, &new_indlen,
> + del->br_blockcount);
> + da_new = got_indlen + new_indlen - stolen;
> + del->br_blockcount -= stolen;
> +
> + /*
> + * Set the reservation for each extent. Warn if either
> + * is zero as this can lead to delalloc problems.
> + */
> + WARN_ON_ONCE(!got_indlen || !new_indlen);
> + got->br_startblock = nullstartblock((int)got_indlen);
> + xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> + trace_xfs_bmap_post_update(ip, *idx, 0, _THIS_IP_);
> +
> + new.br_startoff = del_endoff;
> + new.br_blockcount = got_endoff - del_endoff;
> + new.br_state = got->br_state;
> + new.br_startblock = nullstartblock((int)new_indlen);
> +
> + ++*idx;
> + xfs_iext_insert(ip, *idx, 1, &new, state);
> + break;
> + }
> +
> + ASSERT(da_old >= da_new);
> + da_diff = da_old - da_new;
> + if (!isrt)
> + da_diff += del->br_blockcount;
> + if (da_diff)
> + xfs_mod_fdblocks(mp, da_diff, false);
> + return error;
> +}
> +
> +void
> +xfs_bmap_del_extent_cow(
> + struct xfs_inode *ip,
> + xfs_extnum_t *idx,
> + struct xfs_bmbt_irec *got,
> + struct xfs_bmbt_irec *del)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> + struct xfs_bmbt_irec new;
> + xfs_fileoff_t del_endoff, got_endoff;
> + int state = BMAP_COWFORK;
> +
> + XFS_STATS_INC(mp, xs_del_exlist);
> +
> + del_endoff = del->br_startoff + del->br_blockcount;
> + got_endoff = got->br_startoff + got->br_blockcount;
> +
> + ASSERT(*idx >= 0);
> + ASSERT(*idx < ifp->if_bytes / sizeof(struct xfs_bmbt_rec));
> + ASSERT(del->br_blockcount > 0);
> + ASSERT(got->br_startoff <= del->br_startoff);
> + ASSERT(got_endoff >= del_endoff);
> + ASSERT(!isnullstartblock(got->br_startblock));
> +
> + if (got->br_startoff == del->br_startoff)
> + state |= BMAP_LEFT_CONTIG;
> + if (got_endoff == del_endoff)
> + state |= BMAP_RIGHT_CONTIG;
> +
> + switch (state & (BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG)) {
> + case BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG:
> + /*
> + * Matches the whole extent. Delete the entry.
> + */
> + xfs_iext_remove(ip, *idx, 1, state);
> + --*idx;
> + break;
> + case BMAP_LEFT_CONTIG:
> + /*
> + * Deleting the first part of the extent.
> + */
> + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> + got->br_startoff = del_endoff;
> + got->br_blockcount -= del->br_blockcount;
> + got->br_startblock = del->br_startblock + del->br_blockcount;
> + xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> + trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> + break;
> + case BMAP_RIGHT_CONTIG:
> + /*
> + * Deleting the last part of the extent.
> + */
> + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> + got->br_blockcount -= del->br_blockcount;
> + xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> + trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> + break;
> + case 0:
> + /*
> + * Deleting the middle of the extent.
> + */
> + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> + got->br_blockcount = del->br_startoff - got->br_startoff;
> + xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> + trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> +
> + new.br_startoff = del_endoff;
> + new.br_blockcount = got_endoff - del_endoff;
> + new.br_state = got->br_state;
> + new.br_startblock = del->br_startblock + del->br_blockcount;
> +
> + ++*idx;
> + xfs_iext_insert(ip, *idx, 1, &new, state);
> + break;
> + }
> +}
> +
> /*
> * Called by xfs_bmapi to update file extent records and the btree
> * after removing space (or undoing a delayed allocation).
> @@ -5210,167 +5423,20 @@ xfs_bunmapi_cow(
> struct xfs_inode *ip,
> struct xfs_bmbt_irec *del)
> {
> - xfs_filblks_t da_new;
> - xfs_filblks_t da_old;
> - xfs_fsblock_t del_endblock = 0;
> - xfs_fileoff_t del_endoff;
> - int delay;
> struct xfs_bmbt_rec_host *ep;
> - int error;
> struct xfs_bmbt_irec got;
> - xfs_fileoff_t got_endoff;
> - struct xfs_ifork *ifp;
> - struct xfs_mount *mp;
> - xfs_filblks_t nblks;
> struct xfs_bmbt_irec new;
> - /* REFERENCED */
> - uint qfield;
> - xfs_filblks_t temp;
> - xfs_filblks_t temp2;
> - int state = BMAP_COWFORK;
> int eof;
> xfs_extnum_t eidx;
>
> - mp = ip->i_mount;
> - XFS_STATS_INC(mp, xs_del_exlist);
> -
> ep = xfs_bmap_search_extents(ip, del->br_startoff, XFS_COW_FORK, &eof,
> - &eidx, &got, &new);
> -
> - ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK); ifp = ifp;
> - ASSERT((eidx >= 0) && (eidx < ifp->if_bytes /
> - (uint)sizeof(xfs_bmbt_rec_t)));
> - ASSERT(del->br_blockcount > 0);
> - ASSERT(got.br_startoff <= del->br_startoff);
> - del_endoff = del->br_startoff + del->br_blockcount;
> - got_endoff = got.br_startoff + got.br_blockcount;
> - ASSERT(got_endoff >= del_endoff);
> - delay = isnullstartblock(got.br_startblock);
> - ASSERT(isnullstartblock(del->br_startblock) == delay);
> - qfield = 0;
> - error = 0;
> - /*
> - * If deleting a real allocation, must free up the disk space.
> - */
> - if (!delay) {
> - nblks = del->br_blockcount;
> - qfield = XFS_TRANS_DQ_BCOUNT;
> - /*
> - * Set up del_endblock and cur for later.
> - */
> - del_endblock = del->br_startblock + del->br_blockcount;
> - da_old = da_new = 0;
> - } else {
> - da_old = startblockval(got.br_startblock);
> - da_new = 0;
> - nblks = 0;
> - }
> - qfield = qfield;
> - nblks = nblks;
> -
> - /*
> - * Set flag value to use in switch statement.
> - * Left-contig is 2, right-contig is 1.
> - */
> - switch (((got.br_startoff == del->br_startoff) << 1) |
> - (got_endoff == del_endoff)) {
> - case 3:
> - /*
> - * Matches the whole extent. Delete the entry.
> - */
> - xfs_iext_remove(ip, eidx, 1, BMAP_COWFORK);
> - --eidx;
> - break;
> -
> - case 2:
> - /*
> - * Deleting the first part of the extent.
> - */
> - trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
> - xfs_bmbt_set_startoff(ep, del_endoff);
> - temp = got.br_blockcount - del->br_blockcount;
> - xfs_bmbt_set_blockcount(ep, temp);
> - if (delay) {
> - temp = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip, temp),
> - da_old);
> - xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
> - trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> - da_new = temp;
> - break;
> - }
> - xfs_bmbt_set_startblock(ep, del_endblock);
> - trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> - break;
> -
> - case 1:
> - /*
> - * Deleting the last part of the extent.
> - */
> - temp = got.br_blockcount - del->br_blockcount;
> - trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
> - xfs_bmbt_set_blockcount(ep, temp);
> - if (delay) {
> - temp = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip, temp),
> - da_old);
> - xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
> - trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> - da_new = temp;
> - break;
> - }
> - trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> - break;
> -
> - case 0:
> - /*
> - * Deleting the middle of the extent.
> - */
> - temp = del->br_startoff - got.br_startoff;
> - trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
> - xfs_bmbt_set_blockcount(ep, temp);
> - new.br_startoff = del_endoff;
> - temp2 = got_endoff - del_endoff;
> - new.br_blockcount = temp2;
> - new.br_state = got.br_state;
> - if (!delay) {
> - new.br_startblock = del_endblock;
> - } else {
> - temp = xfs_bmap_worst_indlen(ip, temp);
> - xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
> - temp2 = xfs_bmap_worst_indlen(ip, temp2);
> - new.br_startblock = nullstartblock((int)temp2);
> - da_new = temp + temp2;
> - while (da_new > da_old) {
> - if (temp) {
> - temp--;
> - da_new--;
> - xfs_bmbt_set_startblock(ep,
> - nullstartblock((int)temp));
> - }
> - if (da_new == da_old)
> - break;
> - if (temp2) {
> - temp2--;
> - da_new--;
> - new.br_startblock =
> - nullstartblock((int)temp2);
> - }
> - }
> - }
> - trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> - xfs_iext_insert(ip, eidx + 1, 1, &new, state);
> - ++eidx;
> - break;
> - }
> -
> - /*
> - * Account for change in delayed indirect blocks.
> - * Nothing to do for disk quota accounting here.
> - */
> - ASSERT(da_old >= da_new);
> - if (da_old > da_new)
> - xfs_mod_fdblocks(mp, (int64_t)(da_old - da_new), false);
> -
> - return error;
> + &eidx, &got, &new);
> + ASSERT(ep);
> + if (isnullstartblock(got.br_startblock))
> + xfs_bmap_del_extent_delay(ip, XFS_COW_FORK, &eidx, &got, del);
> + else
> + xfs_bmap_del_extent_cow(ip, &eidx, &got, del);
> + return 0;
> }
>
> /*
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index eb86af0..5f18248 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -224,6 +224,11 @@ int xfs_bunmapi(struct xfs_trans *tp, struct xfs_inode *ip,
> xfs_extnum_t nexts, xfs_fsblock_t *firstblock,
> struct xfs_defer_ops *dfops, int *done);
> int xfs_bunmapi_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *del);
> +int xfs_bmap_del_extent_delay(struct xfs_inode *ip, int whichfork,
> + xfs_extnum_t *idx, struct xfs_bmbt_irec *got,
> + struct xfs_bmbt_irec *del);
> +void xfs_bmap_del_extent_cow(struct xfs_inode *ip, xfs_extnum_t *idx,
> + struct xfs_bmbt_irec *got, struct xfs_bmbt_irec *del);
> int xfs_check_nostate_extents(struct xfs_ifork *ifp, xfs_extnum_t idx,
> xfs_extnum_t num);
> uint xfs_default_attroffset(struct xfs_inode *ip);
> --
> 2.10.1.382.ga23ca1b
>
> --
> 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:[~2016-10-12 14:13 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-10 13:37 optimize the COW I/O path Christoph Hellwig
2016-10-10 13:37 ` [PATCH 1/9] iomap: add IOMAP_REPORT Christoph Hellwig
2016-10-11 1:45 ` Darrick J. Wong
2016-10-11 4:58 ` Christoph Hellwig
[not found] ` <20161011144557.GA16368@lst.de>
2016-10-11 16:22 ` Darrick J. Wong
2016-10-10 13:37 ` [PATCH 2/9] xfs: add xfs_trim_extent Christoph Hellwig
2016-10-10 13:37 ` [PATCH 3/9] xfs: handle "raw" delayed extents xfs_reflink_trim_around_shared Christoph Hellwig
2016-10-10 13:38 ` [PATCH 4/9] xfs: don't bother looking at the refcount tree for reads Christoph Hellwig
2016-10-10 13:38 ` [PATCH 5/9] xfs: optimize writes to reflink files Christoph Hellwig
2016-10-12 14:12 ` Brian Foster
2016-10-13 6:49 ` Christoph Hellwig
2016-10-13 13:26 ` Brian Foster
2016-10-13 18:28 ` Darrick J. Wong
2016-10-10 13:38 ` [PATCH 6/9] xfs: refactor xfs_bunmapi_cow Christoph Hellwig
2016-10-12 14:13 ` Brian Foster [this message]
2016-10-13 6:54 ` Christoph Hellwig
2016-10-13 13:26 ` Brian Foster
2016-10-10 13:38 ` [PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks Christoph Hellwig
2016-10-12 14:13 ` Brian Foster
2016-10-13 7:01 ` Christoph Hellwig
2016-10-10 13:38 ` [PATCH 8/9] xfs: optimize xfs_reflink_end_cow Christoph Hellwig
2016-10-12 14:15 ` Brian Foster
2016-10-13 7:06 ` Christoph Hellwig
2016-10-13 13:27 ` Brian Foster
2016-10-10 13:38 ` [PATCH 9/9] xfs: remove xfs_bunmapi_cow Christoph Hellwig
2016-10-15 8:52 optimize the COW I/O path V2 Christoph Hellwig
2016-10-15 8:52 ` [PATCH 6/9] xfs: refactor xfs_bunmapi_cow Christoph Hellwig
2016-10-17 17:21 ` Brian Foster
2016-10-17 17:44 ` Christoph Hellwig
2016-10-17 17:53 ` Brian Foster
2016-10-17 18:32 ` 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=20161012141308.GB56019@bfoster.bfoster \
--to=bfoster@redhat.com \
--cc=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.