Re: [PATCH] xfs: refactor dir2 leaf readahead shadow buffer cleverness

[Christoph Hellwig <hch@infradead.org>]

Hi Darrick,

a few comments below.  Most are cosmetic except for one which is
a minor improvement.

Reviewed-by: Christoph Hellwig <hch@lst.de>

with the cosmetic bits fixed up.

> +	last_da = xfs_dir2_byte_to_da(geo, XFS_DIR2_LEAF_OFFSET);
> +	map_off = xfs_dir2_db_to_da(geo, xfs_dir2_byte_to_db(geo, *cur_off));
> +	found = xfs_iext_lookup_extent(dp, ifp, map_off, &idx, &map);
> +	if (!found || map.br_startoff >= last_da)
>  		goto out;

I don't think we need the found variable in this function, all the users
only check for in the next line and then ignore it.  E.g. rewrite this
into

	if (!xfs_iext_lookup_extent(dp, ifp, map_off, &idx, &map))
		goto out;
	if (map.br_startoff >= last_da))
		goto out;

> +	ra_want = howmany(bufsize + geo->blksize, (1 << geo->fsblog));
> +	if (*ra_blk >= last_da)
> +		goto out;
> +	else if (*ra_blk == 0)
> +		*ra_blk = map.br_startoff;

No need for the else here.

> +	next_ra = map.br_startoff + geo->fsbcount;
> +	if (next_ra >= last_da)
> +		goto out_no_ra;
> +	found = xfs_iext_lookup_extent(dp, ifp, next_ra, &idx, &map);

Do we really need a new full lookup here?  This should be the same
or the next map compared to the one the original xfs_iext_lookup_extent
returned.  So just checking if it's in the original map that could
be stashed away or otherwise calling xfs_iext_get_extent would more
efficient.

> +	while (ra_want > 0) {
> +		next_ra = roundup((xfs_dablk_t)map.br_startoff, geo->fsbcount);
> +		while (ra_want > 0 &&
> +		       next_ra < map.br_startoff + map.br_blockcount) {
> +			if (next_ra >= last_da) {
> +				*ra_blk = last_da;
> +				break;
> +			} else if (next_ra > *ra_blk) {

No need for the else.