Re: [PATCH 1/1] xfs_repair: fix totally broken unit conversion in directory invalidation

[Allison Collins <allison.henderson@oracle.com>]

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);
>