xfsdump:fill in bs_forkoff

PATCH In review Eric Sandeen 5 participants 9 messages Oct 19, 2012 – Nov 2, 2012

Share & tools

Eric Sandeen

author

2012-10-19 04:00 UTC #0 lore raw

PATCH 1/1 xfsdump:fill in bs_forkoff

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 <[email protected]>

---

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.

common/arch_xlate.c +1 -0

Index: 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);

dump/content.c +1 -0

Index: 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

Reviewed-by on v1 1/1

2012-10-23 12:25 UTC #1 lore raw

Quoted 10 lines · 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 <[email protected]>

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Ben Myers

2012-10-30 19:45 UTC #2 lore raw

In reply to Eric Sandeen

Hey Eric,

Quoted 10 lines · 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 <[email protected]>

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

Thanks,
Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

author

2012-10-30 19:52 UTC #3 lore raw

Quoted 8 lines · Ben Myers

Hey Eric,
Quoted 5 lines · 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.

Quoted 7 lines

Quoted 5 lines
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 <[email protected]>
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

Quoted 3 lines

Thanks,
Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Ben Myers

2012-10-31 19:45 UTC #4 lore raw

Hi Eric,

Quoted 19 lines · Eric Sandeen

Quoted 8 lines · Ben Myers
Hey Eric,
Quoted 5 lines · 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.
Quoted 7 lines
Quoted 5 lines
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 <[email protected]>
Would you say that this is appropriate for the upcoming release?
Hm.

Quoted 7 lines

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2012-11-02 04:34 UTC #5 lore raw

Quoted 1 line · Ben Myers

Hi Eric,

...

Quoted 3 lines

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2012-11-02 05:14 UTC #6 lore raw

Quoted 11 lines · Eric Sandeen

Quoted 1 line · Ben Myers
Hi Eric,
...
Quoted 3 lines
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. (?!)

Quoted 7 lines

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.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Ben Myers

2012-11-02 16:45 UTC #7 lore raw

In reply to Eric Sandeen

Hey Eric,

Quoted 9 lines · Eric Sandeen

Quoted 1 line · Ben Myers
Hi Eric,
...
Quoted 3 lines
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...

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 1 line

Sorry about that,

No problem. It worked out fine. ;)

Thanks,
Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Ben Myers

2012-11-02 17:22 UTC #8 lore raw

In reply to Eric Sandeen

Quoted 10 lines · 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 <[email protected]>

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

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs