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.
> > > + /* 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.
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
