| Message ID | 157784163634.1371001.9270275500137619667.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfsprogs: random fixes | expand |
On 12/31/19 6:20 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > 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. > > Fix all this stupidity by adding a for loop macro to take care of these > details for us so that everyone can iterate all logical directory blocks > (xfs_dir2_db_t) that start within a given bmbt record. > > The pre-5.5 xfs_da_get_buf implementation mostly hides the off-by-one > error because dir_binval turns on "don't complain if no mapping" mode, > but on dirblocksize > fsblocksize filesystems the incorrect units can > cause us to miss invalidating some blocks, which can lead to other > buffer cache errors later. > > Fixes: f9c559f4e4fb4 ("xfs_repair: invalidate dirty dir buffers when we zap a directory") > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> This one looks ok to me. Thanks! Reviewed-by: Allison Collins <allison.henderson@oracle.com> > --- > repair/phase6.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > > diff --git a/repair/phase6.c b/repair/phase6.c > index 91d208a6..a11712a2 100644 > --- a/repair/phase6.c > +++ b/repair/phase6.c > @@ -1276,7 +1276,7 @@ dir_binval( > struct xfs_ifork *ifp; > struct xfs_da_geometry *geo; > struct xfs_buf *bp; > - xfs_dablk_t dabno, end_dabno; > + xfs_dablk_t dabno; > int error = 0; > > if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS && > @@ -1286,11 +1286,9 @@ 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 (dabno = roundup(rec.br_startoff, geo->fsbcount); > + dabno < rec.br_startoff + rec.br_blockcount; > + dabno += geo->fsbcount) { > bp = NULL; > error = -libxfs_da_get_buf(tp, ip, dabno, -2, &bp, > whichfork); >
diff --git a/repair/phase6.c b/repair/phase6.c index 91d208a6..a11712a2 100644 --- a/repair/phase6.c +++ b/repair/phase6.c @@ -1276,7 +1276,7 @@ dir_binval( struct xfs_ifork *ifp; struct xfs_da_geometry *geo; struct xfs_buf *bp; - xfs_dablk_t dabno, end_dabno; + xfs_dablk_t dabno; int error = 0; if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS && @@ -1286,11 +1286,9 @@ 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 (dabno = roundup(rec.br_startoff, geo->fsbcount); + dabno < rec.br_startoff + rec.br_blockcount; + dabno += geo->fsbcount) { bp = NULL; error = -libxfs_da_get_buf(tp, ip, dabno, -2, &bp, whichfork);