xfs
[Top] [All Lists]

Re: [PATCH 2/2] xfs_check: process sparse inode chunks correctly

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs_check: process sparse inode chunks correctly
From: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
Date: Tue, 21 Jun 2016 14:05:13 +0200
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20160621104815.GA3108@xxxxxxxxxxxxxx>
Mail-followup-to: Brian Foster <bfoster@xxxxxxxxxx>, xfs@xxxxxxxxxxx
References: <1466441562-12317-1-git-send-email-bfoster@xxxxxxxxxx> <1466441562-12317-3-git-send-email-bfoster@xxxxxxxxxx> <20160621090153.GB28212@xxxxxxxxxx> <20160621104815.GA3108@xxxxxxxxxxxxxx>
User-agent: Mutt/1.5.24 (2015-08-30)
On Tue, Jun 21, 2016 at 06:48:15AM -0400, Brian Foster wrote:
> On Tue, Jun 21, 2016 at 11:01:53AM +0200, Carlos Maiolino wrote:
> > On Mon, Jun 20, 2016 at 12:52:42PM -0400, Brian Foster wrote:
> > > Update the inode btree scanning functions to process sparse inode chunks
> > > correctly. For filesystems with sparse inode support enabled, process
> > > each chunk a cluster at a time. Each cluster is checked against the
> > > inobt record to determine if it is a hole and skipped if so.
> > > 
> > > Note that since xfs_check is deprecated in favor of xfs_repair,  this
> > > adds the minimum support necessary to process sparse inode enabled
> > > filesystems. In other words, this adds no sparse inode specific checks
> > > or verifications. We only update the inobt scanning functions to extend
> > > the existing level of verification to sparse inode enabled filesystems
> > > (e.g., avoid incorrectly tracking sparse regions as inodes). Problems
> > > or corruptions associated with sparse inode records must be detected and
> > > recovered via xfs_repair.
> > > 
> > 
> > Hi,
> > 
> > I'm not quite sure, but a while ago, I thought I've heard some rumors about
> > deprecating xfs_check, is this true or something that my mind made up for 
> > some
> > weird reason?
> > 
> 
> I actually thought it already was. :) xfstests still runs check in
> certain cases, however, and these patches just fix a regression.
> Personally, I'd be fine with just dumping an "fs has sparse inodes, use
> repair" message from xfs_check, but the basic sparse inode support had
> already been added.
> 

:) I had no objection about the patch, just to make it clear. I was just curious
about how much we still care about xfs_check :)

> Brian
> 
> > 
> > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > > ---
> > >  db/check.c | 143 
> > > +++++++++++++++++++++++++++++++++++++++++++------------------
> > >  1 file changed, 102 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/db/check.c b/db/check.c
> > > index 750ecc1..25146e5 100644
> > > --- a/db/check.c
> > > +++ b/db/check.c
> > > @@ -4324,10 +4324,24 @@ scanfunc_ino(
> > >   int                     i;
> > >   int                     isfree;
> > >   int                     j;
> > > + int                     freecount;
> > >   int                     nfree;
> > >   int                     off;
> > >   xfs_inobt_ptr_t         *pp;
> > >   xfs_inobt_rec_t         *rp;
> > > + xfs_agblock_t           agbno;
> > > + xfs_agblock_t           end_agbno;
> > > + struct xfs_dinode       *dip;
> > > + int                     blks_per_buf;
> > > + int                     inodes_per_buf;
> > > + int                     ioff;
> > > +
> > > + if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> > > +         blks_per_buf = xfs_icluster_size_fsb(mp);
> > > + else
> > > +         blks_per_buf = mp->m_ialloc_blks;
> > > + inodes_per_buf = min(blks_per_buf << mp->m_sb.sb_inopblog,
> > > +                      XFS_INODES_PER_CHUNK);
> > >  
> > >   if (be32_to_cpu(block->bb_magic) != XFS_IBT_MAGIC &&
> > >       be32_to_cpu(block->bb_magic) != XFS_IBT_CRC_MAGIC) {
> > > @@ -4357,54 +4371,74 @@ scanfunc_ino(
> > >           rp = XFS_INOBT_REC_ADDR(mp, block, 1);
> > >           for (i = 0; i < be16_to_cpu(block->bb_numrecs); i++) {
> > >                   agino = be32_to_cpu(rp[i].ir_startino);
> > > -                 off = XFS_INO_TO_OFFSET(mp, agino);
> > > +                 agbno = XFS_AGINO_TO_AGBNO(mp, agino);
> > > +                 off = XFS_AGINO_TO_OFFSET(mp, agino);
> > > +                 end_agbno = agbno + mp->m_ialloc_blks;
> > >                   if (off == 0) {
> > >                           if ((sbversion & XFS_SB_VERSION_ALIGNBIT) &&
> > >                               mp->m_sb.sb_inoalignmt &&
> > >                               (XFS_INO_TO_AGBNO(mp, agino) %
> > >                                mp->m_sb.sb_inoalignmt))
> > >                                   sbversion &= ~XFS_SB_VERSION_ALIGNBIT;
> > > -                         set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, agino),
> > > -                                 (xfs_extlen_t)MAX(1,
> > > -                                         XFS_INODES_PER_CHUNK >>
> > > -                                         mp->m_sb.sb_inopblog),
> > > -                                 DBM_INODE, seqno, bno);
> > >                   }
> > > -                 icount += XFS_INODES_PER_CHUNK;
> > > -                 agicount += XFS_INODES_PER_CHUNK;
> > > -                 ifree += be32_to_cpu(rp[i].ir_u.f.ir_freecount);
> > > -                 agifreecount += be32_to_cpu(rp[i].ir_u.f.ir_freecount);
> > > +
> > >                   push_cur();
> > > -                 set_cur(&typtab[TYP_INODE],
> > > -                         XFS_AGB_TO_DADDR(mp, seqno,
> > > -                                          XFS_AGINO_TO_AGBNO(mp, agino)),
> > > -                         (int)XFS_FSB_TO_BB(mp, mp->m_ialloc_blks),
> > > -                         DB_RING_IGN, NULL);
> > > -                 if (iocur_top->data == NULL) {
> > > -                         if (!sflag)
> > > -                                 dbprintf(_("can't read inode block "
> > > -                                          "%u/%u\n"),
> > > -                                         seqno,
> > > -                                         XFS_AGINO_TO_AGBNO(mp, agino));
> > > -                         error++;
> > > -                         pop_cur();
> > > -                         continue;
> > > -                 }
> > > -                 for (j = 0, nfree = 0; j < XFS_INODES_PER_CHUNK; j++) {
> > > -                         isfree = XFS_INOBT_IS_FREE_DISK(&rp[i], j);
> > > -                         if (isfree)
> > > -                                 nfree++;
> > > -                         process_inode(agf, agino + j,
> > > -                                 (xfs_dinode_t *)((char 
> > > *)iocur_top->data + ((off + j) << mp->m_sb.sb_inodelog)),
> > > -                                         isfree);
> > > +
> > > +                 ioff = 0;
> > > +                 nfree = 0;
> > > +                 while (agbno < end_agbno &&
> > > +                        ioff < XFS_INODES_PER_CHUNK) {
> > > +                         if (xfs_inobt_is_sparse_disk(&rp[i], ioff))
> > > +                                 goto next_buf;
> > > +
> > > +                         if (off < XFS_INODES_PER_CHUNK)
> > > +                                 set_dbmap(seqno, agbno, blks_per_buf,
> > > +                                           DBM_INODE, seqno, bno);
> > > +
> > > +                         icount += inodes_per_buf;
> > > +                         agicount += inodes_per_buf;
> > > +
> > > +                         set_cur(&typtab[TYP_INODE],
> > > +                                 XFS_AGB_TO_DADDR(mp, seqno, agbno),
> > > +                                 XFS_FSB_TO_BB(mp, blks_per_buf),
> > > +                                 DB_RING_IGN, NULL);
> > > +                         if (iocur_top->data == NULL) {
> > > +                                 if (!sflag)
> > > +                                         dbprintf(_("can't read inode 
> > > block "
> > > +                                                    "%u/%u\n"), seqno,
> > > +                                                  agbno);
> > > +                                 error++;
> > > +                                 goto next_buf;
> > > +                         }
> > > +
> > > +                         for (j = 0; j < inodes_per_buf; j++) {
> > > +                                 isfree = XFS_INOBT_IS_FREE_DISK(&rp[i], 
> > > ioff + j);
> > > +                                 if (isfree)
> > > +                                         nfree++;
> > > +                                 dip = (xfs_dinode_t *)((char 
> > > *)iocur_top->data +
> > > +                                         ((off + j) << 
> > > mp->m_sb.sb_inodelog));
> > > +                                 process_inode(agf, agino + ioff + j, 
> > > dip, isfree);
> > > +                         }
> > > +
> > > +next_buf:
> > > +                         agbno += blks_per_buf;
> > > +                         ioff += inodes_per_buf;
> > >                   }
> > > -                 if (nfree != be32_to_cpu(rp[i].ir_u.f.ir_freecount)) {
> > > +
> > > +                 if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> > > +                         freecount = rp[i].ir_u.sp.ir_freecount;
> > > +                 else
> > > +                         freecount = 
> > > be32_to_cpu(rp[i].ir_u.f.ir_freecount);
> > > +
> > > +                 ifree += freecount;
> > > +                 agifreecount += freecount;
> > > +
> > > +                 if (nfree != freecount) {
> > >                           if (!sflag)
> > >                                   dbprintf(_("ir_freecount/free mismatch, 
> > > "
> > >                                            "inode chunk %u/%u, freecount "
> > >                                            "%d nfree %d\n"),
> > > -                                         seqno, agino,
> > > -                                         
> > > be32_to_cpu(rp[i].ir_u.f.ir_freecount), nfree);
> > > +                                         seqno, agino, freecount, nfree);
> > >                           error++;
> > >                   }
> > >                   pop_cur();
> > > @@ -4439,6 +4473,18 @@ scanfunc_fino(
> > >   int                     off;
> > >   xfs_inobt_ptr_t         *pp;
> > >   struct xfs_inobt_rec    *rp;
> > > + xfs_agblock_t           agbno;
> > > + xfs_agblock_t           end_agbno;
> > > + int                     blks_per_buf;
> > > + int                     inodes_per_buf;
> > > + int                     ioff;
> > > +
> > > + if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> > > +         blks_per_buf = xfs_icluster_size_fsb(mp);
> > > + else
> > > +         blks_per_buf = mp->m_ialloc_blks;
> > > + inodes_per_buf = min(blks_per_buf << mp->m_sb.sb_inopblog,
> > > +                      XFS_INODES_PER_CHUNK);
> > >  
> > >   if (be32_to_cpu(block->bb_magic) != XFS_FIBT_MAGIC &&
> > >       be32_to_cpu(block->bb_magic) != XFS_FIBT_CRC_MAGIC) {
> > > @@ -4468,19 +4514,34 @@ scanfunc_fino(
> > >           rp = XFS_INOBT_REC_ADDR(mp, block, 1);
> > >           for (i = 0; i < be16_to_cpu(block->bb_numrecs); i++) {
> > >                   agino = be32_to_cpu(rp[i].ir_startino);
> > > -                 off = XFS_INO_TO_OFFSET(mp, agino);
> > > +                 agbno = XFS_AGINO_TO_AGBNO(mp, agino);
> > > +                 off = XFS_AGINO_TO_OFFSET(mp, agino);
> > > +                 end_agbno = agbno + mp->m_ialloc_blks;
> > >                   if (off == 0) {
> > >                           if ((sbversion & XFS_SB_VERSION_ALIGNBIT) &&
> > >                               mp->m_sb.sb_inoalignmt &&
> > >                               (XFS_INO_TO_AGBNO(mp, agino) %
> > >                                mp->m_sb.sb_inoalignmt))
> > >                                   sbversion &= ~XFS_SB_VERSION_ALIGNBIT;
> > > -                         check_set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, 
> > > agino),
> > > -                                 (xfs_extlen_t)MAX(1,
> > > -                                         XFS_INODES_PER_CHUNK >>
> > > -                                         mp->m_sb.sb_inopblog),
> > > -                                 DBM_INODE, DBM_INODE, seqno, bno);
> > >                   }
> > > +
> > > +                 ioff = 0;
> > > +                 while (agbno < end_agbno &&
> > > +                        ioff < XFS_INODES_PER_CHUNK) {
> > > +                         if (xfs_inobt_is_sparse_disk(&rp[i], ioff))
> > > +                                 goto next_buf;
> > > +
> > > +                                check_set_dbmap(seqno, agbno,
> > > +                                        (xfs_extlen_t)MAX(1,
> > > +                                                inodes_per_buf >>
> > > +                                                mp->m_sb.sb_inopblog),
> > > +                                        DBM_INODE, DBM_INODE, seqno, 
> > > bno);
> > > +
> > > +next_buf:
> > > +                         agbno += blks_per_buf;
> > > +                         ioff += inodes_per_buf;
> > > +                 }
> > > +
> > >           }
> > >           return;
> > >   }
> > > -- 
> > > 2.5.5
> > > 
> > > _______________________________________________
> > > xfs mailing list
> > > xfs@xxxxxxxxxxx
> > > http://oss.sgi.com/mailman/listinfo/xfs
> > 
> > -- 
> > Carlos
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@xxxxxxxxxxx
> > http://oss.sgi.com/mailman/listinfo/xfs
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

<Prev in Thread] Current Thread [Next in Thread>