Re: [PATCH v2] xfs_db: use correct inode to set inode type

[Zorro Lang <zlang@redhat.com>]

On Thu, Aug 13, 2020 at 03:49:42PM -0500, Eric Sandeen wrote:
> On 8/13/20 10:30 AM, Zorro Lang wrote:
> > A test fails as:
> >   # xfs_db -c "inode 133" -c "addr" -c "p core.size" -c "type inode" -c "addr" -c "p core.size" /dev/sdb1
> >   current
> >           byte offset 68096, length 512
> >           buffer block 128 (fsbno 16), 32 bbs
> >           inode 133, dir inode -1, type inode
> >   core.size = 123142
> >   current
> >           byte offset 65536, length 512
> >           buffer block 128 (fsbno 16), 32 bbs
> >           inode 128, dir inode 128, type inode
> >   core.size = 42
> > 
> > The "type inode" command accidentally moves the io cursor because it
> > forgets to include the io cursor's buffer offset when it computes the
> > inode number from the io cursor's location.
> > 
> > Fixes: 533d1d229a88 ("xfs_db: properly set inode type")
> > 
> > Reported-by: Jianhong Yin <jiyin@redhat.com>
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> 
> This looks good to me, I'll test it.  And I think I have a cleanup
> to the whole function, after this fix...

Sure, the buffer verification code is added by:

  9ba69ce2 ("db: verify buffer on type change")

But later we turn to use set_cur() in set_iocur_type(), which looks do
buffer verification too.

Thanks,
Zorro

> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> > ---
> > 
> > V2 did below changes:
> > 0) git commit log is changed.
> > 1) Separate out several clear steps to calculate inode.
> > 2) Remove improper comment which describe inode calculation.
> > 
> > Thanks,
> > Zorro
> > 
> >  db/io.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/db/io.c b/db/io.c
> > index 6628d061..884da599 100644
> > --- a/db/io.c
> > +++ b/db/io.c
> > @@ -588,18 +588,20 @@ set_iocur_type(
> >  {
> >  	struct xfs_buf	*bp = iocur_top->bp;
> >  
> > -	/* Inodes are special; verifier checks all inodes in the chunk */
> > +	/*
> > +	 * Inodes are special; verifier checks all inodes in the chunk, the
> > +	 * set_cur_inode() will help that
> > +	 */
> >  	if (type->typnm == TYP_INODE) {
> >  		xfs_daddr_t	b = iocur_top->bb;
> > +		xfs_agblock_t	agbno;
> > +		xfs_agino_t	agino;
> >  		xfs_ino_t	ino;
> >  
> > -		/*
> > -		 * Note that this will back up to the beginning of the inode
> > - 		 * which contains the current disk location; daddr may change.
> > - 		 */
> > -		ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b),
> > -			((b << BBSHIFT) >> mp->m_sb.sb_inodelog) %
> > -			XFS_AGB_TO_AGINO(mp, mp->m_sb.sb_agblocks));
> > +		agbno = xfs_daddr_to_agbno(mp, b);
> > +		agino = XFS_OFFBNO_TO_AGINO(mp, agbno,
> > +				iocur_top->boff / mp->m_sb.sb_inodesize);
> > +		ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), agino);
> >  		set_cur_inode(ino);
> >  		return;
> >  	}
> > 
>