[PATCH] xfs_db: properly set inode type

[PATCH] xfs_db: properly set inode type

[Eric Sandeen]

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>
---

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

Re: [PATCH] xfs_db: properly set inode type

[Darrick J. Wong]

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

Re: [PATCH] xfs_db: properly set inode type

[Eric Sandeen]

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

Re: [PATCH] xfs_db: properly set inode type

[Eric Sandeen]

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

Re: [PATCH] xfs_db: properly set inode type

[Darrick J. Wong]

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

Re: [PATCH] xfs_db: properly set inode type

[Eric Sandeen]

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
> 

Re: [PATCH] xfs_db: properly set inode type

[Eric Sandeen]

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

Re: [PATCH] xfs_db: properly set inode type

[Darrick J. Wong]

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

[PATCH V2] xfs_db: properly set inode type

[Eric Sandeen]

When we set the type to "inode" the verifier may validate 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>
---

V2: Warn if the daddr given was reset to an inode-aligned
address

diff --git a/db/io.c b/db/io.c
index b046c8d..4153da0 100644
--- a/db/io.c
+++ b/db/io.c
@@ -616,6 +616,22 @@ set_iocur_type(
 {
 	struct xfs_buf	*bp = iocur_top->bp;
 
+	/* Inodes are special; verifier checks all inodes in the buffer */
+	if (type->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);
+		if (iocur_top->bb != b)
+			dbprintf(
+_("cursor set to nearest inode %llu at daddr %lld\n"),
+				 ino, iocur_top->bb);
+		return;
+	}
+
 	/* adjust buffer size for types with fields & hence fsize() */
 	if (type->fields) {
 		int bb_count;	/* type's size in basic blocks */

Re: [PATCH] xfs_db: properly set inode type

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs_db: properly set inode type

[Eric Sandeen]

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.
> 

Re: [PATCH] xfs_db: properly set inode type

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH V2] xfs_db: properly set inode type

[Bill O'Donnell]

On Mon, Jul 17, 2017 at 08:51:13PM -0500, Eric Sandeen wrote:
> When we set the type to "inode" the verifier may validate 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>
> ---

I ran the new, proposed test xfs/424 to test this patch and
it runs fine. Thanks!

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> 
> V2: Warn if the daddr given was reset to an inode-aligned
> address
> 
> diff --git a/db/io.c b/db/io.c
> index b046c8d..4153da0 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -616,6 +616,22 @@ set_iocur_type(
>  {
>  	struct xfs_buf	*bp = iocur_top->bp;
>  
> +	/* Inodes are special; verifier checks all inodes in the buffer */
> +	if (type->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);
> +		if (iocur_top->bb != b)
> +			dbprintf(
> +_("cursor set to nearest inode %llu at daddr %lld\n"),
> +				 ino, iocur_top->bb);
> +		return;
> +	}
> +
>  	/* adjust buffer size for types with fields & hence fsize() */
>  	if (type->fields) {
>  		int bb_count;	/* type's size in basic blocks */
> 
> --
> 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