[PATCH 0/1] xfsprogs: random fixes

[PATCH 0/1] xfsprogs: random fixes

[Darrick J. Wong]

Hi all,

Random fixes.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes

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

[Darrick J. Wong]

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

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

[Allison Collins]

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