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

[Christoph Hellwig <hch@infradead.org>]

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.

>  			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.

> +		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?

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