Re: [PATCH 1/2] xfs: bound maximum wait time for inodegc work

[Brian Foster <bfoster@redhat.com>]

On Thu, Jun 16, 2022 at 08:04:15AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Currently inodegc work can sit queued on the per-cpu queue until
> the workqueue is either flushed of the queue reaches a depth that
> triggers work queuing (and later throttling). This means that we
> could queue work that waits for a long time for some other event to
> trigger flushing.
> 
> Hence instead of just queueing work at a specific depth, use a
> delayed work that queues the work at a bound time. We can still
> schedule the work immediately at a given depth, but we no long need
> to worry about leaving a number of items on the list that won't get
> processed until external events prevail.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_icache.c | 36 ++++++++++++++++++++++--------------
>  fs/xfs/xfs_mount.h  |  2 +-
>  fs/xfs/xfs_super.c  |  2 +-
>  3 files changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 374b3bafaeb0..46b30ecf498c 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
...
> @@ -2176,7 +2184,7 @@ xfs_inodegc_shrinker_scan(
>  			unsigned int	h = READ_ONCE(gc->shrinker_hits);
>  
>  			WRITE_ONCE(gc->shrinker_hits, h + 1);
> -			queue_work_on(cpu, mp->m_inodegc_wq, &gc->work);
> +			mod_delayed_work_on(cpu, mp->m_inodegc_wq, &gc->work, 0);
>  			no_items = false;
>  		}

This all seems reasonable to me, but is there much practical benefit to
shrinker infra/feedback just to expedite a delayed work item by one
jiffy? Maybe there's a use case to continue to trigger throttling..? If
so, it looks like decent enough overhead to cycle through every cpu in
both callbacks that it might be worth spelling out more clearly in the
top-level comment.

Brian

>  	}
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index ba5d42abf66e..d2eaebd85abf 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -61,7 +61,7 @@ struct xfs_error_cfg {
>   */
>  struct xfs_inodegc {
>  	struct llist_head	list;
> -	struct work_struct	work;
> +	struct delayed_work	work;
>  
>  	/* approximate count of inodes in the list */
>  	unsigned int		items;
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 48a7239ed1b2..651ae75a7e23 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1075,7 +1075,7 @@ xfs_inodegc_init_percpu(
>  		gc = per_cpu_ptr(mp->m_inodegc, cpu);
>  		init_llist_head(&gc->list);
>  		gc->items = 0;
> -		INIT_WORK(&gc->work, xfs_inodegc_worker);
> +		INIT_DELAYED_WORK(&gc->work, xfs_inodegc_worker);
>  	}
>  	return 0;
>  }
> -- 
> 2.35.1
>