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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Aug 13, 2020 at 02:03:24PM +0800, 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" get wrong inode addr due to it trys to get the
> beginning of an inode chunk, refer to "533d1d229 xfs_db: properly set
> inode type".

It took me a minute to figure out what this was referring to (though it
was obvious from the code change).  Might I suggest something like:

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

> We don't need to get the beginning of a chunk in set_iocur_type, due
> to set_cur_inode(ino) will help to do all of that and make a proper
> verification. We just need to give it a correct inode.
> 
> Reported-by: Jianhong Yin <jiyin@redhat.com>
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
>  db/io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/db/io.c b/db/io.c
> index 6628d061..61940a07 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -591,6 +591,7 @@ set_iocur_type(
>  	/* Inodes are special; verifier checks all inodes in the chunk */
>  	if (type->typnm == TYP_INODE) {
>  		xfs_daddr_t	b = iocur_top->bb;
> +		int		bo = iocur_top->boff;
>  		xfs_ino_t	ino;
>  
>  		/*
> @@ -598,7 +599,7 @@ set_iocur_type(
>   		 * 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) %
> +			(((b << BBSHIFT) + bo) >> mp->m_sb.sb_inodelog) %
>  			XFS_AGB_TO_AGINO(mp, mp->m_sb.sb_agblocks));

/me feels like this whole thing ought to be revised into something
involving XFS_OFFBNO_TO_AGINO to make the unit conversions easier to
read, e.g.:

	xfs_daddr_t	b = iocur_top->bb;
	xfs_agbno_t	agbno;
	xfs_agino_t	agino;

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

--D

>  		set_cur_inode(ino);
>  		return;
> -- 
> 2.20.1
>