* Re: [PATCH] xfsdump: add locks around the inventory put
2013-10-01 21:06 ` Eric Sandeen
@ 2013-10-01 22:19 ` Rich Johnston
2013-10-03 14:17 ` Rich Johnston
1 sibling, 0 replies; 4+ messages in thread
From: Rich Johnston @ 2013-10-01 22:19 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs-oss
On 10/01/2013 04:06 PM, Eric Sandeen wrote:
> hi Rich -
>
> On 10/1/13 11:30 AM, Rich Johnston wrote:
>> From: Phil White <cerise-xfs@l.armory.com>
>>
>> Add locks around the inventory put to prevent inventory
>> corruption.
>>
>> Signed-off-by: Phil White <cerise-xfs@l.armory.com>
>
> Similar questions here; it says it's thread-safe, but apparently
> not. But why not? what happens? Is there a testcase?
Phil worked on this for a long time. To my knowledge the corruption
occured with multistream dumps only. I did not change his code but we
have had reports from the field on corruption when using multistream
dumps which does not occur with this fix.
Not sure what else Phil did to reproduce this other than a multistream
dump/restore (20 streams), which fails.
Testcase?? Good point I need to submit a testcase for both patches.
>
> (you guys probably have longer history in ptools, maybe you
> can see what if anything changed since the original comment
> was added - or maybe when it was added, etc?)
>
I did not look into this further, I will see if there is a better
explaination in our bug writeup.
> Thanks,
> -Eric
>
>> diff --git a/dump/content.c b/dump/content.c
>> index ac19021..b8977bb 100644
>> --- a/dump/content.c
>> +++ b/dump/content.c
>> @@ -2550,8 +2550,11 @@ decision_more:
>> scwhdrp->cih_startpt.sp_offset );
>> }
>>
>> - /* already thread-safe, don't need to lock
>> + /* Supposedly already thread-safe, according to the
>> + * previous revisions, but corruption of inventory
>> + * objects can occur.
>> */
>> */
>> + lock();
>> ok = inv_put_mediafile( inv_stmt,
>> &mwhdrp->mh_mediaid,
>> mwhdrp->mh_medialabel,
>> @@ -2565,6 +2568,7 @@ decision_more:
>> &&
>> ! empty_mediafile,
>> BOOL_FALSE );
>> + unlock();
>> if ( ! ok ) {
>> mlog( MLOG_NORMAL, _(
>> "inventory media file put failed\n") );
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
>>
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] xfsdump: add locks around the inventory put
2013-10-01 21:06 ` Eric Sandeen
2013-10-01 22:19 ` Rich Johnston
@ 2013-10-03 14:17 ` Rich Johnston
1 sibling, 0 replies; 4+ messages in thread
From: Rich Johnston @ 2013-10-03 14:17 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs-oss
On 10/01/2013 04:06 PM, Eric Sandeen wrote:
> hi Rich -
>
> On 10/1/13 11:30 AM, Rich Johnston wrote:
>> From: Phil White <cerise-xfs@l.armory.com>
>>
>> Add locks around the inventory put to prevent inventory
>> corruption.
>>
>> Signed-off-by: Phil White <cerise-xfs@l.armory.com>
>
> Similar questions here; it says it's thread-safe, but apparently
> not. But why not? what happens? Is there a testcase?
This was reported by a customer, no test case, so we used the customers
data to verify the fix. Failed every time the customer performed a 20
stream dump/restore and with single stream. Single stream would fail
rarley with a different error. Two storage object files were corrupted,
causing xfsinvutil -C to crash. As it is customer data we could not
supply it as a test case.
Customer runs fine now with this patch,
Bug info
BUG Title: xfsinvutil seg faults when trying to use -C
#0 0x00007fd83a98c9d2 in __strlen_sse2 () from /lib64/libc.so.6
#1 0x00007fd83a95520d in vfprintf () from /lib64/libc.so.6
#2 0x00007fd83a9f9a2d in __printf_chk () from /lib64/libc.so.6
#3 0x0000000000402b54 in printf (__fmt=<optimized out>) at
/usr/include/bits/stdio2.h:105
#4 CheckAndPruneStObjFile (checkonly=1, StObjFileName=0x7fd83b2e8a00
<Address 0x7fd83b2e8a00 out of bounds>,
sessionp=0x7ffffc02a150, prunetime=0, r_mf_label=0x0) at invutil.c:759
#5 0x000000000040329b in CheckAndPruneInvIndexFile (checkonly=1,
idxFileName=0x60f040
"/var/lib/xfsdump/inventory/aeb05cfe-c923-4972-ad18-2968326aba46.InvIndex",
sessionp=0x7ffffc02a150,
prunetime=0, r_mf_label=0x0) at invutil.c:651
#6 0x00000000004039f6 in CheckAndPruneFstab (inv_path=0x60e6e0
"/var/lib/xfsdump/inventory", checkonly=1,
mountPt=0x40a9c4 "test", uuidp=0x7ffffc02a160,
sessionp=0x7ffffc02a150, prunetime=0, r_mf_label=0x0) at invutil.c:538
#7 0x00000000004043bf in main (argc=2, argv=0x7ffffc02a288) at
invutil.c:253
The corrupted files seem to have struct inv_mediafile or maybe struct
inv_stream overwriting the header of the file, and both seem to
include media
files from multi stream dumps.
83ab1189-b1a7-4733-bfd6-aa8a4feb188e.StObj has
a good header that looks like it was written over a struct
inv_stream... I
suspect we're looking at some kind of locking issue with multistream
xfsdump
adding stream entries to this file in the inventory.
>
> (you guys probably have longer history in ptools, maybe you
> can see what if anything changed since the original comment
> was added - or maybe when it was added, etc?)
Looks like it was added as the IRIX original commit
dump_content.c.p_cat:
cbullis |1.1 | | /* already thread-safe,
don't need to lock
cbullis |1.1 | | */
>
> Thanks,
> -Eric
>
>> diff --git a/dump/content.c b/dump/content.c
>> index ac19021..b8977bb 100644
>> --- a/dump/content.c
>> +++ b/dump/content.c
>> @@ -2550,8 +2550,11 @@ decision_more:
>> scwhdrp->cih_startpt.sp_offset );
>> }
>>
>> - /* already thread-safe, don't need to lock
>> + /* Supposedly already thread-safe, according to the
>> + * previous revisions, but corruption of inventory
>> + * objects can occur.
>> */
>> */
>> + lock();
>> ok = inv_put_mediafile( inv_stmt,
>> &mwhdrp->mh_mediaid,
>> mwhdrp->mh_medialabel,
>> @@ -2565,6 +2568,7 @@ decision_more:
>> &&
>> ! empty_mediafile,
>> BOOL_FALSE );
>> + unlock();
>> if ( ! ok ) {
>> mlog( MLOG_NORMAL, _(
>> "inventory media file put failed\n") );
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
>>
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread