| Message ID | 20200813173050.26203-1-zlang@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] xfs_db: use correct inode to set inode type | expand |
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... 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; > } >
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; > > } > > >
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; }
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> --- 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(-)