Re: [PATCH 25/30 V2] libxfs: fix root inode handling inconsistencies

Dave Chinner
2013-11-04T23:22:50Z

On Fri, Nov 01, 2013 at 06:03:16AM -0700, Christoph Hellwig wrote:
> On Fri, Nov 01, 2013 at 09:04:38AM +1100, Dave Chinner wrote:
> > > > - if ((flags & LIBXFS_MOUNT_ROOTINOS) && sbp->sb_rootino != NULLFSINO &&
> > > > + if (sbp->sb_rootino != NULLFSINO &&
> > > > xfs_sb_version_haslazysbcount(&mp->m_sb)) {
> > >
> > > Oh, I hadn't noticed that LIBXFS_MOUNT_ROOTINOS does more than reading
> > > the root inode. Seems like mkfs might need the rt inodes if a file with
> > > the rt flag is specified in the proto file.
> >
> > mkfs always allocates the rt inodes directly after the root
> > directory is created.
>
> True, we obviosuly create them in mkfs. In fact looking over the code
> the only call into the rtalloc code in userspace ever is mkfs
> initializing those using libxfs_rtfree_extent.
>
> So removing the call to rtmount_inodes is fine, and we should remove
> the declaration of it as well as it's now unused.

You are right, they are otherwise unused. repair and db just look at
the raw inode numbers, and do direct libxfs_iget calls to get an
xfs_inode and hence don't use the cached values at all. I'll kill it
completely, then.

> In the future I
> suspect we should simplify the mkfs code to not even require
> xfs_rtalloc.c for just marking the inodes as entirely free, but that's
> not something for this series.

Yeah, it seems that way - xfs_repair does all the rebuild itself
without needing the libxfs code, so that seems quite doable.

> > > I can't see how xfs_copy could need either the rt inodes nor the perag
> > > data.
> >
> > Right, it doesn't need them, but it doesn't hurt at all to
> > initialise them because all the ag headers are about to be read to
> > find all the used space, anyway.
>
> xfs_initialize_perag_data does the following:
>
> - read in AGI/AGF. This is something we lazily do whenever we need it
> anyway, so no one should rely on it.
> - update the in-core superblock global counters. Seems like the old
> xfs_check relies on this and still needs an equivalent if we care
> enough. No one else seems to care.

So you are suggesting that I move that initialisation to the
xfs_check code rather than just doing it in the mount code? Or
something else?

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com