Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag

[Brian Foster <bfoster@redhat.com>]

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 <bfoster@redhat.com>
> > > ---
> > >  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 <darrick.wong@oracle.com>
> > 
> > --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