Re: [PATCH] xfs_db: properly set inode type

[Dave Chinner <david@fromorbit.com>]

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