On Tue, Feb 05, 2019 at 09:53:09AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 05, 2019 at 09:24:59AM -0500, Brian Foster wrote:
> > On Mon, Feb 04, 2019 at 10:00:05AM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > >
> > > Use a rhashtable to cache the unlinked list incore. This should speed
> > > up unlinked processing considerably when there are a lot of inodes on
> > > the unlinked list because iunlink_remove no longer has to traverse an
> > > entire bucket list to find which inode points to the one being removed.
> > >
> > > The incore list structure records "X.next_unlinked = Y" relations, with
> > > the rhashtable using Y to index the records. This makes finding the
> > > inode X that points to a inode Y very quick. If our cache fails to find
> > > anything we can always fall back on the old method.
> > >
> > > FWIW this drastically reduces the amount of time it takes to remove
> > > inodes from the unlinked list. I wrote a program to open a lot of
> > > O_TMPFILE files and then close them in the same order, which takes
> > > a very long time if we have to traverse the unlinked lists. With the
> > > ptach, I see:
> > >
> > ...
> > > ---
> > > fs/xfs/xfs_inode.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++
> > > fs/xfs/xfs_inode.h | 9 ++
> > > fs/xfs/xfs_log_recover.c | 12 ++-
> > > fs/xfs/xfs_mount.c | 5 +
> > > fs/xfs/xfs_mount.h | 1
> > > 5 files changed, 233 insertions(+), 1 deletion(-)
> > >
> > >
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index b9696d762c8f..baee8c894447 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -1880,6 +1880,167 @@ xfs_inactive(
> > > xfs_qm_dqdetach(ip);
> > > }
> > >
> > ...
> > > +
> > > +static const struct rhashtable_params xfs_iunlink_hash_params = {
> > > + .min_size = XFS_AGI_UNLINKED_BUCKETS,
> > > + .nelem_hint = 512,
> >
> > Any reasoning behind the 512 value? It seems rather large to me, at
> > least until we get more into deferred inactivation and whatnot. It looks
> > like the rhashtable code will round this up to 1024 as well, FWIW.
> >
> > I'm also wondering whether a kmem_zone might be worthwhile for
> > xfs_iunlink structures, but that's probably also more for when we expect
> > to drive deeper unlinked lists.
>
> I picked an arbitrary value of 64 buckets * 8 items per list. I /do/
> have plans to test various values to see if there's a particular sweet
> spot, though I guess this could be much lower on the assumption that
> we don't expect /that/ many unlinked inodes(?)
Seems pretty large, given we use this for the per-ag buffer cache
rhashtable:
.min_size = 32, /* empty AGs have minimal footprint */
.nelem_hint = 16,
And nobody notices problems when they grow and shrink and they run
from empty to hundreds of thousands of entries and back again in
very short preiods of time. Hence I'd suggest that we make it as
small as possible to begin with and then only change things if there
are performance problems triggered by growing and shrinking....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
