Re: [PATCH 2/2] xfs: introduce xfs_inodegc_push()

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, May 24, 2022 at 01:47:36PM +0300, Amir Goldstein wrote:
> On Tue, May 24, 2022 at 1:37 PM Dave Chinner <david@fromorbit.com> wrote:
> >
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > 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@onthe.net.au>
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  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);
> 
> Unintentional(?) change of behavior:
> trace_xfs_inodegc_flush() will be called in
> (!xfs_is_inodegc_enabled(mp)) case.

At worst we end up waiting for any inodegc workers that are still
running, right?  I think that's reasonable behavior for a flush
function, and shouldn't cause any weird interactions.

> I also wonder if trace_xfs_inodegc_flush()
> should not be before trace_xfs_inodegc_push() in this flow,
> but this is just a matter of tracing conventions and you should
> know best how it will be convenient for xfs developers to be
> reading the trace events stream.

Why?  _push has its own tracepoint which we can use to tell if inodegc
was enabled at _flush time.

--D

> Thanks,
> Amir.