Re: [PATCH 09/10] xfs: poll waiting for quotacheck

[Brian Foster <bfoster@redhat.com>]

On Tue, Jun 04, 2019 at 02:50:27PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create a pwork destroy function that uses polling instead of
> uninterruptible sleep to wait for work items to finish so that we can
> touch the softlockup watchdog.  IOWs, gross hack.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Seems reasonable given the quirky circumstances of quotacheck. Just a
couple nits..

>  fs/xfs/xfs_iwalk.c |    3 +++
>  fs/xfs/xfs_iwalk.h |    3 ++-
>  fs/xfs/xfs_pwork.c |   21 +++++++++++++++++++++
>  fs/xfs/xfs_pwork.h |    2 ++
>  fs/xfs/xfs_qm.c    |    2 +-
>  5 files changed, 29 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c
> index 71ee1628aa70..c4a9c4c246b7 100644
> --- a/fs/xfs/xfs_iwalk.c
> +++ b/fs/xfs/xfs_iwalk.c
> @@ -526,6 +526,7 @@ xfs_iwalk_threaded(
>  	xfs_ino_t		startino,
>  	xfs_iwalk_fn		iwalk_fn,
>  	unsigned int		max_prefetch,
> +	bool			polled,
>  	void			*data)
>  {
>  	struct xfs_pwork_ctl	pctl;
> @@ -556,5 +557,7 @@ xfs_iwalk_threaded(
>  		startino = XFS_AGINO_TO_INO(mp, agno + 1, 0);
>  	}
>  
> +	if (polled)
> +		return xfs_pwork_destroy_poll(&pctl);
>  	return xfs_pwork_destroy(&pctl);

Rather than have duplicate destruction paths, could we rework
xfs_pwork_destroy_poll() to something like xfs_pwork_poll() that just
does the polling and returns, then the caller can fall back into the
current xfs_pwork_destroy()? I.e., this ends up looking like:

	...
	/* poll to keep soft lockup watchdog quiet */
	if (polled)
		xfs_pwork_poll(&pctl);
	return xfs_pwork_destroy(&pctl);

>  }
...
> diff --git a/fs/xfs/xfs_pwork.c b/fs/xfs/xfs_pwork.c
> index 19605a3a2482..3b885e0b52ac 100644
> --- a/fs/xfs/xfs_pwork.c
> +++ b/fs/xfs/xfs_pwork.c
...
> @@ -97,6 +101,23 @@ xfs_pwork_destroy(
>  	return pctl->error;
>  }
>  
> +/*
> + * Wait for the work to finish and tear down the control structure.
> + * Continually poll completion status and touch the soft lockup watchdog.
> + * This is for things like mount that hold locks.
> + */
> +int
> +xfs_pwork_destroy_poll(
> +	struct xfs_pwork_ctl	*pctl)
> +{
> +	while (atomic_read(&pctl->nr_work) > 0) {
> +		msleep(1);
> +		touch_softlockup_watchdog();
> +	}

Any idea what the typical watchdog timeout is? I'm curious where the 1ms
comes from and whether we could get away with anything larger. I realize
that might introduce mount latency with the current sleep based
implementation, but we could also consider a waitqueue and using
something like wait_event_timeout() to schedule out for longer time
periods and still wake up immediately when the count drops to 0.

Brian

> +
> +	return xfs_pwork_destroy(pctl);
> +}
> +
>  /*
>   * Return the amount of parallelism that the data device can handle, or 0 for
>   * no limit.
> diff --git a/fs/xfs/xfs_pwork.h b/fs/xfs/xfs_pwork.h
> index e0c1354a2d8c..08da723a8dc9 100644
> --- a/fs/xfs/xfs_pwork.h
> +++ b/fs/xfs/xfs_pwork.h
> @@ -18,6 +18,7 @@ struct xfs_pwork_ctl {
>  	struct workqueue_struct	*wq;
>  	struct xfs_mount	*mp;
>  	xfs_pwork_work_fn	work_fn;
> +	atomic_t		nr_work;
>  	int			error;
>  };
>  
> @@ -45,6 +46,7 @@ int xfs_pwork_init(struct xfs_mount *mp, struct xfs_pwork_ctl *pctl,
>  		unsigned int nr_threads);
>  void xfs_pwork_queue(struct xfs_pwork_ctl *pctl, struct xfs_pwork *pwork);
>  int xfs_pwork_destroy(struct xfs_pwork_ctl *pctl);
> +int xfs_pwork_destroy_poll(struct xfs_pwork_ctl *pctl);
>  unsigned int xfs_pwork_guess_datadev_parallelism(struct xfs_mount *mp);
>  
>  #endif /* __XFS_PWORK_H__ */
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index e4f3785f7a64..de6a623ada02 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1305,7 +1305,7 @@ xfs_qm_quotacheck(
>  		flags |= XFS_PQUOTA_CHKD;
>  	}
>  
> -	error = xfs_iwalk_threaded(mp, 0, xfs_qm_dqusage_adjust, 0, NULL);
> +	error = xfs_iwalk_threaded(mp, 0, xfs_qm_dqusage_adjust, 0, true, NULL);
>  	if (error)
>  		goto error_return;
>  
>