| Message ID | 20171207185810.48757-4-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 07, 2017 at 01:58:09PM -0500, Brian Foster wrote: > Defer AGFL block frees from deferred extent free context. This means > that extents that are deferred freed via xfs_bmap_add_free() will > add additional deferred items for AGFL block frees during completion > processing. All such items complete before xfs_defer_finish() > returns. > > Update xfs_trans_free_extent() and xfs_free_extent() to receive an > optional dfops pointer and pass it down to the AGFL fixup code via > the allocation arguments structure. Update > xfs_extent_free_finish_item() to pass along the dfops list currently > being processed by xfs_defer_finish(). All other callers pass a NULL > dfops and so do not change behavior. > > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/libxfs/xfs_alloc.c | 9 ++++++--- > fs/xfs/libxfs/xfs_alloc.h | 5 +++-- > fs/xfs/libxfs/xfs_ialloc_btree.c | 2 +- > fs/xfs/libxfs/xfs_refcount_btree.c | 2 +- > fs/xfs/libxfs/xfs_rmap.c | 2 +- > fs/xfs/xfs_extfree_item.c | 2 +- > fs/xfs/xfs_fsops.c | 2 +- > fs/xfs/xfs_trans.h | 3 ++- > fs/xfs/xfs_trans_extfree.c | 7 ++++--- > 9 files changed, 20 insertions(+), 14 deletions(-) Rather than passing the dfops structure around, I'm starting to wonder it it makes more sense to attach it to the struct xfs_trans we pass around to all these functions? That would mean it doesn't need to be manually plumbed into any of this code - it would be directly available in any transaction context that has a dfops associated with it. That would mean all agfl fixups would be able to be deferred without modifying any of the intermediate code paths as all allocation/free transactions require a dfops structure.... Cheers, Dave.
On Fri, Dec 08, 2017 at 09:49:41AM +1100, Dave Chinner wrote: > On Thu, Dec 07, 2017 at 01:58:09PM -0500, Brian Foster wrote: > > Defer AGFL block frees from deferred extent free context. This means > > that extents that are deferred freed via xfs_bmap_add_free() will > > add additional deferred items for AGFL block frees during completion > > processing. All such items complete before xfs_defer_finish() > > returns. > > > > Update xfs_trans_free_extent() and xfs_free_extent() to receive an > > optional dfops pointer and pass it down to the AGFL fixup code via > > the allocation arguments structure. Update > > xfs_extent_free_finish_item() to pass along the dfops list currently > > being processed by xfs_defer_finish(). All other callers pass a NULL > > dfops and so do not change behavior. > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > --- > > fs/xfs/libxfs/xfs_alloc.c | 9 ++++++--- > > fs/xfs/libxfs/xfs_alloc.h | 5 +++-- > > fs/xfs/libxfs/xfs_ialloc_btree.c | 2 +- > > fs/xfs/libxfs/xfs_refcount_btree.c | 2 +- > > fs/xfs/libxfs/xfs_rmap.c | 2 +- > > fs/xfs/xfs_extfree_item.c | 2 +- > > fs/xfs/xfs_fsops.c | 2 +- > > fs/xfs/xfs_trans.h | 3 ++- > > fs/xfs/xfs_trans_extfree.c | 7 ++++--- > > 9 files changed, 20 insertions(+), 14 deletions(-) > > Rather than passing the dfops structure around, I'm starting to > wonder it it makes more sense to attach it to the struct xfs_trans > we pass around to all these functions? That would mean it doesn't > need to be manually plumbed into any of this code - it would be > directly available in any transaction context that has a dfops > associated with it. > Yeah, that sounds like a nice cleanup. > That would mean all agfl fixups would be able to be deferred without > modifying any of the intermediate code paths as all allocation/free > transactions require a dfops structure.... > Ok.. so separate from the refactoring, it sounds like you're suggesting that we should try to always defer AGFL frees rather than doing so very selectively (as the current code does). I may still try to implement that incrementally, but I suppose that may depend on what falls out from the above cleanup. Otherwise that sounds reasonable to me. I'm heading into some time off, but I'll look more into the above and other fixups once I get back to this. Thanks for the review! Brian > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > -- > 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 -- 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
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c index ab636181471c..eafcef839526 100644 --- a/fs/xfs/libxfs/xfs_alloc.c +++ b/fs/xfs/libxfs/xfs_alloc.c @@ -2857,7 +2857,8 @@ int xfs_free_extent_fix_freelist( struct xfs_trans *tp, xfs_agnumber_t agno, - struct xfs_buf **agbp) + struct xfs_buf **agbp, + struct xfs_defer_ops *dfops) { struct xfs_alloc_arg args; int error; @@ -2866,6 +2867,7 @@ xfs_free_extent_fix_freelist( args.tp = tp; args.mp = tp->t_mountp; args.agno = agno; + args.dfops = dfops; /* * validate that the block number is legal - the enables us to detect @@ -2898,7 +2900,8 @@ xfs_free_extent( xfs_fsblock_t bno, /* starting block number of extent */ xfs_extlen_t len, /* length of extent */ struct xfs_owner_info *oinfo, /* extent owner */ - enum xfs_ag_resv_type type) /* block reservation type */ + enum xfs_ag_resv_type type, /* block reservation type */ + struct xfs_defer_ops *dfops) { struct xfs_mount *mp = tp->t_mountp; struct xfs_buf *agbp; @@ -2913,7 +2916,7 @@ xfs_free_extent( XFS_ERRTAG_FREE_EXTENT)) return -EIO; - error = xfs_free_extent_fix_freelist(tp, agno, &agbp); + error = xfs_free_extent_fix_freelist(tp, agno, &agbp, dfops); if (error) return error; diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h index 559568806265..17fc0265fbfa 100644 --- a/fs/xfs/libxfs/xfs_alloc.h +++ b/fs/xfs/libxfs/xfs_alloc.h @@ -196,7 +196,8 @@ xfs_free_extent( xfs_fsblock_t bno, /* starting block number of extent */ xfs_extlen_t len, /* length of extent */ struct xfs_owner_info *oinfo, /* extent owner */ - enum xfs_ag_resv_type type); /* block reservation type */ + enum xfs_ag_resv_type type, /* block reservation type */ + struct xfs_defer_ops *dfops);/* dfops for agfl frees */ int /* error */ xfs_alloc_lookup_ge( @@ -220,7 +221,7 @@ int xfs_free_agfl_block(struct xfs_trans *, xfs_agnumber_t, xfs_agblock_t, struct xfs_buf *, struct xfs_owner_info *); int xfs_alloc_fix_freelist(struct xfs_alloc_arg *args, int flags); int xfs_free_extent_fix_freelist(struct xfs_trans *tp, xfs_agnumber_t agno, - struct xfs_buf **agbp); + struct xfs_buf **agbp, struct xfs_defer_ops *); xfs_extlen_t xfs_prealloc_blocks(struct xfs_mount *mp); diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c index 317caba9faa6..b899f02e5c5e 100644 --- a/fs/xfs/libxfs/xfs_ialloc_btree.c +++ b/fs/xfs/libxfs/xfs_ialloc_btree.c @@ -155,7 +155,7 @@ xfs_inobt_free_block( xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_INOBT); return xfs_free_extent(cur->bc_tp, XFS_DADDR_TO_FSB(cur->bc_mp, XFS_BUF_ADDR(bp)), 1, - &oinfo, XFS_AG_RESV_NONE); + &oinfo, XFS_AG_RESV_NONE, NULL); } STATIC int diff --git a/fs/xfs/libxfs/xfs_refcount_btree.c b/fs/xfs/libxfs/xfs_refcount_btree.c index 3c59dd3d58d7..0f1ff83d0fb8 100644 --- a/fs/xfs/libxfs/xfs_refcount_btree.c +++ b/fs/xfs/libxfs/xfs_refcount_btree.c @@ -136,7 +136,7 @@ xfs_refcountbt_free_block( be32_add_cpu(&agf->agf_refcount_blocks, -1); xfs_alloc_log_agf(cur->bc_tp, agbp, XFS_AGF_REFCOUNT_BLOCKS); error = xfs_free_extent(cur->bc_tp, fsbno, 1, &oinfo, - XFS_AG_RESV_METADATA); + XFS_AG_RESV_METADATA, NULL); if (error) return error; diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c index dd019cee1b3b..439f90d70850 100644 --- a/fs/xfs/libxfs/xfs_rmap.c +++ b/fs/xfs/libxfs/xfs_rmap.c @@ -2107,7 +2107,7 @@ xfs_rmap_finish_one( * rmapbt, because a shape change could cause us to * allocate blocks. */ - error = xfs_free_extent_fix_freelist(tp, agno, &agbp); + error = xfs_free_extent_fix_freelist(tp, agno, &agbp, NULL); if (error) return error; if (!agbp) diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c index 44f8c5451210..100dce7f6f17 100644 --- a/fs/xfs/xfs_extfree_item.c +++ b/fs/xfs/xfs_extfree_item.c @@ -542,7 +542,7 @@ xfs_efi_recover( for (i = 0; i < efip->efi_format.efi_nextents; i++) { extp = &efip->efi_format.efi_extents[i]; error = xfs_trans_free_extent(tp, efdp, extp->ext_start, - extp->ext_len, &oinfo); + extp->ext_len, &oinfo, NULL); if (error) goto abort_error; diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c index 8f22fc579dbb..c50ecf684938 100644 --- a/fs/xfs/xfs_fsops.c +++ b/fs/xfs/xfs_fsops.c @@ -574,7 +574,7 @@ xfs_growfs_data_private( error = xfs_free_extent(tp, XFS_AGB_TO_FSB(mp, agno, be32_to_cpu(agf->agf_length) - new), - new, &oinfo, XFS_AG_RESV_NONE); + new, &oinfo, XFS_AG_RESV_NONE, NULL); if (error) goto error0; } diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h index 815b53d20e26..88c5357d558c 100644 --- a/fs/xfs/xfs_trans.h +++ b/fs/xfs/xfs_trans.h @@ -228,7 +228,8 @@ struct xfs_efd_log_item *xfs_trans_get_efd(struct xfs_trans *, uint); int xfs_trans_free_extent(struct xfs_trans *, struct xfs_efd_log_item *, xfs_fsblock_t, - xfs_extlen_t, struct xfs_owner_info *); + xfs_extlen_t, struct xfs_owner_info *, + struct xfs_defer_ops *); int xfs_trans_commit(struct xfs_trans *); int xfs_trans_roll(struct xfs_trans **); int xfs_trans_roll_inode(struct xfs_trans **, struct xfs_inode *); diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c index f5620796ae25..04cd3f7e3c02 100644 --- a/fs/xfs/xfs_trans_extfree.c +++ b/fs/xfs/xfs_trans_extfree.c @@ -68,7 +68,8 @@ xfs_trans_free_extent( struct xfs_efd_log_item *efdp, xfs_fsblock_t start_block, xfs_extlen_t ext_len, - struct xfs_owner_info *oinfo) + struct xfs_owner_info *oinfo, + struct xfs_defer_ops *dfops) { struct xfs_mount *mp = tp->t_mountp; uint next_extent; @@ -80,7 +81,7 @@ xfs_trans_free_extent( trace_xfs_bmap_free_deferred(tp->t_mountp, agno, 0, agbno, ext_len); error = xfs_free_extent(tp, start_block, ext_len, oinfo, - XFS_AG_RESV_NONE); + XFS_AG_RESV_NONE, dfops); /* * Mark the transaction dirty, even on error. This ensures the @@ -195,7 +196,7 @@ xfs_extent_free_finish_item( error = xfs_trans_free_extent(tp, done_item, free->xefi_startblock, free->xefi_blockcount, - &free->xefi_oinfo); + &free->xefi_oinfo, dop); kmem_free(free); return error; }
Defer AGFL block frees from deferred extent free context. This means that extents that are deferred freed via xfs_bmap_add_free() will add additional deferred items for AGFL block frees during completion processing. All such items complete before xfs_defer_finish() returns. Update xfs_trans_free_extent() and xfs_free_extent() to receive an optional dfops pointer and pass it down to the AGFL fixup code via the allocation arguments structure. Update xfs_extent_free_finish_item() to pass along the dfops list currently being processed by xfs_defer_finish(). All other callers pass a NULL dfops and so do not change behavior. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/libxfs/xfs_alloc.c | 9 ++++++--- fs/xfs/libxfs/xfs_alloc.h | 5 +++-- fs/xfs/libxfs/xfs_ialloc_btree.c | 2 +- fs/xfs/libxfs/xfs_refcount_btree.c | 2 +- fs/xfs/libxfs/xfs_rmap.c | 2 +- fs/xfs/xfs_extfree_item.c | 2 +- fs/xfs/xfs_fsops.c | 2 +- fs/xfs/xfs_trans.h | 3 ++- fs/xfs/xfs_trans_extfree.c | 7 ++++--- 9 files changed, 20 insertions(+), 14 deletions(-)