[PATCH] xfsdump:fill in bs_forkoff

[PATCH] xfsdump:fill in bs_forkoff

[Eric Sandeen]

Upstream, the structure containing bs_forkoff is actually zeroed
prior to these functions, but when pulling the patch back to an
older xfsdump, we got checksum errors due to an uninitialized
bs_forkoff not matching in dump vs. restore.

So even though forkoff won't be explicitly restored from
a dump, do explicitly set it in these routines to keep checksums
happy.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Tested w/ ./check -g dump on a 3.6.0 kernel, no failures.

Setting to 0 would work too, if that would be better?
It just looked odd.



Index: xfsdump-3.0.4/common/arch_xlate.c
===================================================================
--- xfsdump-3.0.4.orig/common/arch_xlate.c
+++ xfsdump-3.0.4/common/arch_xlate.c
@@ -377,6 +377,7 @@ xlate_bstat(bstat_t *bs1, bstat_t *bs2, 
 	IXLATE(bs1, bs2, bs_extents);
 	IXLATE(bs1, bs2, bs_gen);
 	IXLATE(bs1, bs2, bs_projid_lo);
+	IXLATE(bs1, bs2, bs_forkoff);
 	IXLATE(bs1, bs2, bs_projid_hi);
 	IXLATE(bs1, bs2, bs_dmevmask);
 	IXLATE(bs1, bs2, bs_dmstate);
Index: xfsdump-3.0.4/dump/content.c
===================================================================
--- xfsdump-3.0.4.orig/dump/content.c
+++ xfsdump-3.0.4/dump/content.c
@@ -5072,6 +5072,7 @@ copy_xfs_bstat(bstat_t *dst, xfs_bstat_t
 	dst->bs_extents = src->bs_extents;
 	dst->bs_gen = src->bs_gen;
 	dst->bs_projid_lo = src->bs_projid_lo;
+	dst->bs_forkoff = src->bs_forkoff;
 	dst->bs_projid_hi = src->bs_projid_hi;
 	dst->bs_dmevmask = src->bs_dmevmask;
 	dst->bs_dmstate = src->bs_dmstate;

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump:fill in bs_forkoff

[Christoph Hellwig]

On Thu, Oct 18, 2012 at 11:02:05PM -0500, Eric Sandeen wrote:
> Upstream, the structure containing bs_forkoff is actually zeroed
> prior to these functions, but when pulling the patch back to an
> older xfsdump, we got checksum errors due to an uninitialized
> bs_forkoff not matching in dump vs. restore.
> 
> So even though forkoff won't be explicitly restored from
> a dump, do explicitly set it in these routines to keep checksums
> happy.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump:fill in bs_forkoff

[Ben Myers]

Hey Eric,

On Thu, Oct 18, 2012 at 11:02:05PM -0500, Eric Sandeen wrote:
> Upstream, the structure containing bs_forkoff is actually zeroed
> prior to these functions, but when pulling the patch back to an
> older xfsdump, we got checksum errors due to an uninitialized
> bs_forkoff not matching in dump vs. restore.
> 
> So even though forkoff won't be explicitly restored from
> a dump, do explicitly set it in these routines to keep checksums
> happy.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Would you say that this is appropriate for the upcoming release?

Thanks,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump:fill in bs_forkoff

[Eric Sandeen]

On 10/30/12 2:47 PM, Ben Myers wrote:
> Hey Eric,
> 
> On Thu, Oct 18, 2012 at 11:02:05PM -0500, Eric Sandeen wrote:
>> Upstream, the structure containing bs_forkoff is actually zeroed
>> prior to these functions, but when pulling the patch back to an
>> older xfsdump, we got checksum errors due to an uninitialized
>> bs_forkoff not matching in dump vs. restore.
>>
>> So even though forkoff won't be explicitly restored from
>> a dump, do explicitly set it in these routines to keep checksums
>> happy.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Would you say that this is appropriate for the upcoming release?

Hm.

The zeroing isn't in a really obvious spot, IIRC, so explicitly
filling in all members leaves nothing to chance.

OTOH it's a member that (will/should) never get restored,
so filling it in is a little confusing.  What do you think?

I think it should be harmless to functionality either way.

-Eric

> Thanks,
> 	Ben
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump:fill in bs_forkoff

[Ben Myers]

Hi Eric,

On Tue, Oct 30, 2012 at 02:54:29PM -0500, Eric Sandeen wrote:
> On 10/30/12 2:47 PM, Ben Myers wrote:
> > Hey Eric,
> > 
> > On Thu, Oct 18, 2012 at 11:02:05PM -0500, Eric Sandeen wrote:
> >> Upstream, the structure containing bs_forkoff is actually zeroed
> >> prior to these functions, but when pulling the patch back to an
> >> older xfsdump, we got checksum errors due to an uninitialized
> >> bs_forkoff not matching in dump vs. restore.
> >>
> >> So even though forkoff won't be explicitly restored from
> >> a dump, do explicitly set it in these routines to keep checksums
> >> happy.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > 
> > Would you say that this is appropriate for the upcoming release?
> 
> Hm.
> 
> The zeroing isn't in a really obvious spot, IIRC, so explicitly
> filling in all members leaves nothing to chance.
> 
> OTOH it's a member that (will/should) never get restored,
> so filling it in is a little confusing.  What do you think?
>
> I think it should be harmless to functionality either way.

It seemed like it could be an important bugfix but I wasn't really sure so I
asked.  Since it sounds like it's not a big deal, lets just hold off till after
the release...

Thanks,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump:fill in bs_forkoff

[Eric Sandeen]

On 10/31/12 2:46 PM, Ben Myers wrote:
> Hi Eric,

...

> It seemed like it could be an important bugfix but I wasn't really sure so I
> asked.  Since it sounds like it's not a big deal, lets just hold off till after
> the release...

Well, I was wrong - at least at one point, I thought it was getting zeroed. (?!)

But Dave was running into trouble tonight ...

And as the git tree stands today, I'm getting checksum errors w/o this patch
too, and xfstests dump group is passing only with it added back in.

So I think you'd better pull it in, maybe change the commit message a bit
though - it downplays the importance too much I guess.

Sorry about that,

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump:fill in bs_forkoff

[Dave Chinner]

On Thu, Nov 01, 2012 at 11:35:54PM -0500, Eric Sandeen wrote:
> On 10/31/12 2:46 PM, Ben Myers wrote:
> > Hi Eric,
> 
> ...
> 
> > It seemed like it could be an important bugfix but I wasn't really sure so I
> > asked.  Since it sounds like it's not a big deal, lets just hold off till after
> > the release...
> 
> Well, I was wrong - at least at one point, I thought it was getting zeroed. (?!)
> 
> But Dave was running into trouble tonight ...
> 
> And as the git tree stands today, I'm getting checksum errors w/o this patch
> too, and xfstests dump group is passing only with it added back in.
> 
> So I think you'd better pull it in, maybe change the commit message a bit
> though - it downplays the importance too much I guess.

Yes, this patch fixes the failures I see under normal conditions.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump:fill in bs_forkoff

[Ben Myers]

Hey Eric,

On Thu, Nov 01, 2012 at 11:35:54PM -0500, Eric Sandeen wrote:
> On 10/31/12 2:46 PM, Ben Myers wrote:
> > Hi Eric,
> 
> ...
> 
> > It seemed like it could be an important bugfix but I wasn't really sure so I
> > asked.  Since it sounds like it's not a big deal, lets just hold off till after
> > the release...
> 
> Well, I was wrong - at least at one point, I thought it was getting zeroed. (?!)
> 
> But Dave was running into trouble tonight ...
> 
> And as the git tree stands today, I'm getting checksum errors w/o this patch
> too, and xfstests dump group is passing only with it added back in.

Interesting!  It works great as-is on i386.  ;)

I gave it a spin on an x86_64 box and now I see the errors too.

> So I think you'd better pull it in, maybe change the commit message a bit
> though - it downplays the importance too much I guess.

I'll add something like:

This fixes 'bad header checksum' errors in xfsrestore, which were introduced by
commit 1e309da7.

> Sorry about that,

No problem.  It worked out fine.  ;)

Thanks,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump:fill in bs_forkoff

[Ben Myers]

On Thu, Oct 18, 2012 at 11:02:05PM -0500, Eric Sandeen wrote:
> Upstream, the structure containing bs_forkoff is actually zeroed
> prior to these functions, but when pulling the patch back to an
> older xfsdump, we got checksum errors due to an uninitialized
> bs_forkoff not matching in dump vs. restore.
> 
> So even though forkoff won't be explicitly restored from
> a dump, do explicitly set it in these routines to keep checksums
> happy.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

committed to git://oss.sgi.com/xfs/cmds/xfsdump.git, master branch.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs