Re: [PATCH 4/5] xfs: speed up directory bestfree block scanning

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 29, 2019 at 01:18:22AM -0700, Christoph Hellwig wrote:
> On Thu, Aug 29, 2019 at 04:30:41PM +1000, Dave Chinner wrote:
> > +		bests = dp->d_ops->free_bests_p(free);
> > +		dp->d_ops->free_hdr_from_disk(&freehdr, free);
> >  		if (findex >= 0) {
> >  			/* caller already found the freespace for us. */
> > -			bests = dp->d_ops->free_bests_p(free);
> > -			dp->d_ops->free_hdr_from_disk(&freehdr, free);
> > -
> 
> I don't see any way how this is needed or helpful with this patch,
> we are just going to ovewrite bests and freehdr before even looking
> at them if the branch is not taken.

*nod*

The change is not useful anymore as a result of folding in your
previous suggestions. I'll revert it.

> >  			ASSERT(findex < freehdr.nvalid);
> >  			ASSERT(be16_to_cpu(bests[findex]) != NULLDATAOFF);
> >  			ASSERT(be16_to_cpu(bests[findex]) >= length);
> >  			dbno = freehdr.firstdb + findex;
> > -			goto out;
> > +			goto found_block;
> 
> The label rename while more descriptive also seems entirely unrelated.

That was one of your previous suggestions :)

I'll push it back up one patch into the cleanup patch and leave this
as an optimisation only patch.

> > +		findex = 0;
> > +		free = fbp->b_addr;
> >  		bests = dp->d_ops->free_bests_p(free);
> >  		dp->d_ops->free_hdr_from_disk(&freehdr, free);
> > +
> > +		/* Scan the free entry array for a large enough free space. */
> > +		do {
> > +			if (be16_to_cpu(bests[findex]) != NULLDATAOFF &&
> > +			    be16_to_cpu(bests[findex]) >= length) {
> > +				dbno = freehdr.firstdb + findex;
> > +				goto found_block;
> >  			}
> > +		} while (++findex < freehdr.nvalid);
> 
> Nit: wou;dn't this be better written as a for loop also taking the
> initialization of findex into the loop?

Agreed - the next patch does that with the reversal of the search
order. The end result is what you're asking for, so I'll leave this
alone for now....

> Otherwise this looks good.  I always like it when a speedup removes
> code..

I hadn't noticed that - I was more concerned with ending up with
readable code :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com