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

[Brian Foster <bfoster@redhat.com>]

On Wed, May 09, 2018 at 12:46:29AM -0700, Christoph Hellwig 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_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index 4bcc095fe44a..06e4b0c47c01 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -2949,18 +2949,20 @@ xfs_free_extent_fix_freelist(
> >   * after fixing up the freelist.
> >   */
> >  int				/* error */
> > -xfs_free_extent(
> > +__xfs_free_extent(
> >  	struct xfs_trans	*tp,	/* transaction pointer */
> >  	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 */
> > +	bool			skip_discard)
> >  {
> >  	struct xfs_mount	*mp = tp->t_mountp;
> >  	struct xfs_buf		*agbp;
> >  	xfs_agnumber_t		agno = XFS_FSB_TO_AGNO(mp, bno);
> >  	xfs_agblock_t		agbno = XFS_FSB_TO_AGBNO(mp, bno);
> >  	int			error;
> > +	unsigned int		busy_flags = 0;
> >  
> >  	ASSERT(len != 0);
> >  	ASSERT(type != XFS_AG_RESV_AGFL);
> > @@ -2984,7 +2986,9 @@ xfs_free_extent(
> >  	if (error)
> >  		goto err;
> >  
> > -	xfs_extent_busy_insert(tp, agno, agbno, len, 0);
> > +	if (skip_discard)
> > +		busy_flags |= XFS_EXTENT_BUSY_SKIP_DISCARD;
> 
> Please just add an busy_flags argument to xfs_free_extent, and pass 0 in
> the oter callers.  That removes a lot of the boilerplate code.  Then just
> add an assert to xfs_free_extent that no flag other than
> XFS_EXTENT_BUSY_SKIP_DISCARD is passed for now.
> 

I deliberately went away from that approach based on Dave's feedback
(similarly with the xfs_itruncate_extents() comment in the subsequent
patch). I used the bool that is used here rather than pass the flag
directly, but it's essentially the same idea in terms of exposing the
function signature change to callers.

TBH, I don't much care which of the N possible variants of code
factoring we use here, but I think we're officially bikeshedding once
the feedback starts to go in circles as such. ;) So unless I see some
broader/mutual agreement (or a strong preference from the maintainer) on
a change back to something like the original factoring (perhaps using a
flag instead of a bool), I'm going to assume changing it back will
simply reintroduce the previous feedback and leave this as is.

Brian

> >  void
> > -xfs_bmap_add_free(
> > +__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)
> >  {
> >  	struct xfs_extent_free_item	*new;		/* new element */
> >  #ifdef DEBUG
> > @@ -574,6 +575,7 @@ xfs_bmap_add_free(
> >  		new->xefi_oinfo = *oinfo;
> >  	else
> >  		xfs_rmap_skip_owner_update(&new->xefi_oinfo);
> > +	new->xefi_skip_discard = skip_discard;
> 
> Similarly here, I think we should just pass through the bmapi
> flags.  But given the number of callers I think just adding
> a xfs_bmap_add_free_flags as the low-level interface with a higher
> level xfs_bmap_add_free wrapper for all other callers is fine.
> --
> 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