diff mbox series

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

Message ID 157982504329.2765410.10475555316974599797.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series xfsprogs: random fixes | expand

Commit Message

Darrick J. Wong Jan. 24, 2020, 12:17 a.m. UTC 
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(-)

Comments

Christoph Hellwig Jan. 25, 2020, 11:19 p.m. UTC | #1 
On Thu, Jan 23, 2020 at 04:17:23PM -0800, Darrick J. Wong wrote:
> 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.

No more magic macro in here (thankfully).  But the rest looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Jan. 26, 2020, 10:11 p.m. UTC | #2 
On Sat, Jan 25, 2020 at 03:19:27PM -0800, Christoph Hellwig wrote:
> On Thu, Jan 23, 2020 at 04:17:23PM -0800, Darrick J. Wong wrote:
> > 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.
> 
> No more magic macro in here (thankfully).  But the rest looks good:

Oops, yeah... Eric, if you decide to take this patch, could you please
drop that second paragraph about the macro?

> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thank you for the reviews!

--D
Eric Sandeen Jan. 30, 2020, 6:35 p.m. UTC | #3 
On 1/26/20 4:11 PM, Darrick J. Wong wrote:
> On Sat, Jan 25, 2020 at 03:19:27PM -0800, Christoph Hellwig wrote:
>> On Thu, Jan 23, 2020 at 04:17:23PM -0800, Darrick J. Wong wrote:
>>> 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.
>>
>> No more magic macro in here (thankfully).  But the rest looks good:
> 
> Oops, yeah... Eric, if you decide to take this patch, could you please
> drop that second paragraph about the macro?

Done

>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Thank you for the reviews!
> 
> --D
>
diff mbox series

Patch

diff --git a/repair/phase6.c b/repair/phase6.c
index 76709f73..0874b649 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1273,7 +1273,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 &&
@@ -1283,11 +1283,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, &bp,
 					whichfork);