| Message ID | 20200813060324.8159-1-zlang@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs_db: use correct inode to set inode type | expand |
Hi, 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". From the kernel side, the prefered way is commit id ("subject") > > 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)); > set_cur_inode(ino); > return; Not familar with such code, but after looking into a bit, (my premature thought is that) I'm wondering if we need to reverify original buffer in if (type->fields) { ... set_cur() } iocur_top->typ = type; /* verify the buffer if the type has one. */ ... since set_cur() already verified the buffer in set_cur->libxfs_buf_read->...->libxfs_readbuf_verify? Not related to this patchset but I'm a bit curious about it now... Thanks, Gao Xiang > -- > 2.20.1 >
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 >
On Thu, Aug 13, 2020 at 11:29:05PM +0800, Zorro Lang wrote: ... > > > 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)); > > > set_cur_inode(ino); > > > return; > > > > Not familar with such code, but after looking into a bit, (my premature > > thought is that) I'm wondering if we need to reverify original buffer in > > > > if (type->fields) { > > ... > > set_cur() > > } > > > > iocur_top->typ = type; > > > > /* verify the buffer if the type has one. */ > > ... > > > > since set_cur() already verified the buffer in > > set_cur->libxfs_buf_read->...->libxfs_readbuf_verify? > > > > Not related to this patchset but I'm a bit curious about it now... > > I'm not the one who learn about xfsprogs best:) But by looking into > set_cur_inode() -> set_cur(), I think the set_cur() does the xfs_buf > verification, so I don't think we need to do that again at the end > of set_iocur_type(). Nope, I wasn't saying we need to add anything after set_cur_inode(). but commit 55f224baf83d ("xfs_db: update buffer size when new type is set") In detail, I think it might be if (type->fields) { ... set_cur() return; <- here } iocur_top->typ = type; /* verify the buffer if the type has one. */ ... Thanks, Gao Xiang > > Thanks, > Zorro > > > > > Thanks, > > Gao Xiang > > > > > -- > > > 2.20.1 > > > > > >
On Thu, Aug 13, 2020 at 09:53:45PM +0800, Gao Xiang wrote: > Hi, > > 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". > > From the kernel side, the prefered way is > commit id ("subject") Hi Xiang, Thanks for your review. I'll change my commit log. > > > > > 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)); > > set_cur_inode(ino); > > return; > > Not familar with such code, but after looking into a bit, (my premature > thought is that) I'm wondering if we need to reverify original buffer in > > if (type->fields) { > ... > set_cur() > } > > iocur_top->typ = type; > > /* verify the buffer if the type has one. */ > ... > > since set_cur() already verified the buffer in > set_cur->libxfs_buf_read->...->libxfs_readbuf_verify? > > Not related to this patchset but I'm a bit curious about it now... I'm not the one who learn about xfsprogs best:) But by looking into set_cur_inode() -> set_cur(), I think the set_cur() does the xfs_buf verification, so I don't think we need to do that again at the end of set_iocur_type(). Thanks, Zorro > > Thanks, > Gao Xiang > > > -- > > 2.20.1 > > >
On Thu, Aug 13, 2020 at 07:56:16AM -0700, Darrick J. Wong wrote: > 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 Sorry about that :-p > 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") Sure, thanks for this detailed suggestion. > > > 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); Sure, that looks clearer. Thanks, Zorro > > --D > > > set_cur_inode(ino); > > return; > > -- > > 2.20.1 > > >
On 8/13/20 6:53 AM, Gao Xiang wrote: > Hi, > > On Thu, Aug 13, 2020 at 02:03:24PM +0800, Zorro Lang wrote: ... >> 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)); >> set_cur_inode(ino); >> return; > > Not familar with such code, but after looking into a bit, (my premature > thought is that) I'm wondering if we need to reverify original buffer in > > if (type->fields) { > ... > set_cur() > } > > iocur_top->typ = type; > > /* verify the buffer if the type has one. */ > ... > > since set_cur() already verified the buffer in > set_cur->libxfs_buf_read->...->libxfs_readbuf_verify? > > Not related to this patchset but I'm a bit curious about it now... I'm wondering about all this, too. set_cur_inode() actually calls set_cur, which /does/ get into the verifier. It definitely seems like a mess; the early return from the if (type->typnm == TYP_INODE) { block is a little weird, and the explicit verify_read() later in the function seems like it might be unnecessary? Agreed that it's unrelated to this bug, though. -Eric
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)); 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" 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". 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(-)