On Wed, Jun 06, 2018 at 02:32:46PM +1000, Dave Chinner wrote:
> On Tue, Jun 05, 2018 at 08:55:28PM -0700, Darrick J. Wong wrote:
> > On Mon, Jun 04, 2018 at 01:41:30PM +1000, Dave Chinner wrote:
> > > On Wed, May 30, 2018 at 12:31:04PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > > >
> > > > Use the rmapbt to find inode chunks, query the chunks to compute
> > > > hole and free masks, and with that information rebuild the inobt
> > > > and finobt.
> > > >
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > >
> > > [...]
> > >
> > > > +xfs_repair_ialloc_check_free(
> > > > + struct xfs_btree_cur *cur,
> > > > + struct xfs_buf *bp,
> > > > + xfs_ino_t fsino,
> > > > + xfs_agino_t bpino,
> > > > + bool *inuse)
> > > > +{
> > > > + struct xfs_mount *mp = cur->bc_mp;
> > > > + struct xfs_dinode *dip;
> > > > + int error;
> > > > +
> > > > + /* Will the in-core inode tell us if it's in use? */
> > > > + error = xfs_icache_inode_is_allocated(mp, cur->bc_tp, fsino, inuse);
> > > > + if (!error)
> > > > + return 0;
> > > > +
> > > > + /* Inode uncached or half assembled, read disk buffer */
> > > > + dip = xfs_buf_offset(bp, bpino * mp->m_sb.sb_inodesize);
> > > > + if (be16_to_cpu(dip->di_magic) != XFS_DINODE_MAGIC)
> > > > + return -EFSCORRUPTED;
> > >
> > > Do we hold the buffer locked here? i.e. can we race with someone
> > > else allocating/freeing/reading the inode?
> >
> > I think repair should be ok from alloc/free because both of those paths
> > (xfs_dialloc/xfs_difree) will grab the AGI header, whereas repair locks
> > all three AG headers and keeps them locked until repairs are complete.
> > I don't think we have to worry about concurrent reads because the only
> > field we care about are di_mode/i_mode, which don't change outside of
> > inode allocation and freeing.
>
> Comment please :P
>
> And, to be technically correct - di_mode/i_mode can change outside
> of alloc/free. However, only the permission bits can change so it
> doesn't affect the test we are doing here.
Done.
> > > > + /* The per-AG inum of this inode cluster. */
> > > > + agino = XFS_OFFBNO_TO_AGINO(mp, agbno, 0);
> > > > +
> > > > + /* The per-AG inum of the inobt record. */
> > > > + startino = rmino +
> > > > + rounddown(agino - rmino, XFS_INODES_PER_CHUNK);
> > > > + cdist = agino - startino;
> > >
> > > What's "cdist" mean? I can guess at it's meaning, but I don't recall
> > > seeing the inode number offset into a cluster been refered to as a
> > > distanced before....
> >
> > cluster offset?
> >
> > I wasn't sure of the terminology for the offset of the cluster within a
> > chunk, in units of ag inodes.
>
> I'm not sure we have one. :/
>
> But, yeah, going by the definition of inode offset from
> XFS_INO_TO_OFFSET() and XFS_AGINO_TO_OFFSET() - "offset" is the
> inode number index from the start of the block - cluster offset is
> probably the best name for it.
Yeah, that's what I picked.
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
