On Tue, Jun 15, 2021 at 10:43:28AM -0400, Brian Foster wrote:
> On Mon, Jun 14, 2021 at 12:27:20PM -0700, Darrick J. Wong wrote:
> > On Mon, Jun 14, 2021 at 12:17:03PM -0400, Brian Foster wrote:
> > > On Sun, Jun 13, 2021 at 10:20:29AM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > >
> > > > Instead of calling xfs_inactive directly from xfs_fs_destroy_inode,
> > > > defer the inactivation phase to a separate workqueue. With this change,
> > > > we can speed up directory tree deletions by reducing the duration of
> > > > unlink() calls to the directory and unlinked list updates.
> > > >
> > > > By moving the inactivation work to the background, we can reduce the
> > > > total cost of deleting a lot of files by performing the file deletions
> > > > in disk order instead of directory entry order, which can be arbitrary.
> > > >
> > > > We introduce two new inode flags -- NEEDS_INACTIVE and INACTIVATING.
> > > > The first flag helps our worker find inodes needing inactivation, and
> > > > the second flag marks inodes that are in the process of being
> > > > inactivated. A concurrent xfs_iget on the inode can still resurrect the
> > > > inode by clearing NEEDS_INACTIVE (or bailing if INACTIVATING is set).
> > > >
> > > > Unfortunately, deferring the inactivation has one huge downside --
> > > > eventual consistency. Since all the freeing is deferred to a worker
> > > > thread, one can rm a file but the space doesn't come back immediately.
> > > > This can cause some odd side effects with quota accounting and statfs,
> > > > so we flush inactivation work during syncfs in order to maintain the
> > > > existing behaviors, at least for callers that unlink() and sync().
> > > >
> > > > For this patch we'll set the delay to zero to mimic the old timing as
> > > > much as possible; in the next patch we'll play with different delay
> > > > settings.
> > > >
> > > > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > > ---
> > >
> > > Just some high level questions on a first/cursory pass..
> > >
> > > > Documentation/admin-guide/xfs.rst | 3
> > > > fs/xfs/scrub/common.c | 7 +
> > > > fs/xfs/xfs_icache.c | 332 ++++++++++++++++++++++++++++++++++---
> > > > fs/xfs/xfs_icache.h | 5 +
> > > > fs/xfs/xfs_inode.h | 19 ++
> > > > fs/xfs/xfs_log_recover.c | 7 +
> > > > fs/xfs/xfs_mount.c | 26 +++
> > > > fs/xfs/xfs_mount.h | 21 ++
> > > > fs/xfs/xfs_super.c | 53 ++++++
> > > > fs/xfs/xfs_trace.h | 50 +++++-
> > > > 10 files changed, 490 insertions(+), 33 deletions(-)
> > > >
> > > >
> > > > diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
> > > > index 8de008c0c5ad..f9b109bfc6a6 100644
> > > > --- a/Documentation/admin-guide/xfs.rst
> > > > +++ b/Documentation/admin-guide/xfs.rst
> > > > @@ -524,7 +524,8 @@ and the short name of the data device. They all can be found in:
> > > > mount time quotacheck.
> > > > xfs-gc Background garbage collection of disk space that have been
> > > > speculatively allocated beyond EOF or for staging copy on
> > > > - write operations.
> > > > + write operations; and files that are no longer linked into
> > > > + the directory tree.
> > > > ================ ===========
> > > >
> > > > For example, the knobs for the quotacheck workqueue for /dev/nvme0n1 would be
> > > ...
> > > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > > > index 4002f0b84401..e094c16aa8c5 100644
> > > > --- a/fs/xfs/xfs_icache.c
> > > > +++ b/fs/xfs/xfs_icache.c
> > > ...
> > > > @@ -262,6 +285,9 @@ xfs_perag_set_inode_tag(
> > > > case XFS_ICI_BLOCKGC_TAG:
> > > > xfs_blockgc_queue(pag);
> > > > break;
> > > > + case XFS_ICI_INODEGC_TAG:
> > > > + xfs_inodegc_queue(mp);
> > > > + break;
> > > > }
> > > >
> > > > trace_xfs_perag_set_inode_tag(mp, pag->pag_agno, tag, _RET_IP_);
> > > > @@ -338,28 +364,26 @@ xfs_inode_mark_reclaimable(
> > > > {
> > > > struct xfs_mount *mp = ip->i_mount;
> > > > struct xfs_perag *pag;
> > > > + unsigned int tag;
> > > > bool need_inactive = xfs_inode_needs_inactive(ip);
> > >
> > > Nit: I think we usually try to avoid function calls in the variable
> > > declarations like this..
> >
> > <shrug> For boolean flags that don't change value in the function body
> > I'd rather they be initialized at declaration time so that code
> > maintainers don't have to remember if a variable is initialized or not.
> > need_inactive has scope and a valid value wherever you are in the
> > function body.
> >
>
> I think the usual pattern is to init such variables right after the
> declarations so as to not obfuscate function calls (or at least I find
> that much cleaner).
Very well, I'll change it back.
> > > >
> > > > if (!need_inactive) {
> > > > /* Going straight to reclaim, so drop the dquots. */
> > > > xfs_qm_dqdetach(ip);
> > > > - } else {
> > > > - xfs_inactive(ip);
> > > > - }
> > > >
> > >
> > > Ok, so obviously this disconnects the unlink -> destroy path from the
> > > historical inactive -> reclaimable sequence. Is there any concern for
> > > example from a memory usage standpoint or anything where we might want
> > > to consider waiting on outstanding work here? E.g., if we had a large
> > > filesystem, limited memory and enough CPUs doing unlinks where the
> > > background reclaim simply can't keep up.
> >
> > So far in my evaluation, deferred inactivation has improved reclaim
> > behavior because evict_inodes can push all eligible inodes all the way
> > to IRECLAIMABLE without getting stuck behind xfs_inactive, and the
> > shrinker can free other cached inodes sooner. I think we're still
> > mostly reliant on xfs_inode_alloc looping when memory is low to throttle
> > the size of the inode cache, since I think mass deletions are what push
> > the inodegc code the hardest. If reclaim can't free inodes fast enough,
> > I think that means the AIL is running slowly, in which case both
> > frontend and background workers will block on the log.
> >
> > > I suppose it's a similar situation that inode reclaim is in, but we do
> > > have shrinker feedback to hook back in and actually free available
> > > objects. Do we have or need something like that if too many objects are
> > > stuck waiting on inactivation?
> >
> > There's no direct link between the shrinker and the inodegc worker,
> > which means that it can't push the inodegc worker to get even more
> > inodes to IRECLAIMABLE. I've played with racing directory tree
> > deletions of a filesystem with 8M inodes against fsstress on VMs with
> > 512M of memory, but so far I haven't been able to drive the system into
> > OOM any more easily than I could before this patch. The unlink calls
> > seem to get stuck under xfs_iget, like I expect.
> >
>
> Ok, that's sort of the scenario I was thinking about. I have a high
> memory/cpu count box I occasionally use for such purposes. I threw an
> fs_mark 1k file creation workload at a large fs on that system and let
> it run to -ENOSPC, then ran a 64x parallel rm -rf of the top-level
> subdirs. In the baseline case (a 5.12 based distro kernel), I see fairly
> stable xfs_inode slab usage throughout the removal. It mostly hovers
> from 150k-300k objects, sometimes spikes into the 500k-600k range, and
> the test completes as expected.
Hm. Without any patches applied, I loaded up a filesystem with 10
million inodes in a VM with 512M of RAM and ran rm -rf /mnt/*. The
xfs_inode slab usage hovered around 20-30k objects, and occasionally
spiked to about 80k. The inode count would decrease at about 7k inodes
per second...
> I see much different behavior from the same test on for-next plus this
> patch. xfs_inode slab usage continuously increases from the onset,
> eventually gets up to over 175m objects (~295GB worth of inodes), and
> the oom killer takes everything down. This occurs after only about 10%
> of the total consumed space has been removed.
...but as soon as I pushed the patch stack up to this particular patch,
the slab cache usage went straight up to about 200k objects and the VM
OOMed shortly thereafter. Until that happened, I could see the icount
going down about about 12k inodes per second, but that doesn't do users
much good. I observed that if I made xfs_inode_mark_reclaimable call
flush_work on the inodegc work, the behaviors went back to the steady
but slow deletion, though the VM still OOMed.
I bet I'm messing up the shrinker accounting here, because an inode
that's queued for inactivation is no longer on the vfs s_inode_lru and
it's not tagged for reclaim, so a call to super_cache_count will not
know about the inode, decide there isn't any reclaim work to do for this
XFS and trip the OOM killer.
The other thing we might want to do is throttle the frontend so it won't
get too far ahead of the background workers. If xfs_reclaim_inodes_nr
requeues the worker immediately and xfs_inode_mark_reclaimable calls
flush_work on the background worker, the OOMs seem to go away.
> I've not dug into the behavior enough to see where things are getting
> backed up during this test, and I'm sure that this is not an
> ideal/typical configuration (and increased slab usage is probably to be
> expected given the nature of the change), but it does seem like we might
> need some kind of adjustment in place to manage this kind of breakdown
> scenario.
When I pushed the stack up to the parallel inactivation series, the
behavior changes again -- this time it takes a lot longer to push the VM
to OOM. I think what's going on is that once I switch this to per-AG
workers, the workers can keep up with the frontend that's inactivating
the inodes. At least until those workers start competing with the
unlink() activity for log space, and then they fall behind and the box
OOMs again. Unfortunately, the changes I made above didn't prevent the
OOM.
> > The VFS still has this (erroneous) notion that calling ->destroy_inode
> > will directly reclaim inodes from XFS, but the only difference from
> > today's code is that it can take longer for inodes to get to that state.
> > To fix that, I've also thought about making xfs_fs_free_cached_objects
> > requeue the inodegc worker immediately (but without waiting for it), but
> > so far I haven't needed to do that to avoid trouble.
> >
> > > > - if (!XFS_FORCED_SHUTDOWN(mp) && ip->i_delayed_blks) {
> > > > - xfs_check_delalloc(ip, XFS_DATA_FORK);
> > > > - xfs_check_delalloc(ip, XFS_COW_FORK);
> > > > - ASSERT(0);
> > > > + if (!XFS_FORCED_SHUTDOWN(mp) && ip->i_delayed_blks) {
> > > > + xfs_check_delalloc(ip, XFS_DATA_FORK);
> > > > + xfs_check_delalloc(ip, XFS_COW_FORK);
> > > > + ASSERT(0);
> > > > + }
> > > > }
> > > >
> > > > XFS_STATS_INC(mp, vn_reclaim);
> > > >
> > > > /*
> > > > - * We should never get here with one of the reclaim flags already set.
> > > > + * We should never get here with any of the reclaim flags already set.
> > > > */
> > > > - ASSERT_ALWAYS(!xfs_iflags_test(ip, XFS_IRECLAIMABLE));
> > > > - ASSERT_ALWAYS(!xfs_iflags_test(ip, XFS_IRECLAIM));
> > > > + ASSERT_ALWAYS(!xfs_iflags_test(ip, XFS_ALL_IRECLAIM_FLAGS));
> > > >
> > > > /*
> > > > * We always use background reclaim here because even if the inode is
> > > ...
> > > > @@ -889,22 +936,32 @@ xfs_dqrele_igrab(
> > > >
> > > > /*
> > > > * Skip inodes that are anywhere in the reclaim machinery because we
> > > > - * drop dquots before tagging an inode for reclamation.
> > > > + * drop dquots before tagging an inode for reclamation. If the inode
> > > > + * is being inactivated, skip it because inactivation will drop the
> > > > + * dquots for us.
> > > > */
> > > > - if (ip->i_flags & (XFS_IRECLAIM | XFS_IRECLAIMABLE))
> > > > + if (ip->i_flags & (XFS_IRECLAIM | XFS_IRECLAIMABLE | XFS_INACTIVATING))
> > > > goto out_unlock;
> > >
> > > I was a little curious why we'd skip reclaimable inodes here but not
> > > need_inactive..
> > >
> > > >
> > > > /*
> > > > - * The inode looks alive; try to grab a VFS reference so that it won't
> > > > - * get destroyed. If we got the reference, return true to say that
> > > > - * we grabbed the inode.
> > > > + * If the inode is queued but not undergoing inactivation, set the
> > > > + * inactivating flag so everyone will leave it alone and return true
> > > > + * to say that we are taking ownership of it.
> > > > + *
> > > > + * Otherwise, the inode looks alive; try to grab a VFS reference so
> > > > + * that it won't get destroyed. If we got the reference, return true
> > > > + * to say that we grabbed the inode.
> > > > *
> > > > * If we can't get the reference, then we know the inode had its VFS
> > > > * state torn down and hasn't yet entered the reclaim machinery. Since
> > > > * we also know that dquots are detached from an inode before it enters
> > > > * reclaim, we can skip the inode.
> > > > */
> > > > - ret = igrab(VFS_I(ip)) != NULL;
> > > > + ret = true;
> > > > + if (ip->i_flags & XFS_NEED_INACTIVE)
> > > > + ip->i_flags |= XFS_INACTIVATING;
> > > > + else if (!igrab(VFS_I(ip)))
> > > > + ret = false;
> > >
> > > ... but I guess we process the latter (in that we release the dquots and
> > > revert the just added inactivating state). Is this a functional
> > > requirement or considered an optimization? FWIW, it would be helpful if
> > > the comment more explained why we did this as such as opposed to just
> > > reflecting what the code already does.
> >
> > It's an optimization to avoid stalling quotaoff on NEEDS_INACTIVE
> > inodes. This means that quotaoff can now cause the background inodegc
> > worker to have to loop again, but seeing as quotaoff can pin the log
> > tail, I decided that was a reasonable tradeoff.
> > How does this rewording sound to you?
> >
>
> Ok. I guess it would be nice to see some separation between core
> functionality and this kind of optimization. It would make things more
> clear to review for one, but also provides a trail of documentation and
> intent if something like the approach to handling dquots causes some
> problem in the future and the poor developer who might have to analyze
> it otherwise has to navigate how this fits into the broader functional
> rework. But I know people tend to bristle lately over factoring and
> readability feedback and this is v7, so that's just my .02. That aside,
> the comment below looks fine to me.
I don't mind breaking up a large patch into smaller more targeted ones,
because that's directly relevant to the code changes being proposed. It
also isn't a terribly large change. It's more things like being told to
fix or remove quotaoff (or that whole mess over building latency qos
functions to decide when to defer ioend clear_pagewriteback to a
workqueue) that bother me.
> > /*
> > * If the inode is queued but not currently undergoing
> > * inactivation, we want to slip in to drop the dquots ASAP
> > * because quotaoff can pin the log tail and cause log livelock.
> > * Avoiding that is worth potentially forcing the inodegc worker
> > * to make another pass. Set INACTIVATING to prevent inodegc
> > * and iget from touching the inode.
> > *
> > * Otherwise, the inode looks alive; try to grab a VFS reference
> > * so that it won't get destroyed. If we got the reference,
> > * return true to say that we grabbed the inode.
> > *
> > * If we can't get the reference, then we know the inode had its
> > * VFS state torn down and hasn't yet entered the reclaim
> > * machinery. Since we also know that dquots are detached from
> > * an inode before it enters reclaim, we can skip the inode.
> > */
> >
> > > >
> > > > out_unlock:
> > > > spin_unlock(&ip->i_flags_lock);
> > > > @@ -917,6 +974,8 @@ xfs_dqrele_inode(
> > > > struct xfs_inode *ip,
> > > > struct xfs_icwalk *icw)
> > > > {
> > > > + bool live_inode;
> > > > +
> > > > if (xfs_iflags_test(ip, XFS_INEW))
> > > > xfs_inew_wait(ip);
> > > >
> > > > @@ -934,7 +993,19 @@ xfs_dqrele_inode(
> > > > ip->i_pdquot = NULL;
> > > > }
> > > > xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > > > - xfs_irele(ip);
> > > > +
> > > > + /*
> > > > + * If we set INACTIVATING earlier to prevent this inode from being
> > > > + * touched, clear that state to let the inodegc claim it. Otherwise,
> > > > + * it's a live inode and we need to release it.
> > > > + */
> > > > + spin_lock(&ip->i_flags_lock);
> > > > + live_inode = !(ip->i_flags & XFS_INACTIVATING);
> > > > + ip->i_flags &= ~XFS_INACTIVATING;
> > > > + spin_unlock(&ip->i_flags_lock);
> > > > +
> > > > + if (live_inode)
> > > > + xfs_irele(ip);
> > > > }
> > > >
> > > > /*
> > > ...
> > > > +/*
> > > > + * Force all currently queued inode inactivation work to run immediately, and
> > > > + * wait for the work to finish.
> > > > + */
> > > > +void
> > > > +xfs_inodegc_flush(
> > > > + struct xfs_mount *mp)
> > > > +{
> > > > + trace_xfs_inodegc_flush(mp, 0, _RET_IP_);
> > > > + flush_delayed_work(&mp->m_inodegc_work);
> > > > +}
> > > > +
> > > > +/* Disable the inode inactivation background worker and wait for it to stop. */
> > > > +void
> > > > +xfs_inodegc_stop(
> > > > + struct xfs_mount *mp)
> > > > +{
> > > > + if (!test_and_clear_bit(XFS_OPFLAG_INODEGC_RUNNING_BIT, &mp->m_opflags))
> > > > + return;
> > > > +
> > > > + cancel_delayed_work_sync(&mp->m_inodegc_work);
> > > > + trace_xfs_inodegc_stop(mp, 0, _RET_IP_);
> > > > +}
> > > > +
> > > > +/*
> > > > + * Enable the inode inactivation background worker and schedule deferred inode
> > > > + * inactivation work if there is any.
> > > > + */
> > > > +void
> > > > +xfs_inodegc_start(
> > > > + struct xfs_mount *mp)
> > > > +{
> > > > + if (test_and_set_bit(XFS_OPFLAG_INODEGC_RUNNING_BIT, &mp->m_opflags))
> > > > + return;
> > > > +
> > > > + trace_xfs_inodegc_start(mp, 0, _RET_IP_);
> > > > + xfs_inodegc_queue(mp);
> > > > +}
> > > > +
> > >
> > > This seems like a decent amount of boilerplate. Is all of this really
> > > necessary and/or is it worth it just for the added tracepoints?
> >
> > I've gotten a lot of use out of the tracepoints to debug all sorts of
> > weird mis-interactions between freeze, remounts, and the inodegc worker.
> >
>
> Is that due to heavy development activity or something you realistically
> expect to see used in production? I've nothing against tracepoints, but
> I almost always have custom tracepoints during development that are
> temporarily useful but eventually removed, and do question the tradeoff
> of creating a bunch of wrapper code just for the tracepoints.
I've found myself repeatedly adding them to debug some unexpected
behavior, then removing them to polish things before publishing, and
finally got fed up with doing this over and over and over.
> > > ISTM
> > > that the start/stop thing is already technically racy wrt to queue
> > > activity and the bit check in the worker might be sufficient to avoid
> > > transaction/freeze deadlocks or whatever the original intent was. Hm?
> >
> > The bitflag isn't racy, though why isn't obvious. There are seven
> > places where we change the INODEGC_RUNNING state:
> >
> > - For freeze and ro remount, the VFS will take s_umount before calling
> > into XFS, and XFS then calls inodegc_flush to complete all the queued
> > work before changing the bit state.
> >
> > During the v6 review Dave wondered if we even needed the INODEGC
> > RUNNING flag, and I concluded that we need it for freeze, since we
> > allow inodes to sleep under xfs_inactive() while trying to allocate a
> > transaction. Under the new scheme, inodes can be queued for
> > inactivation on a frozen fs, but we need xfs_inodegc_queue to know not
> > to kick the worker without having to mess around with VFS locks to
> > check the freeze state.
> >
>
> Not sure I'm clear on what you describe above, but I'm not really
> concerned about the existence of the state bit as much. The above refers
> to xfs_inodegc_queue(), which already checks the bit, whereas my comment
> was just more around the need for the xfs_inodegc_start/stop() wrappers.
Now I'm completely confused about what you're asking about here. Are
you suggesting that I could set and clear the INODEGC RUNNING bit
directly from xfs_fs_freeze and xfs_fs_unfreeze, change
xfs_inodegc_worker to bail out if INODEGC_RUNNING is set, and then
replace all the inodegc_stop/start calls with direct calls to
cancel_work_delayed_sync/xfs_inodegc_queue?
--D
>
> Brian
>
> > - For thaw and rw remount, the VFS takes s_umount and calls XFS, which
> > re-enables the worker.
> >
> > - Mount and unmount run in isolation (since we can't have any open files
> > on the filesystem) so there won't be any other threads trying to call
> > into the filesystem.
> >
> > - I think xchk_{start,stop}_reaping is racy though. That code probably
> > has to sb_start_write to prevent freeze or ro remount from messing
> > with the INODEGC_RUNNING state. That would still leave a hole if
> > fscounters scrub races with a rw remount and immediately files start
> > getting deleted, but maybe that's just not worth worrying about.
> > Scrub will try again a few times, and in djwong-dev I solve that by
> > freezing the filesystem if the unlocked fscounters scrub fails.
> >
> > --D
> >
> > >
> > > Brian
> > >
> > > > /* XFS Inode Cache Walking Code */
> > > >
> > > > /*
> > > > @@ -1708,6 +1979,8 @@ xfs_icwalk_igrab(
> > > > return xfs_blockgc_igrab(ip);
> > > > case XFS_ICWALK_RECLAIM:
> > > > return xfs_reclaim_igrab(ip, icw);
> > > > + case XFS_ICWALK_INODEGC:
> > > > + return xfs_inodegc_igrab(ip);
> > > > default:
> > > > return false;
> > > > }
> > > > @@ -1736,6 +2009,9 @@ xfs_icwalk_process_inode(
> > > > case XFS_ICWALK_RECLAIM:
> > > > xfs_reclaim_inode(ip, pag);
> > > > break;
> > > > + case XFS_ICWALK_INODEGC:
> > > > + xfs_inodegc_inactivate(ip, pag, icw);
> > > > + break;
> > > > }
> > > > return error;
> > > > }
> > > > diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
> > > > index 00dc98a92835..840eac06a71b 100644
> > > > --- a/fs/xfs/xfs_icache.h
> > > > +++ b/fs/xfs/xfs_icache.h
> > > > @@ -80,4 +80,9 @@ int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
> > > > void xfs_blockgc_stop(struct xfs_mount *mp);
> > > > void xfs_blockgc_start(struct xfs_mount *mp);
> > > >
> > > > +void xfs_inodegc_worker(struct work_struct *work);
> > > > +void xfs_inodegc_flush(struct xfs_mount *mp);
> > > > +void xfs_inodegc_stop(struct xfs_mount *mp);
> > > > +void xfs_inodegc_start(struct xfs_mount *mp);
> > > > +
> > > > #endif
> > > > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> > > > index e3137bbc7b14..fa5be0d071ad 100644
> > > > --- a/fs/xfs/xfs_inode.h
> > > > +++ b/fs/xfs/xfs_inode.h
> > > > @@ -240,6 +240,7 @@ static inline bool xfs_inode_has_bigtime(struct xfs_inode *ip)
> > > > #define __XFS_IPINNED_BIT 8 /* wakeup key for zero pin count */
> > > > #define XFS_IPINNED (1 << __XFS_IPINNED_BIT)
> > > > #define XFS_IEOFBLOCKS (1 << 9) /* has the preallocblocks tag set */
> > > > +#define XFS_NEED_INACTIVE (1 << 10) /* see XFS_INACTIVATING below */
> > > > /*
> > > > * If this unlinked inode is in the middle of recovery, don't let drop_inode
> > > > * truncate and free the inode. This can happen if we iget the inode during
> > > > @@ -248,6 +249,21 @@ static inline bool xfs_inode_has_bigtime(struct xfs_inode *ip)
> > > > #define XFS_IRECOVERY (1 << 11)
> > > > #define XFS_ICOWBLOCKS (1 << 12)/* has the cowblocks tag set */
> > > >
> > > > +/*
> > > > + * If we need to update on-disk metadata before this IRECLAIMABLE inode can be
> > > > + * freed, then NEED_INACTIVE will be set. Once we start the updates, the
> > > > + * INACTIVATING bit will be set to keep iget away from this inode. After the
> > > > + * inactivation completes, both flags will be cleared and the inode is a
> > > > + * plain old IRECLAIMABLE inode.
> > > > + */
> > > > +#define XFS_INACTIVATING (1 << 13)
> > > > +
> > > > +/* All inode state flags related to inode reclaim. */
> > > > +#define XFS_ALL_IRECLAIM_FLAGS (XFS_IRECLAIMABLE | \
> > > > + XFS_IRECLAIM | \
> > > > + XFS_NEED_INACTIVE | \
> > > > + XFS_INACTIVATING)
> > > > +
> > > > /*
> > > > * Per-lifetime flags need to be reset when re-using a reclaimable inode during
> > > > * inode lookup. This prevents unintended behaviour on the new inode from
> > > > @@ -255,7 +271,8 @@ static inline bool xfs_inode_has_bigtime(struct xfs_inode *ip)
> > > > */
> > > > #define XFS_IRECLAIM_RESET_FLAGS \
> > > > (XFS_IRECLAIMABLE | XFS_IRECLAIM | \
> > > > - XFS_IDIRTY_RELEASE | XFS_ITRUNCATED)
> > > > + XFS_IDIRTY_RELEASE | XFS_ITRUNCATED | XFS_NEED_INACTIVE | \
> > > > + XFS_INACTIVATING)
> > > >
> > > > /*
> > > > * Flags for inode locking.
> > > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > > > index 1227503d2246..9d8fc85bd28d 100644
> > > > --- a/fs/xfs/xfs_log_recover.c
> > > > +++ b/fs/xfs/xfs_log_recover.c
> > > > @@ -2784,6 +2784,13 @@ xlog_recover_process_iunlinks(
> > > > }
> > > > xfs_buf_rele(agibp);
> > > > }
> > > > +
> > > > + /*
> > > > + * Flush the pending unlinked inodes to ensure that the inactivations
> > > > + * are fully completed on disk and the incore inodes can be reclaimed
> > > > + * before we signal that recovery is complete.
> > > > + */
> > > > + xfs_inodegc_flush(mp);
> > > > }
> > > >
> > > > STATIC void
> > > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > > > index c3a96fb3ad80..ab65a14e51e6 100644
> > > > --- a/fs/xfs/xfs_mount.c
> > > > +++ b/fs/xfs/xfs_mount.c
> > > > @@ -514,7 +514,8 @@ xfs_check_summary_counts(
> > > > * Flush and reclaim dirty inodes in preparation for unmount. Inodes and
> > > > * internal inode structures can be sitting in the CIL and AIL at this point,
> > > > * so we need to unpin them, write them back and/or reclaim them before unmount
> > > > - * can proceed.
> > > > + * can proceed. In other words, callers are required to have inactivated all
> > > > + * inodes.
> > > > *
> > > > * An inode cluster that has been freed can have its buffer still pinned in
> > > > * memory because the transaction is still sitting in a iclog. The stale inodes
> > > > @@ -546,6 +547,7 @@ xfs_unmount_flush_inodes(
> > > > mp->m_flags |= XFS_MOUNT_UNMOUNTING;
> > > >
> > > > xfs_ail_push_all_sync(mp->m_ail);
> > > > + xfs_inodegc_stop(mp);
> > > > cancel_delayed_work_sync(&mp->m_reclaim_work);
> > > > xfs_reclaim_inodes(mp);
> > > > xfs_health_unmount(mp);
> > > > @@ -782,6 +784,9 @@ xfs_mountfs(
> > > > if (error)
> > > > goto out_log_dealloc;
> > > >
> > > > + /* Enable background inode inactivation workers. */
> > > > + xfs_inodegc_start(mp);
> > > > +
> > > > /*
> > > > * Get and sanity-check the root inode.
> > > > * Save the pointer to it in the mount structure.
> > > > @@ -936,6 +941,15 @@ xfs_mountfs(
> > > > xfs_irele(rip);
> > > > /* Clean out dquots that might be in memory after quotacheck. */
> > > > xfs_qm_unmount(mp);
> > > > +
> > > > + /*
> > > > + * Inactivate all inodes that might still be in memory after a log
> > > > + * intent recovery failure so that reclaim can free them. Metadata
> > > > + * inodes and the root directory shouldn't need inactivation, but the
> > > > + * mount failed for some reason, so pull down all the state and flee.
> > > > + */
> > > > + xfs_inodegc_flush(mp);
> > > > +
> > > > /*
> > > > * Flush all inode reclamation work and flush the log.
> > > > * We have to do this /after/ rtunmount and qm_unmount because those
> > > > @@ -983,6 +997,16 @@ xfs_unmountfs(
> > > > uint64_t resblks;
> > > > int error;
> > > >
> > > > + /*
> > > > + * Perform all on-disk metadata updates required to inactivate inodes
> > > > + * that the VFS evicted earlier in the unmount process. Freeing inodes
> > > > + * and discarding CoW fork preallocations can cause shape changes to
> > > > + * the free inode and refcount btrees, respectively, so we must finish
> > > > + * this before we discard the metadata space reservations. Metadata
> > > > + * inodes and the root directory do not require inactivation.
> > > > + */
> > > > + xfs_inodegc_flush(mp);
> > > > +
> > > > xfs_blockgc_stop(mp);
> > > > xfs_fs_unreserve_ag_blocks(mp);
> > > > xfs_qm_unmount_quotas(mp);
> > > > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > > > index c78b63fe779a..dc906b78e24c 100644
> > > > --- a/fs/xfs/xfs_mount.h
> > > > +++ b/fs/xfs/xfs_mount.h
> > > > @@ -154,6 +154,13 @@ typedef struct xfs_mount {
> > > > uint8_t m_rt_checked;
> > > > uint8_t m_rt_sick;
> > > >
> > > > + /*
> > > > + * This atomic bitset controls flags that alter the behavior of the
> > > > + * filesystem. Use only the atomic bit helper functions here; see
> > > > + * XFS_OPFLAG_* for information about the actual flags.
> > > > + */
> > > > + unsigned long m_opflags;
> > > > +
> > > > /*
> > > > * End of read-mostly variables. Frequently written variables and locks
> > > > * should be placed below this comment from now on. The first variable
> > > > @@ -184,6 +191,7 @@ typedef struct xfs_mount {
> > > > uint64_t m_resblks_avail;/* available reserved blocks */
> > > > uint64_t m_resblks_save; /* reserved blks @ remount,ro */
> > > > struct delayed_work m_reclaim_work; /* background inode reclaim */
> > > > + struct delayed_work m_inodegc_work; /* background inode inactive */
> > > > struct xfs_kobj m_kobj;
> > > > struct xfs_kobj m_error_kobj;
> > > > struct xfs_kobj m_error_meta_kobj;
> > > > @@ -258,6 +266,19 @@ typedef struct xfs_mount {
> > > > #define XFS_MOUNT_DAX_ALWAYS (1ULL << 26)
> > > > #define XFS_MOUNT_DAX_NEVER (1ULL << 27)
> > > >
> > > > +/*
> > > > + * Operation flags -- each entry here is a bit index into m_opflags and is
> > > > + * not itself a flag value. Use the atomic bit functions to access.
> > > > + */
> > > > +enum xfs_opflag_bits {
> > > > + /*
> > > > + * If set, background inactivation worker threads will be scheduled to
> > > > + * process queued inodegc work. If not, queued inodes remain in memory
> > > > + * waiting to be processed.
> > > > + */
> > > > + XFS_OPFLAG_INODEGC_RUNNING_BIT = 0,
> > > > +};
> > > > +
> > > > /*
> > > > * Max and min values for mount-option defined I/O
> > > > * preallocation sizes.
> > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > index dd1ee333dcb3..0b01d9499395 100644
> > > > --- a/fs/xfs/xfs_super.c
> > > > +++ b/fs/xfs/xfs_super.c
> > > > @@ -714,6 +714,8 @@ xfs_fs_sync_fs(
> > > > {
> > > > struct xfs_mount *mp = XFS_M(sb);
> > > >
> > > > + trace_xfs_fs_sync_fs(mp, wait, _RET_IP_);
> > > > +
> > > > /*
> > > > * Doing anything during the async pass would be counterproductive.
> > > > */
> > > > @@ -730,6 +732,25 @@ xfs_fs_sync_fs(
> > > > flush_delayed_work(&mp->m_log->l_work);
> > > > }
> > > >
> > > > + /*
> > > > + * Flush all deferred inode inactivation work so that the free space
> > > > + * counters will reflect recent deletions. Do not force the log again
> > > > + * because log recovery can restart the inactivation from the info that
> > > > + * we just wrote into the ondisk log.
> > > > + *
> > > > + * For regular operation this isn't strictly necessary since we aren't
> > > > + * required to guarantee that unlinking frees space immediately, but
> > > > + * that is how XFS historically behaved.
> > > > + *
> > > > + * If, however, the filesystem is at FREEZE_PAGEFAULTS, this is our
> > > > + * last chance to complete the inactivation work before the filesystem
> > > > + * freezes and the log is quiesced. The background worker will not
> > > > + * activate again until the fs is thawed because the VFS won't evict
> > > > + * any more inodes until freeze_super drops s_umount and we disable the
> > > > + * worker in xfs_fs_freeze.
> > > > + */
> > > > + xfs_inodegc_flush(mp);
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > @@ -844,6 +865,17 @@ xfs_fs_freeze(
> > > > */
> > > > flags = memalloc_nofs_save();
> > > > xfs_blockgc_stop(mp);
> > > > +
> > > > + /*
> > > > + * Stop the inodegc background worker. freeze_super already flushed
> > > > + * all pending inodegc work when it sync'd the filesystem after setting
> > > > + * SB_FREEZE_PAGEFAULTS, and it holds s_umount, so we know that inodes
> > > > + * cannot enter xfs_fs_destroy_inode until the freeze is complete.
> > > > + * If the filesystem is read-write, inactivated inodes will queue but
> > > > + * the worker will not run until the filesystem thaws or unmounts.
> > > > + */
> > > > + xfs_inodegc_stop(mp);
> > > > +
> > > > xfs_save_resvblks(mp);
> > > > ret = xfs_log_quiesce(mp);
> > > > memalloc_nofs_restore(flags);
> > > > @@ -859,6 +891,14 @@ xfs_fs_unfreeze(
> > > > xfs_restore_resvblks(mp);
> > > > xfs_log_work_queue(mp);
> > > > xfs_blockgc_start(mp);
> > > > +
> > > > + /*
> > > > + * Don't reactivate the inodegc worker on a readonly filesystem because
> > > > + * inodes are sent directly to reclaim.
> > > > + */
> > > > + if (!(mp->m_flags & XFS_MOUNT_RDONLY))
> > > > + xfs_inodegc_start(mp);
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > @@ -1665,6 +1705,9 @@ xfs_remount_rw(
> > > > if (error && error != -ENOSPC)
> > > > return error;
> > > >
> > > > + /* Re-enable the background inode inactivation worker. */
> > > > + xfs_inodegc_start(mp);
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > @@ -1687,6 +1730,15 @@ xfs_remount_ro(
> > > > return error;
> > > > }
> > > >
> > > > + /*
> > > > + * Stop the inodegc background worker. xfs_fs_reconfigure already
> > > > + * flushed all pending inodegc work when it sync'd the filesystem.
> > > > + * The VFS holds s_umount, so we know that inodes cannot enter
> > > > + * xfs_fs_destroy_inode during a remount operation. In readonly mode
> > > > + * we send inodes straight to reclaim, so no inodes will be queued.
> > > > + */
> > > > + xfs_inodegc_stop(mp);
> > > > +
> > > > /* Free the per-AG metadata reservation pool. */
> > > > error = xfs_fs_unreserve_ag_blocks(mp);
> > > > if (error) {
> > > > @@ -1810,6 +1862,7 @@ static int xfs_init_fs_context(
> > > > mutex_init(&mp->m_growlock);
> > > > INIT_WORK(&mp->m_flush_inodes_work, xfs_flush_inodes_worker);
> > > > INIT_DELAYED_WORK(&mp->m_reclaim_work, xfs_reclaim_worker);
> > > > + INIT_DELAYED_WORK(&mp->m_inodegc_work, xfs_inodegc_worker);
> > > > mp->m_kobj.kobject.kset = xfs_kset;
> > > > /*
> > > > * We don't create the finobt per-ag space reservation until after log
> > > > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> > > > index d0b4799ad1e6..ca9bfbd28886 100644
> > > > --- a/fs/xfs/xfs_trace.h
> > > > +++ b/fs/xfs/xfs_trace.h
> > > > @@ -156,6 +156,45 @@ DEFINE_PERAG_REF_EVENT(xfs_perag_put);
> > > > DEFINE_PERAG_REF_EVENT(xfs_perag_set_inode_tag);
> > > > DEFINE_PERAG_REF_EVENT(xfs_perag_clear_inode_tag);
> > > >
> > > > +DECLARE_EVENT_CLASS(xfs_fs_class,
> > > > + TP_PROTO(struct xfs_mount *mp, int data, unsigned long caller_ip),
> > > > + TP_ARGS(mp, data, caller_ip),
> > > > + TP_STRUCT__entry(
> > > > + __field(dev_t, dev)
> > > > + __field(unsigned long long, mflags)
> > > > + __field(unsigned long, opflags)
> > > > + __field(unsigned long, sbflags)
> > > > + __field(int, data)
> > > > + __field(unsigned long, caller_ip)
> > > > + ),
> > > > + TP_fast_assign(
> > > > + __entry->dev = mp->m_super->s_dev;
> > > > + __entry->mflags = mp->m_flags;
> > > > + __entry->opflags = mp->m_opflags;
> > > > + __entry->sbflags = mp->m_super->s_flags;
> > > > + __entry->data = data;
> > > > + __entry->caller_ip = caller_ip;
> > > > + ),
> > > > + TP_printk("dev %d:%d flags 0x%llx opflags 0x%lx sflags 0x%lx data %d caller %pS",
> > > > + MAJOR(__entry->dev), MINOR(__entry->dev),
> > > > + __entry->mflags,
> > > > + __entry->opflags,
> > > > + __entry->sbflags,
> > > > + __entry->data,
> > > > + (char *)__entry->caller_ip)
> > > > +);
> > > > +
> > > > +#define DEFINE_FS_EVENT(name) \
> > > > +DEFINE_EVENT(xfs_fs_class, name, \
> > > > + TP_PROTO(struct xfs_mount *mp, int data, unsigned long caller_ip), \
> > > > + TP_ARGS(mp, data, caller_ip))
> > > > +DEFINE_FS_EVENT(xfs_inodegc_flush);
> > > > +DEFINE_FS_EVENT(xfs_inodegc_start);
> > > > +DEFINE_FS_EVENT(xfs_inodegc_stop);
> > > > +DEFINE_FS_EVENT(xfs_inodegc_queue);
> > > > +DEFINE_FS_EVENT(xfs_inodegc_worker);
> > > > +DEFINE_FS_EVENT(xfs_fs_sync_fs);
> > > > +
> > > > DECLARE_EVENT_CLASS(xfs_ag_class,
> > > > TP_PROTO(struct xfs_mount *mp, xfs_agnumber_t agno),
> > > > TP_ARGS(mp, agno),
> > > > @@ -615,14 +654,17 @@ DECLARE_EVENT_CLASS(xfs_inode_class,
> > > > TP_STRUCT__entry(
> > > > __field(dev_t, dev)
> > > > __field(xfs_ino_t, ino)
> > > > + __field(unsigned long, iflags)
> > > > ),
> > > > TP_fast_assign(
> > > > __entry->dev = VFS_I(ip)->i_sb->s_dev;
> > > > __entry->ino = ip->i_ino;
> > > > + __entry->iflags = ip->i_flags;
> > > > ),
> > > > - TP_printk("dev %d:%d ino 0x%llx",
> > > > + TP_printk("dev %d:%d ino 0x%llx iflags 0x%lx",
> > > > MAJOR(__entry->dev), MINOR(__entry->dev),
> > > > - __entry->ino)
> > > > + __entry->ino,
> > > > + __entry->iflags)
> > > > )
> > > >
> > > > #define DEFINE_INODE_EVENT(name) \
> > > > @@ -666,6 +708,10 @@ DEFINE_INODE_EVENT(xfs_inode_free_eofblocks_invalid);
> > > > DEFINE_INODE_EVENT(xfs_inode_set_cowblocks_tag);
> > > > DEFINE_INODE_EVENT(xfs_inode_clear_cowblocks_tag);
> > > > DEFINE_INODE_EVENT(xfs_inode_free_cowblocks_invalid);
> > > > +DEFINE_INODE_EVENT(xfs_inode_set_reclaimable);
> > > > +DEFINE_INODE_EVENT(xfs_inode_reclaiming);
> > > > +DEFINE_INODE_EVENT(xfs_inode_set_need_inactive);
> > > > +DEFINE_INODE_EVENT(xfs_inode_inactivating);
> > > >
> > > > /*
> > > > * ftrace's __print_symbolic requires that all enum values be wrapped in the
> > > >
> > >
> >
>
