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

Dave Chinner
2019-08-29T08:45:35Z

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