On Tue, May 02, 2017 at 12:44:00AM -0700, Christoph Hellwig wrote:
> 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;
Ok.
> > + 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.
Ok.
> > + 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.
Sure, that could be something like:
next_ra = map.br_startoff + geo->fsbcount;
if (next_ra >= last_da)
goto out_no_ra;
if (map.br_blockcount < geo->fsbcount &&
!xfs_iext_get_extent(ifp, ++idx, &map))
goto out_no_ra;
if (map.br_startoff >= last_da)
goto out_no_ra;
xfs_trim_extent(&map, next_ra, last_da - next_ra);
> > + 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.
Ok.
--D
>
> --
> 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
--
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
