On Tue, May 24, 2022 at 7:14 PM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxx> 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);
> >
> > 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 sti
> running, right? I think that's reasonable behavior for a flush
> function, and shouldn't cause any weird interactions.
>
Ah, can xfs_is_inodegc_enabled() be changed in runtime?
Sorry for being thick.
I did not understand why flush should be done in this case and
push should not, but if you say its ok I'll take your word for it.
Thanks,
Amir.
