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

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

[Eric Sandeen]

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

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

[Dave Chinner]

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

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

[Eric Sandeen]

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

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

[Dave Chinner]

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

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

[Eric Sandeen]

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

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

[Eric Sandeen]

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

[PATCH V2] xfs: fix crc field handling in xfs_sb_to/from_disk

[Eric Sandeen]

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