Bug 201453
Summary: | Bug 1640090 - [xfstests xfs/490]: xfs_db print a bad (negative number) as agi freecount | ||
---|---|---|---|
Product: | File System | Reporter: | Zorro Lang (zlang) |
Component: | XFS | Assignee: | FileSystem/XFS Default Virtual Assignee (filesystem_xfs) |
Status: | NEW --- | ||
Severity: | normal | CC: | sandeen |
Priority: | P1 | ||
Hardware: | All | ||
OS: | Linux | ||
Kernel Version: | v4.18 | Subsystem: | |
Regression: | No | Bisected commit-id: | |
Attachments: | xfs metadump |
Description
Zorro Lang
2018-10-17 11:20:42 UTC
Created attachment 279077 [details]
xfs metadump
On Wed, Oct 17, 2018 at 11:20:42AM +0000, bugzilla-daemon@bugzilla.kernel.org wrote: > # _scratch_xfs_get_metadata_field "recs[1].freecount" "agi 0" "addr root" > -197 > ]# xfs_db -c "agi 0" -c "addr root" -c "print recs[1]" /dev/loop1 > recs[1] = [startino,holemask,count,freecount,free] > 1:[128,0,64,-197,0xffffffffffffffe0] ..... > And it's not reproducible on x86_64: > # xfs_db -c "agi 0" -c "addr root" -c "print recs[1]" /dev/loop1 > recs[1] = [startino,holemask,count,freecount,free] > 1:[128,0,64,59,0xffffffffffffffe0] -197 = -(256 - 59) This looks like a sign extension problem in the xfs_db code. s390 is a big endian system, right? Cheers, Dave. Yep, does this fix it? diff --git a/db/btblock.c b/db/btblock.c index cbd2990..5a5b061 100644 --- a/db/btblock.c +++ b/db/btblock.c @@ -513,7 +513,7 @@ const field_t inobt_sprec_flds[] = { { "holemask", FLDT_UINT16X, OI(ROFF(ir_u.sp.ir_holemask)), C1, 0, TYP_NONE }, { "count", FLDT_UINT8D, OI(ROFF(ir_u.sp.ir_count)), C1, 0, TYP_NONE }, - { "freecount", FLDT_INT8D, OI(ROFF(ir_u.sp.ir_freecount)), C1, 0, + { "freecount", FLDT_UINT8D, OI(ROFF(ir_u.sp.ir_freecount)), C1, 0, TYP_NONE }, { "free", FLDT_INOFREE, OI(ROFF(ir_free)), C1, 0, TYP_NONE }, { NULL } (In reply to Eric Sandeen from comment #3) > Yep, does this fix it? Yes, this's helpful. # xfs_db -c "agi 0" -c "addr root" -c "print recs[1]" /dev/loop1 recs[1] = [startino,holemask,count,freecount,free] 1:[64,0,64,59,0xffffffffffffffe0] > > diff --git a/db/btblock.c b/db/btblock.c > index cbd2990..5a5b061 100644 > --- a/db/btblock.c > +++ b/db/btblock.c > @@ -513,7 +513,7 @@ const field_t inobt_sprec_flds[] = { > { "holemask", FLDT_UINT16X, OI(ROFF(ir_u.sp.ir_holemask)), C1, 0, > TYP_NONE }, > { "count", FLDT_UINT8D, OI(ROFF(ir_u.sp.ir_count)), C1, 0, TYP_NONE > }, > - { "freecount", FLDT_INT8D, OI(ROFF(ir_u.sp.ir_freecount)), C1, 0, > + { "freecount", FLDT_UINT8D, OI(ROFF(ir_u.sp.ir_freecount)), C1, 0, > TYP_NONE }, > { "free", FLDT_INOFREE, OI(ROFF(ir_free)), C1, 0, TYP_NONE }, > { NULL } So zorro correctly points out that the big vs little endian certainly should not matter for this u8. What does matter is the signed type, because getbitval is doing tricks to try to handle sign extension and it does it differently for big vs. little endian: if (getbit_l(p, bit + i)) { /* If the last bit is on and we care about sign * bits and we don't have a full 64 bit * container, turn all bits on between the * sign bit and the most sig bit. */ /* handle endian swap here */ #if __BYTE_ORDER == LITTLE_ENDIAN if (i == 0 && signext && nbits < 64) rval = (~0ULL) << nbits; rval |= 1ULL << (nbits - i - 1); #else if ((i == (nbits - 1)) && signext && nbits < 64) rval |= ((~0ULL) << nbits); rval |= 1ULL << (nbits - i - 1); #endif Switching it to FLDT_UINT8D makes "signext" false so none of this happens, but that's papering over the underlying bug with signed types. The bug seems to be the test for if ((i == (nbits - 1)) ...) - this is testing the last / rightmost bit in the number, which is /not/ the MSB. But I cannot seem to wrap my head around the right way to fix it, yet. |