Re: [PATCH] xfs_mdrestore: initialize sb prior to xfs_sb_from_disk()

[Dave Chinner <david@fromorbit.com>]

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