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@xxxxxxxxxxxxx
