xfs_copy: simplify first_agbno calculation

PATCH In review Eric Sandeen 3 participants 4 messages Nov 10, 2014 – Nov 11, 2014

Share & tools

Eric Sandeen

author

2014-11-10 22:15 UTC #0 lore raw

PATCH 1/1 xfs_copy: simplify first_agbno calculation

After ffe9a9a xfsprogs: xfs_copy: fix data corruption of target, xfs_copy started hitting an ASSERT for a 4k sector / 4k blocksize filesystem:

# dd if=/dev/zero of=test.img bs=1M count=1024
# mkfs.xfs -s size=4096 test.img
# xfs_copy test.img xfs.img
xfs_copy: xfs_copy.c:720: main: Assertion `((((((xfs_daddr_t)(3 << (mp)->m_sectbb_log)) + 1) * (1<<9)) + first_residue) % source_blocksize) == 0' failed.
Aborted

I started digging through all the calculations below, and realized that in the end, all it wants is the first filesystem block after the AG header. XFS_AGFL_BLOCK(mp) + 1 suffices for this purpose; rip out the rest which seems overly complex and apparently bug-prone.

I tested this by creating a 4g filesystem with combinations of sector & block size between 512 and 4k, copying in /lib/modules, running an xfs_copy of that, and running repair against the copy; it all looks good. It took a long time, but I will create a simpler/shorter xfstest based on this.

Reported-by Zorro Lang <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

copy/xfs_copy.c +3 ~~-17~~

index 7ce5ec9..279527c 100644
@@ -475,7 +475,7 @@ main(int argc, char **argv)
 	int		open_flags;
 	xfs_off_t	pos, end_pos;
 	size_t		length;
-	int		c, first_residue, tmp_residue;
+	int		c;
 	__uint64_t	size, sizeb;
 	__uint64_t	numblocks = 0;
 	int		wblocks = 0;

@@ -697,27 +697,13 @@ main(int argc, char **argv)
 	ASSERT(source_blocksize % source_sectorsize == 0);
 	ASSERT(source_sectorsize % BBSIZE == 0);
 
-	if (source_blocksize > source_sectorsize)  {
-		/* get number of leftover sectors in last block of ag header */
-
-		tmp_residue = ((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
-					% source_blocksize;
-		first_residue = (tmp_residue == 0) ? 0 :
-			source_blocksize - tmp_residue;
-		ASSERT(first_residue % source_sectorsize == 0);
-	} else if (source_blocksize == source_sectorsize)  {
-		first_residue = 0;
-	} else  {
+	if (source_blocksize < source_sectorsize)  {
 		do_log(_("Error:  filesystem block size is smaller than the"
 			" disk sectorsize.\nAborting XFS copy now.\n"));
 		exit(1);
 	}
 
-	first_agbno = (((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
-				+ first_residue) / source_blocksize;
-	ASSERT(first_agbno != 0);
-	ASSERT(((((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
-				+ first_residue) % source_blocksize) == 0);
+	first_agbno = XFS_AGFL_BLOCK(mp) + 1;
 
 	/* now open targets */

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2014-11-11 00:37 UTC #1 lore raw

Re: [PATCH] xfs_copy: fix ASSERT failure on 4k devices...

Hm, realized that perhaps a more forceful $SUBJECT would have been better :)

-Eric

Quoted 69 lines · Eric Sandeen

After ffe9a9a xfsprogs: xfs_copy: fix data corruption of target, xfs_copy started hitting an ASSERT for a 4k sector / 4k blocksize filesystem:
# dd if=/dev/zero of=test.img bs=1M count=1024
# mkfs.xfs -s size=4096 test.img
# xfs_copy test.img xfs.img
xfs_copy: xfs_copy.c:720: main: Assertion `((((((xfs_daddr_t)(3 << (mp)->m_sectbb_log)) + 1) * (1<<9)) + first_residue) % source_blocksize) == 0' failed.
Aborted
I started digging through all the calculations below, and realized that in the end, all it wants is the first filesystem block after the AG header. XFS_AGFL_BLOCK(mp) + 1 suffices for this purpose; rip out the rest which seems overly complex and apparently bug-prone.
I tested this by creating a 4g filesystem with combinations of sector & block size between 512 and 4k, copying in /lib/modules, running an xfs_copy of that, and running repair against the copy; it all looks good. It took a long time, but I will create a simpler/shorter xfstest based on this.
Reported-by Zorro Lang <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
---
copy/xfs_copy.c +3 ~~-17~~
index 7ce5ec9..279527c 100644
@@ -475,7 +475,7 @@ main(int argc, char **argv)
 	int		open_flags;
 	xfs_off_t	pos, end_pos;
 	size_t		length;
-	int		c, first_residue, tmp_residue;
+	int		c;
 	__uint64_t	size, sizeb;
 	__uint64_t	numblocks = 0;
 	int		wblocks = 0;
@@ -697,27 +697,13 @@ main(int argc, char **argv)
 	ASSERT(source_blocksize % source_sectorsize == 0);
 	ASSERT(source_sectorsize % BBSIZE == 0);
 
-	if (source_blocksize > source_sectorsize)  {
-		/* get number of leftover sectors in last block of ag header */
-
-		tmp_residue = ((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
-					% source_blocksize;
-		first_residue = (tmp_residue == 0) ? 0 :
-			source_blocksize - tmp_residue;
-		ASSERT(first_residue % source_sectorsize == 0);
-	} else if (source_blocksize == source_sectorsize)  {
-		first_residue = 0;
-	} else  {
+	if (source_blocksize < source_sectorsize)  {
 		do_log(_("Error:  filesystem block size is smaller than the"
 			" disk sectorsize.\nAborting XFS copy now.\n"));
 		exit(1);
 	}
 
-	first_agbno = (((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
-				+ first_residue) / source_blocksize;
-	ASSERT(first_agbno != 0);
-	ASSERT(((((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
-				+ first_residue) % source_blocksize) == 0);
+	first_agbno = XFS_AGFL_BLOCK(mp) + 1;
 
 	/* now open targets */
 

Quoted 5 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Brian Foster

Reviewed-by on v1 1/1

2014-11-11 13:37 UTC #2 lore raw

In reply to Eric Sandeen

Quoted 21 lines · Eric Sandeen

After ffe9a9a xfsprogs: xfs_copy: fix data corruption of target, xfs_copy started hitting an ASSERT for a 4k sector / 4k blocksize filesystem:
# dd if=/dev/zero of=test.img bs=1M count=1024
# mkfs.xfs -s size=4096 test.img
# xfs_copy test.img xfs.img
xfs_copy: xfs_copy.c:720: main: Assertion `((((((xfs_daddr_t)(3 << (mp)->m_sectbb_log)) + 1) * (1<<9)) + first_residue) % source_blocksize) == 0' failed.
Aborted
I started digging through all the calculations below, and realized that in the end, all it wants is the first filesystem block after the AG header. XFS_AGFL_BLOCK(mp) + 1 suffices for this purpose; rip out the rest which seems overly complex and apparently bug-prone.
I tested this by creating a 4g filesystem with combinations of sector & block size between 512 and 4k, copying in /lib/modules, running an xfs_copy of that, and running repair against the copy; it all looks good. It took a long time, but I will create a simpler/shorter xfstest based on this.

Quoted 3 lines

Reported-by Zorro Lang <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
---

Looks Ok to me:

Reviewed-by Brian Foster <[email protected]>

I just noticed the bug... so the problem was basically the assumption that sector size == BBSIZE?

Brian

Quoted 45 lines

copy/xfs_copy.c +3 ~~-17~~

index 7ce5ec9..279527c 100644
@@ -475,7 +475,7 @@ main(int argc, char **argv)
 	int		open_flags;
 	xfs_off_t	pos, end_pos;
 	size_t		length;
-	int		c, first_residue, tmp_residue;
+	int		c;
 	__uint64_t	size, sizeb;
 	__uint64_t	numblocks = 0;
 	int		wblocks = 0;

@@ -697,27 +697,13 @@ main(int argc, char **argv)
 	ASSERT(source_blocksize % source_sectorsize == 0);
 	ASSERT(source_sectorsize % BBSIZE == 0);
 
-	if (source_blocksize > source_sectorsize)  {
-		/* get number of leftover sectors in last block of ag header */
-
-		tmp_residue = ((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
-					% source_blocksize;
-		first_residue = (tmp_residue == 0) ? 0 :
-			source_blocksize - tmp_residue;
-		ASSERT(first_residue % source_sectorsize == 0);
-	} else if (source_blocksize == source_sectorsize)  {
-		first_residue = 0;
-	} else  {
+	if (source_blocksize < source_sectorsize)  {
 		do_log(_("Error:  filesystem block size is smaller than the"
 			" disk sectorsize.\nAborting XFS copy now.\n"));
 		exit(1);
 	}
 
-	first_agbno = (((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
-				+ first_residue) / source_blocksize;
-	ASSERT(first_agbno != 0);
-	ASSERT(((((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
-				+ first_residue) % source_blocksize) == 0);
+	first_agbno = XFS_AGFL_BLOCK(mp) + 1;
 
 	/* now open targets */

Quoted 4 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2014-11-11 16:02 UTC #3 lore raw

Quoted 26 lines · Brian Foster

Quoted 21 lines · Eric Sandeen
After ffe9a9a xfsprogs: xfs_copy: fix data corruption of target, xfs_copy started hitting an ASSERT for a 4k sector / 4k blocksize filesystem:
# dd if=/dev/zero of=test.img bs=1M count=1024
# mkfs.xfs -s size=4096 test.img
# xfs_copy test.img xfs.img
xfs_copy: xfs_copy.c:720: main: Assertion `((((((xfs_daddr_t)(3 << (mp)->m_sectbb_log)) + 1) * (1<<9)) + first_residue) % source_blocksize) == 0' failed.
Aborted
I started digging through all the calculations below, and realized that in the end, all it wants is the first filesystem block after the AG header. XFS_AGFL_BLOCK(mp) + 1 suffices for this purpose; rip out the rest which seems overly complex and apparently bug-prone.
I tested this by creating a 4g filesystem with combinations of sector & block size between 512 and 4k, copying in /lib/modules, running an xfs_copy of that, and running repair against the copy; it all looks good. It took a long time, but I will create a simpler/shorter xfstest based on this.
Quoted 3 lines
Reported-by Zorro Lang <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
---

Quoted 6 lines

Looks Ok to me:
Reviewed-by Brian Foster <[email protected]>
I just noticed the bug... so the problem was basically the assumption that sector size == BBSIZE?

To be honest, I never quite figured out what the root cause bug was; bad me. The previous commit was supposed to *fix* the assumption that sector size == BBSIZE. But I got lost in all the gyrations.

I guess maybe I should work that out, but I'm not sure it's worth it. ;)

-Eric

Quoted 47 lines

Brian

Quoted 45 lines

copy/xfs_copy.c +3 ~~-17~~

index 7ce5ec9..279527c 100644
@@ -475,7 +475,7 @@ main(int argc, char **argv)
 	int		open_flags;
 	xfs_off_t	pos, end_pos;
 	size_t		length;
-	int		c, first_residue, tmp_residue;
+	int		c;
 	__uint64_t	size, sizeb;
 	__uint64_t	numblocks = 0;
 	int		wblocks = 0;

@@ -697,27 +697,13 @@ main(int argc, char **argv)
 	ASSERT(source_blocksize % source_sectorsize == 0);
 	ASSERT(source_sectorsize % BBSIZE == 0);
 
-	if (source_blocksize > source_sectorsize)  {
-		/* get number of leftover sectors in last block of ag header */
-
-		tmp_residue = ((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
-					% source_blocksize;
-		first_residue = (tmp_residue == 0) ? 0 :
-			source_blocksize - tmp_residue;
-		ASSERT(first_residue % source_sectorsize == 0);
-	} else if (source_blocksize == source_sectorsize)  {
-		first_residue = 0;
-	} else  {
+	if (source_blocksize < source_sectorsize)  {
 		do_log(_("Error:  filesystem block size is smaller than the"
 			" disk sectorsize.\nAborting XFS copy now.\n"));
 		exit(1);
 	}
 
-	first_agbno = (((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
-				+ first_residue) / source_blocksize;
-	ASSERT(first_agbno != 0);
-	ASSERT(((((XFS_AGFL_DADDR(mp) + 1) * BBSIZE)
-				+ first_residue) % source_blocksize) == 0);
+	first_agbno = XFS_AGFL_BLOCK(mp) + 1;
 
 	/* now open targets */

Quoted 10 lines

Quoted 4 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs