On Wed, Oct 04, 2017 at 10:47:14PM -0700, Darrick J. Wong wrote:
> On Thu, Oct 05, 2017 at 01:08:10PM +1100, Dave Chinner wrote:
> > On Tue, Oct 03, 2017 at 01:42:11PM -0700, Darrick J. Wong wrote:
> > > +/* Count the number of free inodes. */
> > > +static unsigned int
> > > +xfs_scrub_iallocbt_freecount(
> > > + xfs_inofree_t freemask)
> > > +{
> > > + int bits = XFS_INODES_PER_CHUNK;
> > > + unsigned int ret = 0;
> > > +
> > > + while (bits--) {
> > > + if (freemask & 1)
> > > + ret++;
> > > + freemask >>= 1;
> > > + }
> >
> >
> > Seems a little cumbersome. Perhaps a loop using xfs_next_bit()
> > might be a bit faster, something like:
> >
> > nextbit = xfs_next_bit(&freemask, 1, 0);
> > while (nextbit != -1) {
> > ret++;
> > nextbit = xfs_next_bit(&freemask, 1, nextbit + 1);
> > }
>
> <nod> A pity there's no popcnt()...
Oh, hweight64().
> > > + error = xfs_icache_inode_is_allocated(mp, bs->cur->bc_tp,
> > > + fsino + clusterino, &inuse);
> > > + if (error == -ENODATA) {
> > > + /* Not cached, just read the disk buffer */
> >
> > I think that is wrong. xfs_icache_inode_is_allocated() returns
> > -ENOENT if the inode is not in cache....
>
> I changed it to ENODATA so that we can tell the difference between
> inode not in cache (ENODATA) and inode racing with unlink (ENOENT).
>
> (Patch was sent to the ML a while ago and I omitted it from this posting...)
Ah, if it's not committed upstream yet, include it in the next
posting, please :)
> > > + /* Make sure the freemask matches the inode records. */
> > > + blks_per_cluster = xfs_icluster_size_fsb(mp);
> > > + nr_inodes = XFS_OFFBNO_TO_AGINO(mp, blks_per_cluster, 0);
> >
> > Does this setup and loop work for the case where we have 64k
> > filesystem blocks and so two or more inode chunks per filesystem
> > block (i.e. ppc64)?
>
> I think the answer is yes, at worst case we end up processing a block's
> worth of inodes at a time. The last time I ran scrub on ppc64 (last
> week) it worked fine.
Hmmm - there's nothing to count how many inodes are scrubbed, is
there? Perhaps it would be good to gcount as we go so we know if
we've scrubbed all the inodes?
Hmmm - I might have missed it, but is there anywhere in this code
where we check the inode number in the inode that we have read
actually matches the agino we are attempting to validate?
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
