Re: [PATCH v4 05/11] xfs: refactor cntbt lastblock scan best extent logic into helper

[Brian Foster <bfoster@redhat.com>]

On Wed, Sep 18, 2019 at 12:03:41PM -0700, Darrick J. Wong wrote:
> On Mon, Sep 16, 2019 at 08:16:29AM -0400, Brian Foster wrote:
> > The cntbt lastblock scan checks the size, alignment, locality, etc.
> > of each free extent in the block and compares it with the current
> > best candidate. This logic will be reused by the upcoming optimized
> > cntbt algorithm, so refactor it into a separate helper.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_alloc.c | 113 +++++++++++++++++++++++++++++---------
> >  fs/xfs/xfs_trace.h        |  25 +++++++++
> >  2 files changed, 111 insertions(+), 27 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index ee46989ab723..2fa7bb6a00a8 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -791,6 +791,89 @@ xfs_alloc_cur_close(
> >  	acur->cnt = acur->bnolt = acur->bnogt = NULL;
> >  }
> >  
> > +/*
> > + * Check an extent for allocation and track the best available candidate in the
> > + * allocation structure. The cursor is deactivated if it has entered an out of
> > + * range state based on allocation arguments. Optionally return the extent
> > + * extent geometry and allocation status if requested by the caller.
> > + */
> > +static int
> > +xfs_alloc_cur_check(
> > +	struct xfs_alloc_arg		*args,
> > +	struct xfs_alloc_cur		*acur,
> > +	struct xfs_btree_cur		*cur,
> > +	int				*new)
> > +{
> > +	int			error, i;
> 
> Inconsistent indentation here.
> 

Fixed.

> > +	xfs_agblock_t		bno, bnoa, bnew;
> > +	xfs_extlen_t		len, lena, diff = -1;
> > +	bool			busy;
> > +	unsigned		busy_gen = 0;
> > +	bool			deactivate = false;
> > +
> > +	*new = 0;
> > +
...
> > +}
> > +
> >  /*
> >   * Deal with the case where only small freespaces remain. Either return the
> >   * contents of the last freespace record, or allocate space from the freelist if
...
> > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> > index eaae275ed430..b12fad3e45cb 100644
> > --- a/fs/xfs/xfs_trace.h
> > +++ b/fs/xfs/xfs_trace.h
> > @@ -1663,6 +1663,31 @@ DEFINE_ALLOC_EVENT(xfs_alloc_vextent_noagbp);
> >  DEFINE_ALLOC_EVENT(xfs_alloc_vextent_loopfailed);
> >  DEFINE_ALLOC_EVENT(xfs_alloc_vextent_allfailed);
> >  
> > +TRACE_EVENT(xfs_alloc_cur_check,
> > +	TP_PROTO(struct xfs_mount *mp, xfs_btnum_t btnum, xfs_agblock_t bno,
> > +		 xfs_extlen_t len, xfs_extlen_t diff, bool new),
> > +	TP_ARGS(mp, btnum, bno, len, diff, new),
> > +	TP_STRUCT__entry(
> > +		__field(dev_t, dev)
> > +		__field(xfs_btnum_t, btnum)
> > +		__field(xfs_agblock_t, bno)
> > +		__field(xfs_extlen_t, len)
> > +		__field(xfs_extlen_t, diff)
> > +		__field(bool, new)
> > +	),
> > +	TP_fast_assign(
> > +		__entry->dev = mp->m_super->s_dev;
> > +		__entry->btnum = btnum;
> > +		__entry->bno = bno;
> > +		__entry->len = len;
> > +		__entry->diff = diff;
> > +		__entry->new = new;
> > +	),
> > +	TP_printk("dev %d:%d btnum %d bno 0x%x len 0x%x diff 0x%x new %d",
> > +		  MAJOR(__entry->dev), MINOR(__entry->dev), __entry->btnum,
> 
> Perhaps:
> 
> 	__print_symbolic(__entry->btnum, XFS_BTNUM_STRINGS),
> 
> instead of dumping the raw btnum value?
> 

Good point, fixed.

Brian

> Other than those two things, this looks like a pretty straightforward
> hoisting.
> 
> --D
> 
> > +		  __entry->bno, __entry->len, __entry->diff, __entry->new)
> > +)
> > +
> >  DECLARE_EVENT_CLASS(xfs_da_class,
> >  	TP_PROTO(struct xfs_da_args *args),
> >  	TP_ARGS(args),
> > -- 
> > 2.20.1
> >