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@xxxxxxxxxx>
> > Signed-off-by: Zorro Lang <zlang@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
>
> > ---
> >
> > 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;
> > }
> >
>
