On Tue, May 24, 2022 at 09:17:25AM -0700, Darrick J. Wong wrote:
> On Tue, May 24, 2022 at 04:38:02PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > The current blocking mechanism for pushing the inodegc queue out to
> > disk can result in systems becoming unusable when there is a long
> > running inodegc operation. This is because the statfs()
> > implementation currently issues a blocking flush of the inodegc
> > queue and a significant number of common system utilities will call
> > statfs() to discover something about the underlying filesystem.
> >
> > This can result in userspace operations getting stuck on inodegc
> > progress, and when trying to remove a heavily reflinked file on slow
> > storage with a full journal, this can result in delays measuring in
> > hours.
> >
> > Avoid this problem by adding "push" function that expedites the
> > flushing of the inodegc queue, but doesn't wait for it to complete.
> >
> > Convert xfs_fs_statfs() to use this mechanism so it doesn't block
> > but it does ensure that queued operations are expedited.
> >
> > Fixes: ab23a7768739 ("xfs: per-cpu deferred inode inactivation queues")
> > Reported-by: Chris Dunlop <chris@xxxxxxxxxxxx>
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_icache.c | 20 +++++++++++++++-----
> > fs/xfs/xfs_icache.h | 1 +
> > fs/xfs/xfs_super.c | 7 +++++--
> > fs/xfs/xfs_trace.h | 1 +
> > 4 files changed, 22 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index 786702273621..2609825d53ee 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -1862,19 +1862,29 @@ xfs_inodegc_worker(
> > }
> >
> > /*
> > - * Force all currently queued inode inactivation work to run immediately and
> > - * wait for the work to finish.
> > + * Expedite all pending inodegc work to run immediately. This does not wait for
> > + * completion of the work.
> > */
> > void
> > -xfs_inodegc_flush(
> > +xfs_inodegc_push(
> > struct xfs_mount *mp)
> > {
> > if (!xfs_is_inodegc_enabled(mp))
> > return;
> > + trace_xfs_inodegc_push(mp, __return_address);
> > + xfs_inodegc_queue_all(mp);
> > +}
> >
> > +/*
> > + * 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)
> > +{
> > + xfs_inodegc_push(mp);
> > trace_xfs_inodegc_flush(mp, __return_address);
> > -
> > - xfs_inodegc_queue_all(mp);
> > flush_workqueue(mp->m_inodegc_wq);
> > }
> >
> > diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
> > index 2e4cfddf8b8e..6cd180721659 100644
> > --- a/fs/xfs/xfs_icache.h
> > +++ b/fs/xfs/xfs_icache.h
> > @@ -76,6 +76,7 @@ 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_push(struct xfs_mount *mp);
> > void xfs_inodegc_flush(struct xfs_mount *mp);
> > void xfs_inodegc_stop(struct xfs_mount *mp);
> > void xfs_inodegc_start(struct xfs_mount *mp);
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 62f6b97355a2..e14101813851 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -796,8 +796,11 @@ xfs_fs_statfs(
> > xfs_extlen_t lsize;
> > int64_t ffree;
> >
> > - /* Wait for whatever inactivations are in progress. */
> > - xfs_inodegc_flush(mp);
> > + /*
> > + * Expedite background inodegc but don't wait. We do not want to block
> > + * here waiting hours for a billion extent file to be truncated.
> > + */
> > + xfs_inodegc_push(mp);
>
> I think the same "don't wait forever for inodegc during a stats call"
> logic applies to the _inodegc_flush calls in xfs_qm_scall_getquota*,
> wouldn't it?
I guess so - I haven't tried to identify all the places we need to
do non-blocking pushes - I figured we'd probably be playing
whack-a-mole with this for a while as new and more "interesting"
cases of blocking are discovered.
I'll change it given that it is a likely source of blocking user
reporting operations just like df->statfs() is.
> The logic in this patch looks solid otherwise.
Thanks!
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
