* [PATCH] xfs_copy: fix up initial sb buffer read on CRC fs @ 2015-07-16 18:20 Eric Sandeen 2015-07-21 12:31 ` Brian Foster 0 siblings, 1 reply; 4+ messages in thread From: Eric Sandeen @ 2015-07-16 18:20 UTC (permalink / raw) To: xfs-oss 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 <sandeen@redhat.com> --- diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c index 44a32e8..fd96e15 100644 --- a/copy/xfs_copy.c +++ b/copy/xfs_copy.c @@ -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 xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs_copy: fix up initial sb buffer read on CRC fs 2015-07-16 18:20 [PATCH] xfs_copy: fix up initial sb buffer read on CRC fs Eric Sandeen @ 2015-07-21 12:31 ` Brian Foster 2015-07-21 15:11 ` Eric Sandeen 0 siblings, 1 reply; 4+ messages in thread From: Brian Foster @ 2015-07-21 12:31 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfs-oss On Thu, Jul 16, 2015 at 01:20:30PM -0500, Eric Sandeen wrote: > 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 <sandeen@redhat.com> > --- > > diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c > index 44a32e8..fd96e15 100644 > --- a/copy/xfs_copy.c > +++ b/copy/xfs_copy.c > @@ -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); 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 > + 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 > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs_copy: fix up initial sb buffer read on CRC fs 2015-07-21 12:31 ` Brian Foster @ 2015-07-21 15:11 ` Eric Sandeen 2015-07-21 15:45 ` Brian Foster 0 siblings, 1 reply; 4+ messages in thread From: Eric Sandeen @ 2015-07-21 15:11 UTC (permalink / raw) To: Brian Foster; +Cc: xfs-oss > On Jul 21, 2015, at 7:31 AM, Brian Foster <bfoster@redhat.com> wrote: > >> On Thu, Jul 16, 2015 at 01:20:30PM -0500, Eric Sandeen wrote: >> 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 <sandeen@redhat.com> >> --- >> >> diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c >> index 44a32e8..fd96e15 100644 >> --- a/copy/xfs_copy.c >> +++ b/copy/xfs_copy.c >> @@ -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); > > 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. :) > Without the purge, a re-read of a different size at the same offset seems to cause cache mismatch problems. Eric > Brian > >> + 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 >> xfs@oss.sgi.com >> http://oss.sgi.com/mailman/listinfo/xfs > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs_copy: fix up initial sb buffer read on CRC fs 2015-07-21 15:11 ` Eric Sandeen @ 2015-07-21 15:45 ` Brian Foster 0 siblings, 0 replies; 4+ messages in thread From: Brian Foster @ 2015-07-21 15:45 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfs-oss On Tue, Jul 21, 2015 at 10:11:05AM -0500, Eric Sandeen wrote: > > > > On Jul 21, 2015, at 7:31 AM, Brian Foster <bfoster@redhat.com> wrote: > > > >> On Thu, Jul 16, 2015 at 01:20:30PM -0500, Eric Sandeen wrote: > >> 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 <sandeen@redhat.com> > >> --- > >> > >> diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c > >> index 44a32e8..fd96e15 100644 > >> --- a/copy/xfs_copy.c > >> +++ b/copy/xfs_copy.c > >> @@ -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); > > > > 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. :) > > > 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 <bfoster@redhat.com> > Eric > > > Brian > > > >> + 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 > >> xfs@oss.sgi.com > >> http://oss.sgi.com/mailman/listinfo/xfs > > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-07-21 15:45 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-07-16 18:20 [PATCH] xfs_copy: fix up initial sb buffer read on CRC fs Eric Sandeen 2015-07-21 12:31 ` Brian Foster 2015-07-21 15:11 ` Eric Sandeen 2015-07-21 15:45 ` Brian Foster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox