xfs_mdrestore: initialize sb prior to xfs_sb_from_disk()
PATCH 1/1 xfs_mdrestore: initialize sb prior to xfs_sb_from_disk()
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.
---
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...
mdrestore/xfs_mdrestore.c +1 -0
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 14 lines · Eric Sandeen
If we
xfs_mdrestorean 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_diskdoesn't fill in thesb_crcfield.
Quoted 8 lines
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 <[email protected]>
Patch looks fine, but lets answer the question below before pushing this...
Quoted 7 lines
---
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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 1 line · Dave Chinner
On Thu, Jun 05, 2014 at 06:09:16PM -0500, Eric Sandeen wrote:
...
Quoted 11 lines
Quoted 5 lines
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>
Quoted 2 lines
Perhaps we should move the
memset()to withinxfs_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
Quoted 4 lines
Cheers,
Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 17 lines · Eric Sandeen
Quoted 1 line · Dave Chinner
On Thu, Jun 05, 2014 at 06:09:16PM -0500, Eric Sandeen wrote:
...
Quoted 11 lines
Quoted 5 lines
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.
Quoted 7 lines
<snip stuff>
Quoted 2 lines
Perhaps we should move the
memset()to withinxfs_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.
Quoted 2 lines
Yeah, the more I think about it, the more I think that's probably the obviously correct thing to do.
*nod*
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 20 lines · Dave Chinner
Quoted 19 lines · Eric Sandeen
Quoted 1 line · Dave Chinner
On Thu, Jun 05, 2014 at 06:09:16PM -0500, Eric Sandeen wrote:
...
Quoted 11 lines
Quoted 5 lines
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>
Quoted 10 lines
Quoted 5 lines
Quoted 2 lines
Perhaps we should move the
memset()to withinxfs_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.
Quoted 2 lines
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
Quoted 6 lines
*nod*
Cheers,
Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 13 lines · Eric Sandeen
Quoted 12 lines · Dave Chinner
Quoted 11 lines · Eric Sandeen
Quoted 1 line · Dave Chinner
On Thu, Jun 05, 2014 at 06:09:16PM -0500, Eric Sandeen wrote:
...
Quoted 6 lines
Quoted 5 lines
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...
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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);
Quoted 10 lines
Quoted 10 lines
Quoted 7 lines
<snip stuff>
Quoted 2 lines
Perhaps we should move the
memset()to withinxfs_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.
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
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 exceptsb_crcis explicitly filled in in the function.Maybe better to just set
sb_crcto 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 1/1 xfs: fix crc field handling in xfs_sb_to/from_disk
In reply to Eric SandeenI 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.
---
fs/xfs/xfs_sb.c +5 -0
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs