Hi Darrick,
a few comments below. Most are cosmetic except for one which is
a minor improvement.
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
