| Message ID | e6c83eea-af29-c01b-752e-e049e8fdc69f@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Jun 27, 2017 at 07:16:39PM -0500, Eric Sandeen wrote: > When we set the type to "inode" the verifier validates multiple > inodes in the current fs block, so setting the buffer size to > that of just one inode is not sufficient and it'll emit spurious > verifier errors for all but the first, as we read off the end: > > xfs_db> daddr 99 > xfs_db> type inode > Metadata corruption detected at xfs_inode block 0x63/0x200 > Metadata corruption detected at xfs_inode block 0x63/0x200 > Metadata corruption detected at xfs_inode block 0x63/0x200 > Metadata corruption detected at xfs_inode block 0x63/0x200 > Metadata corruption detected at xfs_inode block 0x63/0x200 > Metadata corruption detected at xfs_inode block 0x63/0x200 > Metadata corruption detected at xfs_inode block 0x63/0x200 > > Use the special set_cur_inode() function for this purpose > as is done in inode_f(). Do you need a similar thing for dquots? I sort of worry that we're going down the rabbit hole of special casing all over xfs_db, but... I'll defer to you. :) > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > diff --git a/db/io.c b/db/io.c > index b97b710..655a978 100644 > --- a/db/io.c > +++ b/db/io.c > @@ -618,6 +618,18 @@ set_iocur_type( > struct xfs_buf *bp = iocur_top->bp; > int bb_count; > > + /* Inodes are special; verifier checks all inodes in the buffer */ > + if (t->typnm == TYP_INODE) { > + xfs_daddr_t b = iocur_top->bb; > + xfs_ino_t ino; > + > + ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), > + ((b << BBSHIFT) >> mp->m_sb.sb_inodelog) % > + (mp->m_sb.sb_agblocks << mp->m_sb.sb_inopblog)); XFS_OFFBNO_TO_AGINO(mp, xfs_daddr_to_agbno(mp, b), 0) instead of that long third argument? --D > + set_cur_inode(ino); > + return; > + } > + > /* adjust cursor for types that contain fields */ > if (t->fields) { > bb_count = BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0))); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/27/17 7:42 PM, Darrick J. Wong wrote: > On Tue, Jun 27, 2017 at 07:16:39PM -0500, Eric Sandeen wrote: >> When we set the type to "inode" the verifier validates multiple >> inodes in the current fs block, so setting the buffer size to >> that of just one inode is not sufficient and it'll emit spurious >> verifier errors for all but the first, as we read off the end: >> >> xfs_db> daddr 99 >> xfs_db> type inode >> Metadata corruption detected at xfs_inode block 0x63/0x200 >> Metadata corruption detected at xfs_inode block 0x63/0x200 >> Metadata corruption detected at xfs_inode block 0x63/0x200 >> Metadata corruption detected at xfs_inode block 0x63/0x200 >> Metadata corruption detected at xfs_inode block 0x63/0x200 >> Metadata corruption detected at xfs_inode block 0x63/0x200 >> Metadata corruption detected at xfs_inode block 0x63/0x200 >> >> Use the special set_cur_inode() function for this purpose >> as is done in inode_f(). > > Do you need a similar thing for dquots? I sort of worry that we're > going down the rabbit hole of special casing all over xfs_db, but... > I'll defer to you. :) I thought we did, but a quick test worked... the dquot verifier only verifies one (indeed printing the dquot only shows the first one in the block) so for now, I think not. I did find it interesting/curious that i.e. "agi_f" hard-codes the size and doesn't use the type's size function, but for now I'm just ignoring that... >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> diff --git a/db/io.c b/db/io.c >> index b97b710..655a978 100644 >> --- a/db/io.c >> +++ b/db/io.c >> @@ -618,6 +618,18 @@ set_iocur_type( >> struct xfs_buf *bp = iocur_top->bp; >> int bb_count; >> >> + /* Inodes are special; verifier checks all inodes in the buffer */ >> + if (t->typnm == TYP_INODE) { >> + xfs_daddr_t b = iocur_top->bb; >> + xfs_ino_t ino; >> + >> + ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), >> + ((b << BBSHIFT) >> mp->m_sb.sb_inodelog) % >> + (mp->m_sb.sb_agblocks << mp->m_sb.sb_inopblog)); > > XFS_OFFBNO_TO_AGINO(mp, xfs_daddr_to_agbno(mp, b), 0) instead of that > long third argument? Macros, macros everywhere. Ok, sounds good. -Eric > --D > >> + set_cur_inode(ino); >> + return; >> + } >> + >> /* adjust cursor for types that contain fields */ >> if (t->fields) { >> bb_count = BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0))); >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/27/17 7:42 PM, Darrick J. Wong wrote: >> + ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), >> + ((b << BBSHIFT) >> mp->m_sb.sb_inodelog) % >> + (mp->m_sb.sb_agblocks << mp->m_sb.sb_inopblog)); > XFS_OFFBNO_TO_AGINO(mp, xfs_daddr_to_agbno(mp, b), 0) instead of that > long third argument? Hm, nope: xfs_db> inode 99 xfs_db> daddr current daddr is 99 xfs_db> daddr 99 xfs_db> type inode xfs_db> inode current inode number is 96 That ends up taking the first inode in the fsblock (12), not the sector (99) I guess. The macro needs a non-zero offset into the fsblock... found by, um... I'm not sure that's going to be much prettier. How much do you hate how I wrote it first? ;) (I kind of hate it a lot but dunno what else we have?) -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 28, 2017 at 05:45:55PM -0500, Eric Sandeen wrote: > On 6/27/17 7:42 PM, Darrick J. Wong wrote: > >> + ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), > >> + ((b << BBSHIFT) >> mp->m_sb.sb_inodelog) % > >> + (mp->m_sb.sb_agblocks << mp->m_sb.sb_inopblog)); > > > XFS_OFFBNO_TO_AGINO(mp, xfs_daddr_to_agbno(mp, b), 0) instead of that > > long third argument? > > Hm, nope: > > xfs_db> inode 99 > xfs_db> daddr > current daddr is 99 > > xfs_db> daddr 99 > xfs_db> type inode > xfs_db> inode > current inode number is 96 > > > That ends up taking the first inode in the fsblock (12), not the sector > (99) I guess. > > The macro needs a non-zero offset into the fsblock... found by, um... > I'm not sure that's going to be much prettier. > > How much do you hate how I wrote it first? ;) (I kind of hate it a > lot but dunno what else we have?) I guess there's also the problem that if inodesize != 512 then what are we targeting, anyway? If inodesize = 256 then we can only hit even-numbered inodes (not so bad) but if inodesize > 512 then do we jump back to wherever the inode starts? Or just give the user what they asked for, even if it's garbage? (FWIW I was fine with xfs_db being dumb and giving you exactly what you point it at, even if that makes no sense. :P) --D > > -Eric > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/28/17 6:54 PM, Darrick J. Wong wrote: > On Wed, Jun 28, 2017 at 05:45:55PM -0500, Eric Sandeen wrote: >> On 6/27/17 7:42 PM, Darrick J. Wong wrote: >>>> + ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), >>>> + ((b << BBSHIFT) >> mp->m_sb.sb_inodelog) % >>>> + (mp->m_sb.sb_agblocks << mp->m_sb.sb_inopblog)); >> >>> XFS_OFFBNO_TO_AGINO(mp, xfs_daddr_to_agbno(mp, b), 0) instead of that >>> long third argument? >> >> Hm, nope: >> >> xfs_db> inode 99 >> xfs_db> daddr >> current daddr is 99 >> >> xfs_db> daddr 99 >> xfs_db> type inode >> xfs_db> inode >> current inode number is 96 >> >> >> That ends up taking the first inode in the fsblock (12), not the sector >> (99) I guess. >> >> The macro needs a non-zero offset into the fsblock... found by, um... >> I'm not sure that's going to be much prettier. >> >> How much do you hate how I wrote it first? ;) (I kind of hate it a >> lot but dunno what else we have?) > > I guess there's also the problem that if inodesize != 512 then what are > we targeting, anyway? If inodesize = 256 then we can only hit > even-numbered inodes (not so bad) but if inodesize > 512 then do we jump > back to wherever the inode starts? Or just give the user what they > asked for, even if it's garbage? Oh, hum. Right, big inodes.... So I'm trying to go from disk location to inode number, but the current disk location may not even be the start of an inode. Hell. Well, I'm sure I could craft a test to disallow "type inode" if the current location cannot be on an inode boundary. But is that too clever? Should the debug tool just do what the user asks? > (FWIW I was fine with xfs_db being dumb and giving you exactly what you > point it at, even if that makes no sense. :P) yeah, if you do "daddr 0" "type agf" it'll squawk, too, so it's fine to do the dumb user's bidding... The difference here is that to set things up properly we need "an" inode number. Probably best to reject locations that cannot be the starts of inodes, I guess, but that kind of violates the spirit of a debug tool. Maybe we're /looking/ for misaligned inodes ... Bleah. -Eric > --D > >> >> -Eric >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/28/2017 11:01 PM, Eric Sandeen wrote: >> I guess there's also the problem that if inodesize != 512 then what are >> we targeting, anyway? If inodesize = 256 then we can only hit >> even-numbered inodes (not so bad) but if inodesize > 512 then do we jump >> back to wherever the inode starts? Or just give the user what they >> asked for, even if it's garbage? > Oh, hum. Right, big inodes.... > > So I'm trying to go from disk location to inode number, but the current > disk location may not even be the start of an inode. Hell. Well, I'm > sure I could craft a test to disallow "type inode" if the current location > cannot be on an inode boundary. But is that too clever? Should the > debug tool just do what the user asks? > >> (FWIW I was fine with xfs_db being dumb and giving you exactly what you >> point it at, even if that makes no sense. :P) > yeah, if you do "daddr 0" "type agf" it'll squawk, too, so it's fine to > do the dumb user's bidding... So as I have it today, issuing "type inode" will actually re-align you to an inode offset even if you're not on one: xfs_db> sb 0 xfs_db> addr rootino xfs_db> daddr current daddr is 128 xfs_db> daddr 129 xfs_db> type inode xfs_db> daddr current daddr is 128 xfs_db> good? bad? indifferent? I'm thinking "bad" but we have no mechanism to read a batch of inodes in xfs_db other than by passing in a legit inode number. And we have no mechanism to read only a /single/ inode... Maybe print a warning about the re-alignment and carry on? -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 17, 2017 at 03:51:50PM -0500, Eric Sandeen wrote: > > > On 06/28/2017 11:01 PM, Eric Sandeen wrote: > >> I guess there's also the problem that if inodesize != 512 then what are > >> we targeting, anyway? If inodesize = 256 then we can only hit > >> even-numbered inodes (not so bad) but if inodesize > 512 then do we jump > >> back to wherever the inode starts? Or just give the user what they > >> asked for, even if it's garbage? > > Oh, hum. Right, big inodes.... > > > > So I'm trying to go from disk location to inode number, but the current > > disk location may not even be the start of an inode. Hell. Well, I'm > > sure I could craft a test to disallow "type inode" if the current location > > cannot be on an inode boundary. But is that too clever? Should the > > debug tool just do what the user asks? > > > >> (FWIW I was fine with xfs_db being dumb and giving you exactly what you > >> point it at, even if that makes no sense. :P) > > yeah, if you do "daddr 0" "type agf" it'll squawk, too, so it's fine to > > do the dumb user's bidding... > > So as I have it today, issuing "type inode" will actually re-align you > to an inode offset even if you're not on one: > > xfs_db> sb 0 > xfs_db> addr rootino > xfs_db> daddr > current daddr is 128 > xfs_db> daddr 129 > xfs_db> type inode > xfs_db> daddr > current daddr is 128 > xfs_db> > > good? bad? indifferent? I'm thinking "bad" but we have no mechanism to > read a batch of inodes in xfs_db other than by passing in a legit inode number. I suppose there's the argument that if the user feeds us garbage then they'll find out pretty quickly when the CRC checks blow up in print... ...or that it's a debugging tool so if they want to do weird things then let them. > And we have no mechanism to read only a /single/ inode... > > Maybe print a warning about the re-alignment and carry on? Sure. --D > > -Eric > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 17, 2017 at 03:51:50PM -0500, Eric Sandeen wrote: > > > On 06/28/2017 11:01 PM, Eric Sandeen wrote: > >> I guess there's also the problem that if inodesize != 512 then what are > >> we targeting, anyway? If inodesize = 256 then we can only hit > >> even-numbered inodes (not so bad) but if inodesize > 512 then do we jump > >> back to wherever the inode starts? Or just give the user what they > >> asked for, even if it's garbage? > > Oh, hum. Right, big inodes.... > > > > So I'm trying to go from disk location to inode number, but the current > > disk location may not even be the start of an inode. Hell. Well, I'm > > sure I could craft a test to disallow "type inode" if the current location > > cannot be on an inode boundary. But is that too clever? Should the > > debug tool just do what the user asks? > > > >> (FWIW I was fine with xfs_db being dumb and giving you exactly what you > >> point it at, even if that makes no sense. :P) > > yeah, if you do "daddr 0" "type agf" it'll squawk, too, so it's fine to > > do the dumb user's bidding... > > So as I have it today, issuing "type inode" will actually re-align you > to an inode offset even if you're not on one: > > xfs_db> sb 0 > xfs_db> addr rootino > xfs_db> daddr > current daddr is 128 > xfs_db> daddr 129 > xfs_db> type inode > xfs_db> daddr > current daddr is 128 > xfs_db> > > good? bad? indifferent? I'm thinking "bad" but we have no mechanism to > read a batch of inodes in xfs_db other than by passing in a legit inode number. > > And we have no mechanism to read only a /single/ inode... > > Maybe print a warning about the re-alignment and carry on? xfs_db is meant to be a developer tool, not idiot-proof. :P i.e. some knowledge of the layout of the metadata you are trying to examine is expected. I would not like xfs_db to change the daddr I've specified simply because I tried to parse it as the wrong object type.... Cheers, Dave.
On 07/17/2017 09:20 PM, Dave Chinner wrote: > On Mon, Jul 17, 2017 at 03:51:50PM -0500, Eric Sandeen wrote: >> >> >> On 06/28/2017 11:01 PM, Eric Sandeen wrote: >>>> I guess there's also the problem that if inodesize != 512 then what are >>>> we targeting, anyway? If inodesize = 256 then we can only hit >>>> even-numbered inodes (not so bad) but if inodesize > 512 then do we jump >>>> back to wherever the inode starts? Or just give the user what they >>>> asked for, even if it's garbage? >>> Oh, hum. Right, big inodes.... >>> >>> So I'm trying to go from disk location to inode number, but the current >>> disk location may not even be the start of an inode. Hell. Well, I'm >>> sure I could craft a test to disallow "type inode" if the current location >>> cannot be on an inode boundary. But is that too clever? Should the >>> debug tool just do what the user asks? >>> >>>> (FWIW I was fine with xfs_db being dumb and giving you exactly what you >>>> point it at, even if that makes no sense. :P) >>> yeah, if you do "daddr 0" "type agf" it'll squawk, too, so it's fine to >>> do the dumb user's bidding... >> >> So as I have it today, issuing "type inode" will actually re-align you >> to an inode offset even if you're not on one: >> >> xfs_db> sb 0 >> xfs_db> addr rootino >> xfs_db> daddr >> current daddr is 128 >> xfs_db> daddr 129 >> xfs_db> type inode >> xfs_db> daddr >> current daddr is 128 >> xfs_db> >> >> good? bad? indifferent? I'm thinking "bad" but we have no mechanism to >> read a batch of inodes in xfs_db other than by passing in a legit inode number. >> >> And we have no mechanism to read only a /single/ inode... >> >> Maybe print a warning about the re-alignment and carry on? > > xfs_db is meant to be a developer tool, not idiot-proof. :P > > i.e. some knowledge of the layout of the metadata you are trying > to examine is expected. I would not like xfs_db to change the daddr > I've specified simply because I tried to parse it as the wrong > object type.... That would be my first preference as well, but all the infrastructure we have for gathering up an inode from a cluster is predicated on being given an inode nr, not a daddr. I could hack the heck out of it and refactor it to allow clusters starting on arbitrary sectors, I guess, but I'm not sure it's worth it. For now it seems more useful than not to be able to switch from a daddr to an inode (which possibly /contains/ rather than starts at that daddr ...) -Eric > Cheers, > > Dave. > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 17, 2017 at 09:25:57PM -0500, Eric Sandeen wrote: > > > On 07/17/2017 09:20 PM, Dave Chinner wrote: > > On Mon, Jul 17, 2017 at 03:51:50PM -0500, Eric Sandeen wrote: > >> > >> > >> On 06/28/2017 11:01 PM, Eric Sandeen wrote: > >>>> I guess there's also the problem that if inodesize != 512 then what are > >>>> we targeting, anyway? If inodesize = 256 then we can only hit > >>>> even-numbered inodes (not so bad) but if inodesize > 512 then do we jump > >>>> back to wherever the inode starts? Or just give the user what they > >>>> asked for, even if it's garbage? > >>> Oh, hum. Right, big inodes.... > >>> > >>> So I'm trying to go from disk location to inode number, but the current > >>> disk location may not even be the start of an inode. Hell. Well, I'm > >>> sure I could craft a test to disallow "type inode" if the current location > >>> cannot be on an inode boundary. But is that too clever? Should the > >>> debug tool just do what the user asks? > >>> > >>>> (FWIW I was fine with xfs_db being dumb and giving you exactly what you > >>>> point it at, even if that makes no sense. :P) > >>> yeah, if you do "daddr 0" "type agf" it'll squawk, too, so it's fine to > >>> do the dumb user's bidding... > >> > >> So as I have it today, issuing "type inode" will actually re-align you > >> to an inode offset even if you're not on one: > >> > >> xfs_db> sb 0 > >> xfs_db> addr rootino > >> xfs_db> daddr > >> current daddr is 128 > >> xfs_db> daddr 129 > >> xfs_db> type inode > >> xfs_db> daddr > >> current daddr is 128 > >> xfs_db> > >> > >> good? bad? indifferent? I'm thinking "bad" but we have no mechanism to > >> read a batch of inodes in xfs_db other than by passing in a legit inode number. > >> > >> And we have no mechanism to read only a /single/ inode... > >> > >> Maybe print a warning about the re-alignment and carry on? > > > > xfs_db is meant to be a developer tool, not idiot-proof. :P > > > > i.e. some knowledge of the layout of the metadata you are trying > > to examine is expected. I would not like xfs_db to change the daddr > > I've specified simply because I tried to parse it as the wrong > > object type.... > > That would be my first preference as well, but all the infrastructure > we have for gathering up an inode from a cluster is predicated on being > given an inode nr, not a daddr. I could hack the heck out of it and refactor > it to allow clusters starting on arbitrary sectors, I guess, but I'm not > sure it's worth it. For now it seems more useful than not to be able to > switch from a daddr to an inode (which possibly /contains/ rather than starts > at that daddr ...) That's what already have a mechanism for converting arbitrary daddrs to a correctly aligned inode number: xfs_db> convert daddr <daddr> inode Cheers, Dave.
diff --git a/db/io.c b/db/io.c index b97b710..655a978 100644 --- a/db/io.c +++ b/db/io.c @@ -618,6 +618,18 @@ set_iocur_type( struct xfs_buf *bp = iocur_top->bp; int bb_count; + /* Inodes are special; verifier checks all inodes in the buffer */ + if (t->typnm == TYP_INODE) { + xfs_daddr_t b = iocur_top->bb; + xfs_ino_t ino; + + ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), + ((b << BBSHIFT) >> mp->m_sb.sb_inodelog) % + (mp->m_sb.sb_agblocks << mp->m_sb.sb_inopblog)); + set_cur_inode(ino); + return; + } + /* adjust cursor for types that contain fields */ if (t->fields) { bb_count = BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));
When we set the type to "inode" the verifier validates multiple inodes in the current fs block, so setting the buffer size to that of just one inode is not sufficient and it'll emit spurious verifier errors for all but the first, as we read off the end: xfs_db> daddr 99 xfs_db> type inode Metadata corruption detected at xfs_inode block 0x63/0x200 Metadata corruption detected at xfs_inode block 0x63/0x200 Metadata corruption detected at xfs_inode block 0x63/0x200 Metadata corruption detected at xfs_inode block 0x63/0x200 Metadata corruption detected at xfs_inode block 0x63/0x200 Metadata corruption detected at xfs_inode block 0x63/0x200 Metadata corruption detected at xfs_inode block 0x63/0x200 Use the special set_cur_inode() function for this purpose as is done in inode_f(). Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html