On Sat, Nov 11, 2023 at 07:40:43AM +1100, Dave Chinner wrote:
> On Fri, Nov 10, 2023 at 11:27:52AM -0800, Darrick J. Wong wrote:
> > On Fri, Nov 10, 2023 at 03:33:13PM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > >
> > > Discovered when trying to track down a weird recovery corruption
> > > issue that wasn't detected at recovery time.
> > >
> > > The specific corruption was a zero extent count field when big
> > > extent counts are in use, and it turns out the dinode verifier
> > > doesn't detect that specific corruption case, either. So fix it too.
> > >
> > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > > ---
> > > fs/xfs/libxfs/xfs_inode_buf.c | 3 +++
> > > fs/xfs/xfs_inode_item_recover.c | 14 +++++++++++++-
> > > 2 files changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > > index a35781577cad..0f970a0b3382 100644
> > > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > > @@ -508,6 +508,9 @@ xfs_dinode_verify(
> > > if (mode && nextents + naextents > nblocks)
> > > return __this_address;
> > >
> > > + if (nextents + naextents == 0 && nblocks != 0)
> > > + return __this_address;
> > > +
> > > if (S_ISDIR(mode) && nextents > mp->m_dir_geo->max_extents)
> > > return __this_address;
> > >
> > > diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c
> > > index 6b09e2bf2d74..f4c31c2b60d5 100644
> > > --- a/fs/xfs/xfs_inode_item_recover.c
> > > +++ b/fs/xfs/xfs_inode_item_recover.c
> > > @@ -286,6 +286,7 @@ xlog_recover_inode_commit_pass2(
> > > struct xfs_log_dinode *ldip;
> > > uint isize;
> > > int need_free = 0;
> > > + xfs_failaddr_t fa;
> > >
> > > if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) {
> > > in_f = item->ri_buf[0].i_addr;
> > > @@ -529,8 +530,19 @@ xlog_recover_inode_commit_pass2(
> > > (dip->di_mode != 0))
> > > error = xfs_recover_inode_owner_change(mp, dip, in_f,
> > > buffer_list);
> > > - /* re-generate the checksum. */
> > > + /* re-generate the checksum and validate the recovered inode. */
> > > xfs_dinode_calc_crc(log->l_mp, dip);
> > > + fa = xfs_dinode_verify(log->l_mp, in_f->ilf_ino, dip);
> > > + if (fa) {
> >
> > Does xlog_recover_dquot_commit_pass2 need to call xfs_dquot_verify as
> > well?
>
> Maybe - I haven't looked closely at that, and it depends what the
> dquot buffer verifier does. If it's similar to the inode cluster
> buffer verifier (i.e. only checks for dquots, doesn't verify the
> dquots) then it should do the same thing. I don't have time to do
> this right now because I'm OOO for the next week, so maybe you could
> check this and send a patch for it?
I tossed on a patch to do this and after a couple of days of generic/388
and generic/475 spinning I haven't noticed any failures.
--D
> > This patch looks good though,
> > Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> Thanks!
>
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
