On Tue, Dec 24, 2019 at 12:38:43AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 17, 2019 at 08:24:02PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Your humble author forgot that xfs_dablk_t has the same units as
> > xfs_fileoff_t, and totally screwed up the directory buffer invalidation
> > loop in dir_binval. Not only is there an off-by-one error in the loop
> > conditional, but the unit conversions are wrong.
>
> Can we kill off xfs_dablk_t? I found the concept very, very confusing
> when touching the dir code.
I personally wouldn't mind if that happened (as a separate patch).
> > --- a/libxfs/xfs_dir2.h
> > +++ b/libxfs/xfs_dir2.h
> > @@ -308,6 +308,16 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
> > sizeof(struct xfs_dir2_leaf_tail));
> > }
> >
> > +/*
> > + * For a given dir/attr geometry and extent mapping record, walk every file
> > + * offset block (xfs_dablk_t) in the mapping that corresponds to the start
> > + * of a logical directory block (xfs_dir2_db_t).
> > + */
> > +#define for_each_xfs_bmap_dabno(geo, irec, dabno) \
> > + for ((dabno) = round_up((irec)->br_startoff, (geo)->fsbcount); \
> > + (dabno) < (irec)->br_startoff + (irec)->br_blockcount; \
> > + (dabno) += (geo)->fsbcount)
>
> I think not having the magic for macro would be cleaner..
Eh, yeah, we don't really need it...
--D
>
> > + xfs_dablk_t dabno;
> > int error = 0;
> >
> > if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS &&
> > @@ -1286,11 +1286,7 @@ dir_binval(
> > geo = tp->t_mountp->m_dir_geo;
> > ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> > for_each_xfs_iext(ifp, &icur, &rec) {
> > - dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
> > - geo->fsbcount - 1);
> > - end_dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
> > - rec.br_blockcount);
> > - for (; dabno <= end_dabno; dabno += geo->fsbcount) {
> > + for_each_xfs_bmap_dabno(geo, &rec, dabno) {
> > bp = NULL;
> > error = -libxfs_da_get_buf(tp, ip, dabno, -2, &bp,
> > whichfork);
>
> But either way, the fix looks good.
