* [PATCH] xfs_mdrestore: initialize sb prior to xfs_sb_from_disk() @ 2014-06-05 23:09 Eric Sandeen 2014-06-05 23:56 ` Dave Chinner 2014-06-09 21:30 ` [PATCH V2] xfs: fix crc field handling in xfs_sb_to/from_disk Eric Sandeen 0 siblings, 2 replies; 7+ messages in thread From: Eric Sandeen @ 2014-06-05 23:09 UTC (permalink / raw) To: xfs-oss If we xfs_mdrestore an image from a non-crc filesystem, lo and behold the restored image has gained a CRC: # db/xfs_metadump.sh -o /dev/sdc1 - | xfs_mdrestore - test.img # xfs_db -c "sb 0" -c "p crc" /dev/sdc1 crc = 0 (correct) # xfs_db -c "sb 0" -c "p crc" test.img crc = 0xb6f8d6a0 (correct) Obviously it can't really be correct :) The problem is, xfs_sb_from_disk doesn't fill in the sb_crc field. An earlier commit: 47de6e1 repair: ensure that unused superblock fields are zeroed fixed this same sort of problem for xfs_repair. Do the same for mdrestore. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- But ... should we maybe just do this once and for all in xfs_sb_from_disk? I'm not sure leaving it up to every caller is a good idea, unless somebody ahs a reason to pre-populate some fields - I can't imagine why that would be, though... diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c index e57bdb2..0453f17 100644 --- a/mdrestore/xfs_mdrestore.c +++ b/mdrestore/xfs_mdrestore.c @@ -104,6 +104,7 @@ perform_restore( 1, src_f) != 1) fatal("error reading from file: %s\n", strerror(errno)); + memset(&sb, 0, sizeof(struct xfs_sb)); libxfs_sb_from_disk(&sb, (xfs_dsb_t *)block_buffer); if (sb.sb_magicnum != XFS_SB_MAGIC) _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs_mdrestore: initialize sb prior to xfs_sb_from_disk() 2014-06-05 23:09 [PATCH] xfs_mdrestore: initialize sb prior to xfs_sb_from_disk() Eric Sandeen @ 2014-06-05 23:56 ` Dave Chinner 2014-06-06 0:00 ` Eric Sandeen 2014-06-09 21:30 ` [PATCH V2] xfs: fix crc field handling in xfs_sb_to/from_disk Eric Sandeen 1 sibling, 1 reply; 7+ messages in thread From: Dave Chinner @ 2014-06-05 23:56 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfs-oss On Thu, Jun 05, 2014 at 06:09:16PM -0500, Eric Sandeen wrote: > If we xfs_mdrestore an image from a non-crc filesystem, lo > and behold the restored image has gained a CRC: > > # db/xfs_metadump.sh -o /dev/sdc1 - | xfs_mdrestore - test.img > # xfs_db -c "sb 0" -c "p crc" /dev/sdc1 > crc = 0 (correct) > # xfs_db -c "sb 0" -c "p crc" test.img > crc = 0xb6f8d6a0 (correct) > > Obviously it can't really be correct :) > > The problem is, xfs_sb_from_disk doesn't fill in the sb_crc > field. > > An earlier commit: > > 47de6e1 repair: ensure that unused superblock fields are zeroed > > fixed this same sort of problem for xfs_repair. Do the same > for mdrestore. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> Patch looks fine, but lets answer the question below before pushing this... > --- > > But ... should we maybe just do this once and for all in > xfs_sb_from_disk? I'm not sure leaving it up to every > caller is a good idea, unless somebody ahs a reason to > pre-populate some fields - I can't imagine why that would > be, though... We don't ever read in the CRC field into the in-memory structures because it has no meaning in memory. Simiarly, we don't ever write the CRC field from the in-core structure because we always re-calculate it in the IO path if CRCs are configured. That is consistent behaviour across the entire code-base. The superblock is a special case because of the way it is written. In kernel, we only write *specific* fields based on the field bitmask passed to xfs_mod_sb(), and we never set the CRC field bit in the kernel. Hence we never write that field to the superblock except when growing the filesystem and are initialising new secondary superblocks (where the in memory value is zero, anyway). Essentially, what userspace doing is the same: libxfs_sb_to_disk(buf, sbp, XFS_SB_ALL_BITS) which is telling the code to write the sb_crc field from the in-memory superblock buffer. So, either we need to zero the sbp->sb_crc field before it gets written, or we need to mask out the XFS_SB_CRC bit from the writable flags. IMO, the former is the correct thing to do we have to ensure that fields that are not read from disk appear in memory as zero. That way no matter how the superblock is written it will have the correct zero values for anything that was not specifically initialised.... Perhaps we should move the memset() to within xfs_sb_from_disk() to make this explicit? Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs_mdrestore: initialize sb prior to xfs_sb_from_disk() 2014-06-05 23:56 ` Dave Chinner @ 2014-06-06 0:00 ` Eric Sandeen 2014-06-06 1:42 ` Dave Chinner 0 siblings, 1 reply; 7+ messages in thread From: Eric Sandeen @ 2014-06-06 0:00 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs-oss On 6/5/14, 6:56 PM, Dave Chinner wrote: > On Thu, Jun 05, 2014 at 06:09:16PM -0500, Eric Sandeen wrote: ... >> But ... should we maybe just do this once and for all in >> xfs_sb_from_disk? I'm not sure leaving it up to every >> caller is a good idea, unless somebody ahs a reason to >> pre-populate some fields - I can't imagine why that would >> be, though... > > We don't ever read in the CRC field into the in-memory structures > because it has no meaning in memory. Simiarly, we don't ever write > the CRC field from the in-core structure because we always > re-calculate it in the IO path if CRCs are configured. That is > consistent behaviour across the entire code-base. <snip stuff> > Perhaps we should move the memset() to within xfs_sb_from_disk() > to make this explicit? Yes, that's what I meant by "this" in "do this once and for all" - sorry, that wasn't clear. memset(0) in xfs_sb_from_disk(). Yeah, the more I think about it, the more I think that's probably the obviously correct thing to do. -Eric > Cheers, > > Dave. > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs_mdrestore: initialize sb prior to xfs_sb_from_disk() 2014-06-06 0:00 ` Eric Sandeen @ 2014-06-06 1:42 ` Dave Chinner 2014-06-06 2:53 ` Eric Sandeen 0 siblings, 1 reply; 7+ messages in thread From: Dave Chinner @ 2014-06-06 1:42 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfs-oss On Thu, Jun 05, 2014 at 07:00:02PM -0500, Eric Sandeen wrote: > On 6/5/14, 6:56 PM, Dave Chinner wrote: > > On Thu, Jun 05, 2014 at 06:09:16PM -0500, Eric Sandeen wrote: > > ... > > >> But ... should we maybe just do this once and for all in > >> xfs_sb_from_disk? I'm not sure leaving it up to every > >> caller is a good idea, unless somebody ahs a reason to > >> pre-populate some fields - I can't imagine why that would > >> be, though... > > > > We don't ever read in the CRC field into the in-memory structures > > because it has no meaning in memory. Simiarly, we don't ever write > > the CRC field from the in-core structure because we always > > re-calculate it in the IO path if CRCs are configured. That is > > consistent behaviour across the entire code-base. > > <snip stuff> > > > Perhaps we should move the memset() to within xfs_sb_from_disk() > > to make this explicit? > > Yes, that's what I meant by "this" in "do this once and for all" - > sorry, that wasn't clear. memset(0) in xfs_sb_from_disk(). I didn't read it clearly. my fault. > Yeah, the more I think about it, the more I think that's probably > the obviously correct thing to do. *nod* Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs_mdrestore: initialize sb prior to xfs_sb_from_disk() 2014-06-06 1:42 ` Dave Chinner @ 2014-06-06 2:53 ` Eric Sandeen 2014-06-09 20:58 ` Eric Sandeen 0 siblings, 1 reply; 7+ messages in thread From: Eric Sandeen @ 2014-06-06 2:53 UTC (permalink / raw) To: Dave Chinner, Eric Sandeen; +Cc: xfs-oss On 6/5/14, 8:42 PM, Dave Chinner wrote: > On Thu, Jun 05, 2014 at 07:00:02PM -0500, Eric Sandeen wrote: >> On 6/5/14, 6:56 PM, Dave Chinner wrote: >>> On Thu, Jun 05, 2014 at 06:09:16PM -0500, Eric Sandeen wrote: >> >> ... >> >>>> But ... should we maybe just do this once and for all in >>>> xfs_sb_from_disk? I'm not sure leaving it up to every >>>> caller is a good idea, unless somebody ahs a reason to >>>> pre-populate some fields - I can't imagine why that would >>>> be, though... >>> >>> We don't ever read in the CRC field into the in-memory structures >>> because it has no meaning in memory. Simiarly, we don't ever write >>> the CRC field from the in-core structure because we always >>> re-calculate it in the IO path if CRCs are configured. That is >>> consistent behaviour across the entire code-base. >> >> <snip stuff> >> >>> Perhaps we should move the memset() to within xfs_sb_from_disk() >>> to make this explicit? >> >> Yes, that's what I meant by "this" in "do this once and for all" - >> sorry, that wasn't clear. memset(0) in xfs_sb_from_disk(). > > I didn't read it clearly. my fault. > >> Yeah, the more I think about it, the more I think that's probably >> the obviously correct thing to do. Actually, a memset() seems like overkill - every field except sb_crc is explicitly filled in in the function. Maybe better to just set sb_crc to 0, with a comment as to why? I think I'll whip that one up. -Eric > *nod* > > Cheers, > > Dave. > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs_mdrestore: initialize sb prior to xfs_sb_from_disk() 2014-06-06 2:53 ` Eric Sandeen @ 2014-06-09 20:58 ` Eric Sandeen 0 siblings, 0 replies; 7+ messages in thread From: Eric Sandeen @ 2014-06-09 20:58 UTC (permalink / raw) To: Dave Chinner, Eric Sandeen; +Cc: xfs-oss On 6/5/14, 9:53 PM, Eric Sandeen wrote: > On 6/5/14, 8:42 PM, Dave Chinner wrote: >> On Thu, Jun 05, 2014 at 07:00:02PM -0500, Eric Sandeen wrote: >>> On 6/5/14, 6:56 PM, Dave Chinner wrote: >>>> On Thu, Jun 05, 2014 at 06:09:16PM -0500, Eric Sandeen wrote: >>> >>> ... >>> >>>>> But ... should we maybe just do this once and for all in >>>>> xfs_sb_from_disk? I'm not sure leaving it up to every >>>>> caller is a good idea, unless somebody ahs a reason to >>>>> pre-populate some fields - I can't imagine why that would >>>>> be, though... >>>> >>>> We don't ever read in the CRC field into the in-memory structures >>>> because it has no meaning in memory. Simiarly, we don't ever write >>>> the CRC field from the in-core structure because we always >>>> re-calculate it in the IO path if CRCs are configured. That is >>>> consistent behaviour across the entire code-base. But as you say in userspace, this libxfs_sb_to_disk(buf, sbp, XFS_SB_ALL_BITS) *does* write it. kernelspace does the same here: 0 xfs_fsops.c xfs_growfs_data_private 520 xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb, XFS_SB_ALL_BITS); >>> <snip stuff> >>> >>>> Perhaps we should move the memset() to within xfs_sb_from_disk() >>>> to make this explicit? >>> >>> Yes, that's what I meant by "this" in "do this once and for all" - >>> sorry, that wasn't clear. memset(0) in xfs_sb_from_disk(). >> >> I didn't read it clearly. my fault. >> >>> Yeah, the more I think about it, the more I think that's probably >>> the obviously correct thing to do. > > Actually, a memset() seems like overkill - every field except > sb_crc is explicitly filled in in the function. > > Maybe better to just set sb_crc to 0, with a comment as to why? > I think I'll whip that one up. But now I realize we do sometimes write the in-memory value to disk, as seen above. Backing up - shouldn't we just go ahead and read/write it from/to disk just like every other field, at least in the cases where we are writing all, and when we are reading (which always reads all)? -Eric _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH V2] xfs: fix crc field handling in xfs_sb_to/from_disk 2014-06-05 23:09 [PATCH] xfs_mdrestore: initialize sb prior to xfs_sb_from_disk() Eric Sandeen 2014-06-05 23:56 ` Dave Chinner @ 2014-06-09 21:30 ` Eric Sandeen 1 sibling, 0 replies; 7+ messages in thread From: Eric Sandeen @ 2014-06-09 21:30 UTC (permalink / raw) To: Eric Sandeen, xfs-oss I discovered this in userspace, but the same change applies to the kernel. If we xfs_mdrestore an image from a non-crc filesystem, lo and behold the restored image has gained a CRC: # db/xfs_metadump.sh -o /dev/sdc1 - | xfs_mdrestore - test.img # xfs_db -c "sb 0" -c "p crc" /dev/sdc1 crc = 0 (correct) # xfs_db -c "sb 0" -c "p crc" test.img crc = 0xb6f8d6a0 (correct) This is because xfs_sb_from_disk doesn't fill in sb_crc, but xfs_sb_to_disk(XFS_SB_ALL_BITS) does write the in-memory CRC to disk - so we get uninitialized memory on disk. Fix this by always initializing sb_crc to 0 when we read the superblock, and masking out the CRC bit from ALL_BITS when we write it. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c index 8baf61a..d2ccb2e 100644 --- a/fs/xfs/xfs_sb.c +++ b/fs/xfs/xfs_sb.c @@ -450,6 +450,8 @@ xfs_sb_from_disk( to->sb_features_incompat = be32_to_cpu(from->sb_features_incompat); to->sb_features_log_incompat = be32_to_cpu(from->sb_features_log_incompat); + /* crc is only used on disk, not in memory; just init to 0 here. */ + to->sb_crc = 0; to->sb_pad = 0; to->sb_pquotino = be64_to_cpu(from->sb_pquotino); to->sb_lsn = be64_to_cpu(from->sb_lsn); @@ -527,6 +529,9 @@ xfs_sb_to_disk( if (!fields) return; + /* We should never write the crc here, it's updated in the IO path */ + fields &= ~XFS_SB_CRC; + xfs_sb_quota_to_disk(to, from, &fields); while (fields) { f = (xfs_sb_field_t)xfs_lowbit64((__uint64_t)fields); _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-06-09 21:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-05 23:09 [PATCH] xfs_mdrestore: initialize sb prior to xfs_sb_from_disk() Eric Sandeen 2014-06-05 23:56 ` Dave Chinner 2014-06-06 0:00 ` Eric Sandeen 2014-06-06 1:42 ` Dave Chinner 2014-06-06 2:53 ` Eric Sandeen 2014-06-09 20:58 ` Eric Sandeen 2014-06-09 21:30 ` [PATCH V2] xfs: fix crc field handling in xfs_sb_to/from_disk Eric Sandeen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.