xfs_copy: fix up initial sb buffer read on CRC fs

PATCH In review Eric Sandeen 2 participants 4 messages Jul 16, 2015 – Jul 21, 2015

Share & tools

Eric Sandeen

author

2015-07-16 18:20 UTC #0 lore raw

PATCH 1/1 xfs_copy: fix up initial sb buffer read on CRC fs

My prior commit, aaf90a2 xfs_copy: fix copy of hard 4k devices causes xfs_copy to emit a CRC error warning when copying a CRC filesystem.

This is because we are now reading the maximum sector size, and attempting to verify the CRC based on that (likely incorrect) length.

In xfs_db, we currently just don't verify this read, so it's not a problem. In xfs_copy, we almost certainly want to verify.

So, first do the maximal read with no verifier; once it's read, drop that buffer, and re-read with the proper sector size and verifier.

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

---

copy/xfs_copy.c +8 -2

index 44a32e8..fd96e15 100644
@@ -654,11 +654,17 @@ main(int argc, char **argv)
 
 	memset(&mbuf, 0, sizeof(xfs_mount_t));
 	libxfs_buftarg_init(&mbuf, xargs.ddev, xargs.logdev, xargs.rtdev);
+	/* We don't yet know the sector size, so read maximal size */
 	sbp = libxfs_readbuf(mbuf.m_ddev_targp, XFS_SB_DADDR,
-			     1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT),
-			     0, &xfs_sb_buf_ops);
+			     1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
 	sb = &mbuf.m_sb;
 	libxfs_sb_from_disk(sb, XFS_BUF_TO_SBP(sbp));
+	/* Do it again, now with proper length and verifier */
+	libxfs_putbuf(sbp);
+	libxfs_purgebuf(sbp);
+	sbp = libxfs_readbuf(mbuf.m_ddev_targp, XFS_SB_DADDR,
+			     1 << (sb->sb_sectlog - BBSHIFT),
+			     0, &xfs_sb_buf_ops);
 
 	/*
 	 * For now, V5 superblock filesystems are not supported without -d;

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

Brian Foster

2015-07-21 12:32 UTC #1 lore raw

Quoted 26 lines · Eric Sandeen

My prior commit, aaf90a2 xfs_copy: fix copy of hard 4k devices causes xfs_copy to emit a CRC error warning when copying a CRC filesystem.
This is because we are now reading the maximum sector size, and attempting to verify the CRC based on that (likely incorrect) length.
In xfs_db, we currently just don't verify this read, so it's not a problem. In xfs_copy, we almost certainly want to verify.
So, first do the maximal read with no verifier; once it's read, drop that buffer, and re-read with the proper sector size and verifier.
Signed-off-by Eric Sandeen <[email protected]>
---
copy/xfs_copy.c +0 -0
index 44a32e8..fd96e15 100644
@@ -654,11 +654,17 @@ main(int argc, char **argv)
 
 	memset(&mbuf, 0, sizeof(xfs_mount_t));
 	libxfs_buftarg_init(&mbuf, xargs.ddev, xargs.logdev, xargs.rtdev);

copy/xfs_copy.c main(int argc, char **argv) · line 664

Quoted 10 lines

copy/xfs_copy.c +5 -2

+	/* We don't yet know the sector size, so read maximal size */
 	sbp = libxfs_readbuf(mbuf.m_ddev_targp, XFS_SB_DADDR,
-			     1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT),
-			     0, &xfs_sb_buf_ops);
+			     1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
 	sb = &mbuf.m_sb;
 	libxfs_sb_from_disk(sb, XFS_BUF_TO_SBP(sbp));
+	/* Do it again, now with proper length and verifier */
+	libxfs_putbuf(sbp);
+	libxfs_purgebuf(sbp);

Why the purge? On a quick look, it looks like the buffer cache code would handle this if the buffer size changes.

Hmm, is it to ensure the verification occurs if the buffer size doesn't actually change? If so, I'd suggest to enhance the comment. :)

Brian

Quoted 11 lines

copy/xfs_copy.c +3 -0
+	sbp = libxfs_readbuf(mbuf.m_ddev_targp, XFS_SB_DADDR,
+			     1 << (sb->sb_sectlog - BBSHIFT),
+			     0, &xfs_sb_buf_ops);
 
 	/*
 	 * For now, V5 superblock filesystems are not supported without -d;
_______________________________________________
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

author

2015-07-21 15:11 UTC #2 lore raw

Quoted 45 lines · Brian Foster

Quoted 28 lines · Eric Sandeen
My prior commit, aaf90a2 xfs_copy: fix copy of hard 4k devices causes xfs_copy to emit a CRC error warning when copying a CRC filesystem.
This is because we are now reading the maximum sector size, and attempting to verify the CRC based on that (likely incorrect) length.
In xfs_db, we currently just don't verify this read, so it's not a problem. In xfs_copy, we almost certainly want to verify.
So, first do the maximal read with no verifier; once it's read, drop that buffer, and re-read with the proper sector size and verifier.
Signed-off-by Eric Sandeen <[email protected]>
---
copy/xfs_copy.c +0 -0
index 44a32e8..fd96e15 100644
@@ -654,11 +654,17 @@ main(int argc, char **argv)
 
   memset(&mbuf, 0, sizeof(xfs_mount_t));
   libxfs_buftarg_init(&mbuf, xargs.ddev, xargs.logdev, xargs.rtdev);
copy/xfs_copy.c main(int argc, char **argv) · line 664
Quoted 10 lines
copy/xfs_copy.c +5 -2
+    /* We don't yet know the sector size, so read maximal size */
   sbp = libxfs_readbuf(mbuf.m_ddev_targp, XFS_SB_DADDR,
-                 1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT),
-                 0, &xfs_sb_buf_ops);
+                 1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
   sb = &mbuf.m_sb;
   libxfs_sb_from_disk(sb, XFS_BUF_TO_SBP(sbp));
+    /* Do it again, now with proper length and verifier */
+    libxfs_putbuf(sbp);
+    libxfs_purgebuf(sbp);
Why the purge? On a quick look, it looks like the buffer cache code would handle this if the buffer size changes.

Quoted 3 lines

Hmm, is it to ensure the verification occurs if the buffer size doesn't actually change? If so, I'd suggest to enhance the comment. :)

Without the purge, a re-read of a different size at the same offset seems to cause cache mismatch problems.

Eric

Quoted 14 lines

Brian
Quoted 11 lines
copy/xfs_copy.c +3 -0
+    sbp = libxfs_readbuf(mbuf.m_ddev_targp, XFS_SB_DADDR,
+                 1 << (sb->sb_sectlog - BBSHIFT),
+                 0, &xfs_sb_buf_ops);
 
   /*
    * For now, V5 superblock filesystems are not supported without -d;
_______________________________________________
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

2015-07-21 15:45 UTC #3 lore raw

Quoted 45 lines · Eric Sandeen

Quoted 45 lines · Brian Foster
Quoted 28 lines · Eric Sandeen
My prior commit, aaf90a2 xfs_copy: fix copy of hard 4k devices causes xfs_copy to emit a CRC error warning when copying a CRC filesystem.
This is because we are now reading the maximum sector size, and attempting to verify the CRC based on that (likely incorrect) length.
In xfs_db, we currently just don't verify this read, so it's not a problem. In xfs_copy, we almost certainly want to verify.
So, first do the maximal read with no verifier; once it's read, drop that buffer, and re-read with the proper sector size and verifier.
Signed-off-by Eric Sandeen <[email protected]>
---
copy/xfs_copy.c +0 -0
index 44a32e8..fd96e15 100644
@@ -654,11 +654,17 @@ main(int argc, char **argv)
 
   memset(&mbuf, 0, sizeof(xfs_mount_t));
   libxfs_buftarg_init(&mbuf, xargs.ddev, xargs.logdev, xargs.rtdev);
copy/xfs_copy.c main(int argc, char **argv) · line 664
Quoted 10 lines
copy/xfs_copy.c +5 -2
+    /* We don't yet know the sector size, so read maximal size */
   sbp = libxfs_readbuf(mbuf.m_ddev_targp, XFS_SB_DADDR,
-                 1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT),
-                 0, &xfs_sb_buf_ops);
+                 1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
   sb = &mbuf.m_sb;
   libxfs_sb_from_disk(sb, XFS_BUF_TO_SBP(sbp));
+    /* Do it again, now with proper length and verifier */
+    libxfs_putbuf(sbp);
+    libxfs_purgebuf(sbp);
Why the purge? On a quick look, it looks like the buffer cache code would handle this if the buffer size changes.

Quoted 5 lines

Quoted 3 lines
Hmm, is it to ensure the verification occurs if the buffer size doesn't actually change? If so, I'd suggest to enhance the comment. :)
Without the purge, a re-read of a different size at the same offset seems to cause cache mismatch problems.

Ok, fair enough. Care to update the comment? Otherwise this seems good to me:

Reviewed-by Brian Foster <[email protected]>

Quoted 16 lines

Eric
Quoted 14 lines
Brian
Quoted 11 lines
copy/xfs_copy.c +3 -0
+    sbp = libxfs_readbuf(mbuf.m_ddev_targp, XFS_SB_DADDR,
+                 1 << (sb->sb_sectlog - BBSHIFT),
+                 0, &xfs_sb_buf_ops);
 
   /*
    * For now, V5 superblock filesystems are not supported without -d;
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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