On Mon, Apr 03, 2017 at 02:18:31PM +0200, Christoph Hellwig wrote:
> Add a new helper to be used for reflink extent list additions instead of
> funneling them through xfs_bmapi_write and overloading the firstblock
> member in struct xfs_bmalloca and struct xfs_alloc_args.
>
> With some small changes to xfs_bmap_remap_alloc this also means we do
> not need a xfs_bmalloca structure for this case at all.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 152 +++++++++++++++++++++++++++++++++--------------
> 1 file changed, 107 insertions(+), 45 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index d404dee7ede4..caadfe27af39 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3862,11 +3862,12 @@ xfs_bmap_btalloc(
> */
> STATIC int
> xfs_bmap_remap_alloc(
> - struct xfs_bmalloca *ap)
> + struct xfs_trans *tp,
> + struct xfs_inode *ip,
> + xfs_fsblock_t startblock,
> + xfs_extlen_t length)
Why change all this if the next patch removes the whole function except:
ip->i_d.di_nblocks += length;
xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
Wouldn't it make more sense to have this patch add _bmapi_remap (as it
appears in the next patch) and change the callers to use it; and then
the next patch removes the old _bmap_remap_alloc and its callers?
> {
> - struct xfs_trans *tp = ap->tp;
> struct xfs_mount *mp = tp->t_mountp;
> - xfs_fsblock_t bno;
> struct xfs_alloc_arg args;
> int error;
>
> @@ -3875,24 +3876,24 @@ xfs_bmap_remap_alloc(
> * and handle a silent filesystem corruption rather than crashing.
> */
> memset(&args, 0, sizeof(struct xfs_alloc_arg));
> - args.tp = ap->tp;
> - args.mp = ap->tp->t_mountp;
> - bno = *ap->firstblock;
> - args.agno = XFS_FSB_TO_AGNO(mp, bno);
> - args.agbno = XFS_FSB_TO_AGBNO(mp, bno);
> + args.tp = tp;
> + args.mp = mp;
> + args.agno = XFS_FSB_TO_AGNO(mp, startblock);
> + args.agbno = XFS_FSB_TO_AGBNO(mp, startblock);
> +
> if (args.agno >= mp->m_sb.sb_agcount ||
> args.agbno >= mp->m_sb.sb_agblocks)
> return -EFSCORRUPTED;
>
> /* "Allocate" the extent from the range we passed in. */
> - trace_xfs_bmap_remap_alloc(ap->ip, *ap->firstblock, ap->length);
> - ap->blkno = bno;
> - ap->ip->i_d.di_nblocks += ap->length;
> - xfs_trans_log_inode(ap->tp, ap->ip, XFS_ILOG_CORE);
> + trace_xfs_bmap_remap_alloc(ip, startblock, length);
> +
> + ip->i_d.di_nblocks += length;
> + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>
> /* Fix the freelist, like a real allocator does. */
> - args.datatype = ap->datatype;
> - args.pag = xfs_perag_get(args.mp, args.agno);
> + args.datatype = XFS_ALLOC_USERDATA | XFS_ALLOC_NOBUSY;
> + args.pag = xfs_perag_get(mp, args.agno);
> ASSERT(args.pag);
>
> /*
> @@ -3905,7 +3906,7 @@ xfs_bmap_remap_alloc(
> error = xfs_alloc_fix_freelist(&args, XFS_ALLOC_FLAG_FREEING);
> xfs_perag_put(args.pag);
> if (error)
> - trace_xfs_bmap_remap_alloc_error(ap->ip, error, _RET_IP_);
> + trace_xfs_bmap_remap_alloc_error(ip, error, _RET_IP_);
> return error;
> }
>
> @@ -3917,8 +3918,6 @@ STATIC int
> xfs_bmap_alloc(
> struct xfs_bmalloca *ap) /* bmap alloc argument struct */
> {
> - if (ap->flags & XFS_BMAPI_REMAP)
> - return xfs_bmap_remap_alloc(ap);
> if (XFS_IS_REALTIME_INODE(ap->ip) &&
> xfs_alloc_is_userdata(ap->datatype))
> return xfs_bmap_rtalloc(ap);
> @@ -4554,9 +4553,7 @@ xfs_bmapi_write(
> ASSERT(len > 0);
> ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> - ASSERT(!(flags & XFS_BMAPI_REMAP) || whichfork == XFS_DATA_FORK);
> - ASSERT(!(flags & XFS_BMAPI_PREALLOC) || !(flags & XFS_BMAPI_REMAP));
> - ASSERT(!(flags & XFS_BMAPI_CONVERT) || !(flags & XFS_BMAPI_REMAP));
> + ASSERT(!(flags & XFS_BMAPI_REMAP));
>
> /* zeroing is for currently only for data extents, not metadata */
> ASSERT((flags & (XFS_BMAPI_METADATA | XFS_BMAPI_ZERO)) !=
> @@ -4640,13 +4637,8 @@ xfs_bmapi_write(
> } else {
> need_alloc = true;
> }
> - } else {
> - /*
> - * Make sure we only reflink into a hole.
> - */
> - ASSERT(!(flags & XFS_BMAPI_REMAP));
> - if (isnullstartblock(bma.got.br_startblock))
> - wasdelay = true;
> + } else if (isnullstartblock(bma.got.br_startblock)) {
> + wasdelay = true;
> }
>
> /*
> @@ -4775,6 +4767,89 @@ xfs_bmapi_write(
> return error;
> }
>
> +static int
> +xfs_bmapi_remap(
> + struct xfs_trans *tp,
> + struct xfs_inode *ip,
> + xfs_fileoff_t bno,
> + xfs_filblks_t len,
> + xfs_fsblock_t startblock,
> + struct xfs_defer_ops *dfops)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> + struct xfs_btree_cur *cur = NULL;
> + xfs_fsblock_t firstblock = NULLFSBLOCK;
> + struct xfs_bmbt_irec got;
> + xfs_extnum_t idx;
> + int logflags = 0, error;
> +
> + ASSERT(len > 0);
> + ASSERT(len <= (xfs_filblks_t)MAXEXTLEN);
> + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + ASSERT(ifp->if_flags & XFS_IFEXTENTS);
> +
> + if (unlikely(XFS_TEST_ERROR(
> + (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
> + XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
> + mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
> + XFS_ERROR_REPORT("xfs_bmapi_remap", XFS_ERRLEVEL_LOW, mp);
> + return -EFSCORRUPTED;
> + }
> +
> + if (XFS_FORCED_SHUTDOWN(mp))
> + return -EIO;
> +
> + if (xfs_iext_lookup_extent(ip, ifp, bno, &idx, &got)) {
> + /* make sure we only reflink into a hole. */
> + ASSERT(got.br_startoff > bno);
> + ASSERT(got.br_startoff - bno >= len);
> + }
> +
> + error = xfs_bmap_remap_alloc(tp, ip, startblock, len);
> + if (error)
> + goto error0;
> +
> + if (ifp->if_flags & XFS_IFBROOT) {
> + cur = xfs_bmbt_init_cursor(mp, tp, ip, XFS_DATA_FORK);
> + cur->bc_private.b.firstblock = firstblock;
> + cur->bc_private.b.dfops = dfops;
> + cur->bc_private.b.flags = 0;
> + }
> +
> + got.br_startoff = bno;
> + got.br_startblock = startblock;
> + got.br_blockcount = len;
> + got.br_state = XFS_EXT_NORM;
> +
> + error = xfs_bmap_add_extent_hole_real(tp, ip, XFS_DATA_FORK, &idx, &cur,
> + &got, &firstblock, dfops, &logflags);
> + if (error)
> + goto error0;
> +
> + if (xfs_bmap_wants_extents(ip, XFS_DATA_FORK)) {
> + int tmp_logflags = 0;
> +
> + error = xfs_bmap_btree_to_extents(tp, ip, cur,
> + &tmp_logflags, XFS_DATA_FORK);
> + logflags |= tmp_logflags;
> + }
> +
> +error0:
> + if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS)
> + logflags &= ~XFS_ILOG_DEXT;
> + else if (ip->i_d.di_format != XFS_DINODE_FMT_BTREE)
> + logflags &= ~XFS_ILOG_DBROOT;
> +
> + if (logflags)
> + xfs_trans_log_inode(tp, ip, logflags);
> + if (cur) {
> + xfs_btree_del_cursor(cur,
> + error ? XFS_BTREE_ERROR : XFS_BTREE_NOERROR);
Double indent here?
xfs_btree_del_cursor(cur,
error ? ...);
Especially since you do that in one of the subsequent hunks.
> + }
> + return error;
I think the code in this function looks ok aside from my earlier
comments.
--D
> +}
> +
> /*
> * When a delalloc extent is split (e.g., due to a hole punch), the original
> * indlen reservation must be shared across the two new extents that are left
> @@ -6493,16 +6568,7 @@ xfs_bmap_finish_one(
> xfs_filblks_t blockcount,
> xfs_exntst_t state)
> {
> - struct xfs_bmbt_irec bmap;
> - int nimaps = 1;
> - xfs_fsblock_t firstfsb;
> - int done;
> - int error = 0;
> -
> - bmap.br_startblock = startblock;
> - bmap.br_startoff = startoff;
> - bmap.br_blockcount = blockcount;
> - bmap.br_state = state;
> + int error = 0, done;
>
> trace_xfs_bmap_deferred(tp->t_mountp,
> XFS_FSB_TO_AGNO(tp->t_mountp, startblock), type,
> @@ -6519,16 +6585,12 @@ xfs_bmap_finish_one(
>
> switch (type) {
> case XFS_BMAP_MAP:
> - firstfsb = bmap.br_startblock;
> - error = xfs_bmapi_write(tp, ip, bmap.br_startoff,
> - bmap.br_blockcount, XFS_BMAPI_REMAP, &firstfsb,
> - bmap.br_blockcount, &bmap, &nimaps,
> - dfops);
> + error = xfs_bmapi_remap(tp, ip, startoff, blockcount,
> + startblock, dfops);
> break;
> case XFS_BMAP_UNMAP:
> - error = xfs_bunmapi(tp, ip, bmap.br_startoff,
> - bmap.br_blockcount, XFS_BMAPI_REMAP, 1, &firstfsb,
> - dfops, &done);
> + error = xfs_bunmapi(tp, ip, startoff, blockcount,
> + XFS_BMAPI_REMAP, 1, &startblock, dfops, &done);
> ASSERT(done);
> break;
> default:
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
