From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:54503 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750767AbeEIK4l (ORCPT ); Wed, 9 May 2018 06:56:41 -0400 Date: Wed, 9 May 2018 06:56:39 -0400 From: Brian Foster Subject: Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag Message-ID: <20180509105638.GC64624@bfoster.bfoster> References: <20180508172231.53570-1-bfoster@redhat.com> <20180508172231.53570-2-bfoster@redhat.com> <20180508181242.GW11261@magnolia> <20180509012808.GD11261@magnolia> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180509012808.GD11261@magnolia> Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: "Darrick J. Wong" Cc: linux-xfs@vger.kernel.org On Tue, May 08, 2018 at 06:28:08PM -0700, Darrick J. Wong wrote: > On Tue, May 08, 2018 at 11:12:42AM -0700, Darrick J. Wong wrote: > > On Tue, May 08, 2018 at 01:22:29PM -0400, Brian Foster wrote: > > > Freed extents are unconditionally discarded when online discard is > > > enabled. Define XFS_BMAPI_NODISCARD to allow callers to bypass > > > discards when unnecessary. For example, this will be useful for > > > eofblocks trimming. > > > > > > This patch does not change behavior. > > > > > > Signed-off-by: Brian Foster > > > --- > > > fs/xfs/libxfs/xfs_alloc.c | 10 +++++++--- > > > fs/xfs/libxfs/xfs_alloc.h | 27 +++++++++++++++++++++++++-- > > > fs/xfs/libxfs/xfs_bmap.c | 17 +++++++++++++---- > > > fs/xfs/libxfs/xfs_bmap.h | 30 ++++++++++++++++++++++++++++-- > > > fs/xfs/xfs_extfree_item.c | 2 +- > > > fs/xfs/xfs_trans.h | 3 ++- > > > fs/xfs/xfs_trans_extfree.c | 13 +++++++++---- > > > 7 files changed, 85 insertions(+), 17 deletions(-) > > > ... > > > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h > > > index 2b766b37096d..d8832e049636 100644 > > > --- a/fs/xfs/libxfs/xfs_bmap.h > > > +++ b/fs/xfs/libxfs/xfs_bmap.h > > > @@ -68,6 +68,7 @@ struct xfs_extent_free_item > > > xfs_extlen_t xefi_blockcount;/* number of blocks in extent */ > > > struct list_head xefi_list; > > > struct xfs_owner_info xefi_oinfo; /* extent owner */ > > > + bool xefi_skip_discard; > > > }; > > > > > > #define XFS_BMAP_MAX_NMAP 4 > > > @@ -116,6 +117,9 @@ struct xfs_extent_free_item > > > /* Only convert unwritten extents, don't allocate new blocks */ > > > #define XFS_BMAPI_CONVERT_ONLY 0x800 > > > > > > +/* Skip online discard of freed extents */ > > > +#define XFS_BMAPI_NODISCARD 0x1000 > > > + > > > #define XFS_BMAPI_FLAGS \ > > > { XFS_BMAPI_ENTIRE, "ENTIRE" }, \ > > > { XFS_BMAPI_METADATA, "METADATA" }, \ > > > > XFS_BMAPI_FLAGS needs to be updated ot have XFS_BMAPI_NODISCARD. > > Ok, truncated thought there... > > XFS_BMAPI_FLAGS needs to be updated to include a string for > XFS_BMAPI_NODISCARD. > Yep, sorry. I should have caught that given it's right beneath the flag definition. :P > Should I just add: > > { XFS_BMAPI_NODISCARD, "NODISCARD" }, \ > > at the end of XFS_BMAPI_FLAGS? > Please do, I've made the change locally as well. > Granted I kinda hate these, because the C definition is emitted as a > string as part of every tracepoint definition in the xfs module and again > for every trace point definition in the ftrace binary stream, but I > guess people find the flags translation useful? I ripped all of mine > out prior to submitting reflink/rmap/scrub... > FWIW, in general I find these string translations useful when sifting through a large enough pile of related trace data. It's much easier to quickly and correctly grok the state/flags. I suppose it would be nice if we could somehow centralize the data structure that sources the strings, but on a quick look it seems that the trace infrastructure expects this macro thing in order to create its own internal structure. :/ Brian > --D > > > > > Looks ok otherwise, > > Reviewed-by: Darrick J. Wong > > > > --D > > > > > @@ -192,9 +196,9 @@ void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, > > > void xfs_trim_extent_eof(struct xfs_bmbt_irec *, struct xfs_inode *); > > > int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); > > > void xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork); > > > -void xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops, > > > +void __xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops, > > > xfs_fsblock_t bno, xfs_filblks_t len, > > > - struct xfs_owner_info *oinfo); > > > + struct xfs_owner_info *oinfo, bool skip_discard); > > > void xfs_bmap_compute_maxlevels(struct xfs_mount *mp, int whichfork); > > > int xfs_bmap_first_unused(struct xfs_trans *tp, struct xfs_inode *ip, > > > xfs_extlen_t len, xfs_fileoff_t *unused, int whichfork); > > > @@ -240,6 +244,28 @@ int xfs_bmapi_reserve_delalloc(struct xfs_inode *ip, int whichfork, > > > struct xfs_bmbt_irec *got, struct xfs_iext_cursor *cur, > > > int eof); > > > > > > +static inline void > > > +xfs_bmap_add_free( > > > + struct xfs_mount *mp, > > > + struct xfs_defer_ops *dfops, > > > + xfs_fsblock_t bno, > > > + xfs_filblks_t len, > > > + struct xfs_owner_info *oinfo) > > > +{ > > > + __xfs_bmap_add_free(mp, dfops, bno, len, oinfo, false); > > > +} > > > + > > > +static inline void > > > +xfs_bmap_add_free_nodiscard( > > > + struct xfs_mount *mp, > > > + struct xfs_defer_ops *dfops, > > > + xfs_fsblock_t bno, > > > + xfs_filblks_t len, > > > + struct xfs_owner_info *oinfo) > > > +{ > > > + __xfs_bmap_add_free(mp, dfops, bno, len, oinfo, true); > > > +} > > > + > > > enum xfs_bmap_intent_type { > > > XFS_BMAP_MAP = 1, > > > XFS_BMAP_UNMAP, > > > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c > > > index b5b1e567b9f4..4735a31793b0 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, false); > > > if (error) > > > goto abort_error; > > > > > > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h > > > index 9d542dfe0052..d5be3f6a3e8f 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 *, > > > + bool); > > > 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 ab438647592a..dab8b3b7a9b8 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, > > > + bool skip_discard) > > > { > > > struct xfs_mount *mp = tp->t_mountp; > > > uint next_extent; > > > @@ -79,8 +80,12 @@ 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); > > > + if (skip_discard) > > > + error = xfs_free_extent_nodiscard(tp, start_block, ext_len, > > > + oinfo, XFS_AG_RESV_NONE); > > > + else > > > + error = xfs_free_extent(tp, start_block, ext_len, oinfo, > > > + XFS_AG_RESV_NONE); > > > > > > /* > > > * Mark the transaction dirty, even on error. This ensures the > > > @@ -195,7 +200,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, free->xefi_skip_discard); > > > kmem_free(free); > > > return error; > > > } > > > -- > > > 2.14.3 > > > > > > -- > > > 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 > -- > 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