[PATCHSET 0/3] xfs: speed up parallel workqueues

[PATCHSET 0/3] xfs: speed up parallel workqueues

[Darrick J. Wong]

Hi all,

After some discussion on IRC with Dave, we came to the conclusion that
our background workqueue behavior could use some tweaking.  Kernel
worker threads that scan the filesystem and/or run their own
transactions more closely fit the definition of an unbound workqueue --
the work items can take a long time, they don't have much in common with
the submitter thread, the submitter isn't waiting hotly for a response,
and we the process scheduler should deal with scheduling them.
Furthermore, we don't want to place artificial limits on workqueue
scaling because that can leave unused capacity while we're blocking
(pwork is currently used for mount time quotacheck).

Therefore, we switch pwork to use an unbound workqueue, and now we let
the workqueue code figure out the relevant concurrency.

The final tweak is to enable WQ_SYSFS on all workqueues so that we can
monitor their concurrency management (or lack thereof) via sysfs.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=workqueue-speedups-5.12
---
 fs/xfs/xfs_iwalk.c     |    5 +----
 fs/xfs/xfs_log.c       |    4 ++--
 fs/xfs/xfs_mru_cache.c |    2 +-
 fs/xfs/xfs_pwork.c     |   21 ++-------------------
 fs/xfs/xfs_pwork.h     |    1 -
 fs/xfs/xfs_super.c     |   23 ++++++++++++++---------
 6 files changed, 20 insertions(+), 36 deletions(-)

[PATCH 1/3] xfs: increase the default parallelism levels of pwork clients

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Increase the parallelism level for pwork clients to the workqueue
defaults so that we can take advantage of computers with a lot of CPUs
and a lot of hardware.  On fast systems this will speed up quotacheck by
a large factor, and the following posteof/cowblocks cleanup series will
use the functionality presented in this patch to run garbage collection
as quickly as possible.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_iwalk.c |    5 +----
 fs/xfs/xfs_pwork.c |   17 -----------------
 fs/xfs/xfs_pwork.h |    1 -
 3 files changed, 1 insertion(+), 22 deletions(-)


diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c
index eae3aff9bc97..fc5ea8eb701f 100644
--- a/fs/xfs/xfs_iwalk.c
+++ b/fs/xfs/xfs_iwalk.c
@@ -618,15 +618,12 @@ xfs_iwalk_threaded(
 {
 	struct xfs_pwork_ctl	pctl;
 	xfs_agnumber_t		agno = XFS_INO_TO_AGNO(mp, startino);
-	unsigned int		nr_threads;
 	int			error;
 
 	ASSERT(agno < mp->m_sb.sb_agcount);
 	ASSERT(!(flags & ~XFS_IWALK_FLAGS_ALL));
 
-	nr_threads = xfs_pwork_guess_datadev_parallelism(mp);
-	error = xfs_pwork_init(mp, &pctl, xfs_iwalk_ag_work, "xfs_iwalk",
-			nr_threads);
+	error = xfs_pwork_init(mp, &pctl, xfs_iwalk_ag_work, "xfs_iwalk", 0);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/xfs_pwork.c b/fs/xfs/xfs_pwork.c
index b03333f1c84a..33fe952cdaf8 100644
--- a/fs/xfs/xfs_pwork.c
+++ b/fs/xfs/xfs_pwork.c
@@ -117,20 +117,3 @@ xfs_pwork_poll(
 				atomic_read(&pctl->nr_work) == 0, HZ) == 0)
 		touch_softlockup_watchdog();
 }
-
-/*
- * Return the amount of parallelism that the data device can handle, or 0 for
- * no limit.
- */
-unsigned int
-xfs_pwork_guess_datadev_parallelism(
-	struct xfs_mount	*mp)
-{
-	struct xfs_buftarg	*btp = mp->m_ddev_targp;
-
-	/*
-	 * For now we'll go with the most conservative setting possible,
-	 * which is two threads for an SSD and 1 thread everywhere else.
-	 */
-	return blk_queue_nonrot(btp->bt_bdev->bd_disk->queue) ? 2 : 1;
-}
diff --git a/fs/xfs/xfs_pwork.h b/fs/xfs/xfs_pwork.h
index 8133124cf3bb..e72676c0c285 100644
--- a/fs/xfs/xfs_pwork.h
+++ b/fs/xfs/xfs_pwork.h
@@ -56,6 +56,5 @@ int xfs_pwork_init(struct xfs_mount *mp, struct xfs_pwork_ctl *pctl,
 void xfs_pwork_queue(struct xfs_pwork_ctl *pctl, struct xfs_pwork *pwork);
 int xfs_pwork_destroy(struct xfs_pwork_ctl *pctl);
 void xfs_pwork_poll(struct xfs_pwork_ctl *pctl);
-unsigned int xfs_pwork_guess_datadev_parallelism(struct xfs_mount *mp);
 
 #endif /* __XFS_PWORK_H__ */

Re: [PATCH 1/3] xfs: increase the default parallelism levels of pwork clients

[Christoph Hellwig]

On Sat, Jan 23, 2021 at 10:53:08AM -0800, Darrick J. Wong wrote:
>  	ASSERT(agno < mp->m_sb.sb_agcount);
>  	ASSERT(!(flags & ~XFS_IWALK_FLAGS_ALL));
>  
> -	nr_threads = xfs_pwork_guess_datadev_parallelism(mp);
> -	error = xfs_pwork_init(mp, &pctl, xfs_iwalk_ag_work, "xfs_iwalk",
> -			nr_threads);
> +	error = xfs_pwork_init(mp, &pctl, xfs_iwalk_ag_work, "xfs_iwalk", 0);


Why not drop the last argument to xfs_pwork_init as well?  Also I don't
think this makes sense as a standalone step without the changes in the
next patch.

Re: [PATCH 1/3] xfs: increase the default parallelism levels of pwork clients

[Darrick J. Wong]

On Sun, Jan 24, 2021 at 09:57:17AM +0000, Christoph Hellwig wrote:
> On Sat, Jan 23, 2021 at 10:53:08AM -0800, Darrick J. Wong wrote:
> >  	ASSERT(agno < mp->m_sb.sb_agcount);
> >  	ASSERT(!(flags & ~XFS_IWALK_FLAGS_ALL));
> >  
> > -	nr_threads = xfs_pwork_guess_datadev_parallelism(mp);
> > -	error = xfs_pwork_init(mp, &pctl, xfs_iwalk_ag_work, "xfs_iwalk",
> > -			nr_threads);
> > +	error = xfs_pwork_init(mp, &pctl, xfs_iwalk_ag_work, "xfs_iwalk", 0);
> 
> 
> Why not drop the last argument to xfs_pwork_init as well?  Also I don't
> think this makes sense as a standalone step without the changes in the
> next patch.

Ok, I'll combine these two.

--D

[PATCH v2.1 1/3] xfs: increase the default parallelism levels of pwork clients

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Increase the parallelism level for pwork clients to the workqueue
defaults so that we can take advantage of computers with a lot of CPUs
and a lot of hardware.  On fast systems this will speed up quotacheck by
a large factor, and the following posteof/cowblocks cleanup series will
use the functionality presented in this patch to run garbage collection
as quickly as possible.

We do this by switching the pwork workqueue to unbounded, since the
current user (quotacheck) runs lengthy scans for each work item and we
don't care about dispatching the work on a warm cpu cache or anything
like that.  Also set WQ_SYSFS so that we can monitor where the wq is
running.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v2.1: document the workqueue knobs, kill the nr_threads argument to
pwork, and convert it to unbounded all in one patch
---
 Documentation/admin-guide/xfs.rst |   33 +++++++++++++++++++++++++++++++++
 fs/xfs/xfs_iwalk.c                |    5 +----
 fs/xfs/xfs_pwork.c                |   25 +++++--------------------
 fs/xfs/xfs_pwork.h                |    4 +---
 4 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
index 86de8a1ad91c..5fd14556c6fe 100644
--- a/Documentation/admin-guide/xfs.rst
+++ b/Documentation/admin-guide/xfs.rst
@@ -495,3 +495,36 @@ the class and error context. For example, the default values for
 "metadata/ENODEV" are "0" rather than "-1" so that this error handler defaults
 to "fail immediately" behaviour. This is done because ENODEV is a fatal,
 unrecoverable error no matter how many times the metadata IO is retried.
+
+Workqueue Concurrency
+=====================
+
+XFS uses kernel workqueues to parallelize metadata update processes.  This
+enables it to take advantage of storage hardware that can service many IO
+operations simultaneously.
+
+The control knobs for a filesystem's workqueues are organized by task at hand
+and the short name of the data device.  They all can be found in:
+
+  /sys/bus/workqueue/devices/${task}!${device}
+
+================  ===========
+  Task            Description
+================  ===========
+  xfs_iwalk-$pid  Inode scans of the entire filesystem. Currently limited to
+                  mount time quotacheck.
+================  ===========
+
+For example, the knobs for the quotacheck workqueue for /dev/nvme0n1 would be
+found in /sys/bus/workqueue/devices/xfs_iwalk-1111!nvme0n1/.
+
+The interesting knobs for XFS workqueues are as follows:
+
+============     ===========
+  Knob           Description
+============     ===========
+  max_active     Maximum number of background threads that can be started to
+                 run the work.
+  cpumask        CPUs upon which the threads are allowed to run.
+  nice           Relative priority of scheduling the threads.  These are the
+                 same nice levels that can be applied to userspace processes.
diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c
index eae3aff9bc97..c4a340f1f1e1 100644
--- a/fs/xfs/xfs_iwalk.c
+++ b/fs/xfs/xfs_iwalk.c
@@ -618,15 +618,12 @@ xfs_iwalk_threaded(
 {
 	struct xfs_pwork_ctl	pctl;
 	xfs_agnumber_t		agno = XFS_INO_TO_AGNO(mp, startino);
-	unsigned int		nr_threads;
 	int			error;
 
 	ASSERT(agno < mp->m_sb.sb_agcount);
 	ASSERT(!(flags & ~XFS_IWALK_FLAGS_ALL));
 
-	nr_threads = xfs_pwork_guess_datadev_parallelism(mp);
-	error = xfs_pwork_init(mp, &pctl, xfs_iwalk_ag_work, "xfs_iwalk",
-			nr_threads);
+	error = xfs_pwork_init(mp, &pctl, xfs_iwalk_ag_work, "xfs_iwalk");
 	if (error)
 		return error;
 
diff --git a/fs/xfs/xfs_pwork.c b/fs/xfs/xfs_pwork.c
index b03333f1c84a..c283b801cc5d 100644
--- a/fs/xfs/xfs_pwork.c
+++ b/fs/xfs/xfs_pwork.c
@@ -61,16 +61,18 @@ xfs_pwork_init(
 	struct xfs_mount	*mp,
 	struct xfs_pwork_ctl	*pctl,
 	xfs_pwork_work_fn	work_fn,
-	const char		*tag,
-	unsigned int		nr_threads)
+	const char		*tag)
 {
+	unsigned int		nr_threads = 0;
+
 #ifdef DEBUG
 	if (xfs_globals.pwork_threads >= 0)
 		nr_threads = xfs_globals.pwork_threads;
 #endif
 	trace_xfs_pwork_init(mp, nr_threads, current->pid);
 
-	pctl->wq = alloc_workqueue("%s-%d", WQ_FREEZABLE, nr_threads, tag,
+	pctl->wq = alloc_workqueue("%s-%d",
+			WQ_UNBOUND | WQ_SYSFS | WQ_FREEZABLE, nr_threads, tag,
 			current->pid);
 	if (!pctl->wq)
 		return -ENOMEM;
@@ -117,20 +119,3 @@ xfs_pwork_poll(
 				atomic_read(&pctl->nr_work) == 0, HZ) == 0)
 		touch_softlockup_watchdog();
 }
-
-/*
- * Return the amount of parallelism that the data device can handle, or 0 for
- * no limit.
- */
-unsigned int
-xfs_pwork_guess_datadev_parallelism(
-	struct xfs_mount	*mp)
-{
-	struct xfs_buftarg	*btp = mp->m_ddev_targp;
-
-	/*
-	 * For now we'll go with the most conservative setting possible,
-	 * which is two threads for an SSD and 1 thread everywhere else.
-	 */
-	return blk_queue_nonrot(btp->bt_bdev->bd_disk->queue) ? 2 : 1;
-}
diff --git a/fs/xfs/xfs_pwork.h b/fs/xfs/xfs_pwork.h
index 8133124cf3bb..c0ef81fc85dd 100644
--- a/fs/xfs/xfs_pwork.h
+++ b/fs/xfs/xfs_pwork.h
@@ -51,11 +51,9 @@ xfs_pwork_want_abort(
 }
 
 int xfs_pwork_init(struct xfs_mount *mp, struct xfs_pwork_ctl *pctl,
-		xfs_pwork_work_fn work_fn, const char *tag,
-		unsigned int nr_threads);
+		xfs_pwork_work_fn work_fn, const char *tag);
 void xfs_pwork_queue(struct xfs_pwork_ctl *pctl, struct xfs_pwork *pwork);
 int xfs_pwork_destroy(struct xfs_pwork_ctl *pctl);
 void xfs_pwork_poll(struct xfs_pwork_ctl *pctl);
-unsigned int xfs_pwork_guess_datadev_parallelism(struct xfs_mount *mp);
 
 #endif /* __XFS_PWORK_H__ */

Re: [PATCH v2.1 1/3] xfs: increase the default parallelism levels of pwork clients

[Dave Chinner]

On Mon, Jan 25, 2021 at 09:04:52PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Increase the parallelism level for pwork clients to the workqueue
> defaults so that we can take advantage of computers with a lot of CPUs
> and a lot of hardware.  On fast systems this will speed up quotacheck by
> a large factor, and the following posteof/cowblocks cleanup series will
> use the functionality presented in this patch to run garbage collection
> as quickly as possible.
> 
> We do this by switching the pwork workqueue to unbounded, since the
> current user (quotacheck) runs lengthy scans for each work item and we
> don't care about dispatching the work on a warm cpu cache or anything
> like that.  Also set WQ_SYSFS so that we can monitor where the wq is
> running.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v2.1: document the workqueue knobs, kill the nr_threads argument to
> pwork, and convert it to unbounded all in one patch
> ---
>  Documentation/admin-guide/xfs.rst |   33 +++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_iwalk.c                |    5 +----
>  fs/xfs/xfs_pwork.c                |   25 +++++--------------------
>  fs/xfs/xfs_pwork.h                |    4 +---
>  4 files changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
> index 86de8a1ad91c..5fd14556c6fe 100644
> --- a/Documentation/admin-guide/xfs.rst
> +++ b/Documentation/admin-guide/xfs.rst
> @@ -495,3 +495,36 @@ the class and error context. For example, the default values for
>  "metadata/ENODEV" are "0" rather than "-1" so that this error handler defaults
>  to "fail immediately" behaviour. This is done because ENODEV is a fatal,
>  unrecoverable error no matter how many times the metadata IO is retried.
> +
> +Workqueue Concurrency
> +=====================
> +
> +XFS uses kernel workqueues to parallelize metadata update processes.  This
> +enables it to take advantage of storage hardware that can service many IO
> +operations simultaneously.
> +
> +The control knobs for a filesystem's workqueues are organized by task at hand
> +and the short name of the data device.  They all can be found in:
> +
> +  /sys/bus/workqueue/devices/${task}!${device}
> +
> +================  ===========
> +  Task            Description
> +================  ===========
> +  xfs_iwalk-$pid  Inode scans of the entire filesystem. Currently limited to
> +                  mount time quotacheck.
> +================  ===========
> +
> +For example, the knobs for the quotacheck workqueue for /dev/nvme0n1 would be
> +found in /sys/bus/workqueue/devices/xfs_iwalk-1111!nvme0n1/.
> +
> +The interesting knobs for XFS workqueues are as follows:
> +
> +============     ===========
> +  Knob           Description
> +============     ===========
> +  max_active     Maximum number of background threads that can be started to
> +                 run the work.
> +  cpumask        CPUs upon which the threads are allowed to run.
> +  nice           Relative priority of scheduling the threads.  These are the
> +                 same nice levels that can be applied to userspace processes.

I'd suggest that a comment be added here along the lines of:

This interface exposes an internal implementation detail of XFS, and
as such is explicitly not part of any userspace API/ABI guarantee
the kernel may give userspace. These are undocumented features of
the generic workqueue implementation XFS uses for concurrency, and
they are provided here purely for diagnostic and tuning purposes and
may change at any time in the future.

Otherwise looks ok.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2.1 1/3] xfs: increase the default parallelism levels of pwork clients

[Darrick J. Wong]

On Wed, Jan 27, 2021 at 07:46:12AM +1100, Dave Chinner wrote:
> On Mon, Jan 25, 2021 at 09:04:52PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Increase the parallelism level for pwork clients to the workqueue
> > defaults so that we can take advantage of computers with a lot of CPUs
> > and a lot of hardware.  On fast systems this will speed up quotacheck by
> > a large factor, and the following posteof/cowblocks cleanup series will
> > use the functionality presented in this patch to run garbage collection
> > as quickly as possible.
> > 
> > We do this by switching the pwork workqueue to unbounded, since the
> > current user (quotacheck) runs lengthy scans for each work item and we
> > don't care about dispatching the work on a warm cpu cache or anything
> > like that.  Also set WQ_SYSFS so that we can monitor where the wq is
> > running.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > v2.1: document the workqueue knobs, kill the nr_threads argument to
> > pwork, and convert it to unbounded all in one patch
> > ---
> >  Documentation/admin-guide/xfs.rst |   33 +++++++++++++++++++++++++++++++++
> >  fs/xfs/xfs_iwalk.c                |    5 +----
> >  fs/xfs/xfs_pwork.c                |   25 +++++--------------------
> >  fs/xfs/xfs_pwork.h                |    4 +---
> >  4 files changed, 40 insertions(+), 27 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
> > index 86de8a1ad91c..5fd14556c6fe 100644
> > --- a/Documentation/admin-guide/xfs.rst
> > +++ b/Documentation/admin-guide/xfs.rst
> > @@ -495,3 +495,36 @@ the class and error context. For example, the default values for
> >  "metadata/ENODEV" are "0" rather than "-1" so that this error handler defaults
> >  to "fail immediately" behaviour. This is done because ENODEV is a fatal,
> >  unrecoverable error no matter how many times the metadata IO is retried.
> > +
> > +Workqueue Concurrency
> > +=====================
> > +
> > +XFS uses kernel workqueues to parallelize metadata update processes.  This
> > +enables it to take advantage of storage hardware that can service many IO
> > +operations simultaneously.
> > +
> > +The control knobs for a filesystem's workqueues are organized by task at hand
> > +and the short name of the data device.  They all can be found in:
> > +
> > +  /sys/bus/workqueue/devices/${task}!${device}
> > +
> > +================  ===========
> > +  Task            Description
> > +================  ===========
> > +  xfs_iwalk-$pid  Inode scans of the entire filesystem. Currently limited to
> > +                  mount time quotacheck.
> > +================  ===========
> > +
> > +For example, the knobs for the quotacheck workqueue for /dev/nvme0n1 would be
> > +found in /sys/bus/workqueue/devices/xfs_iwalk-1111!nvme0n1/.
> > +
> > +The interesting knobs for XFS workqueues are as follows:
> > +
> > +============     ===========
> > +  Knob           Description
> > +============     ===========
> > +  max_active     Maximum number of background threads that can be started to
> > +                 run the work.
> > +  cpumask        CPUs upon which the threads are allowed to run.
> > +  nice           Relative priority of scheduling the threads.  These are the
> > +                 same nice levels that can be applied to userspace processes.
> 
> I'd suggest that a comment be added here along the lines of:
> 
> This interface exposes an internal implementation detail of XFS, and
> as such is explicitly not part of any userspace API/ABI guarantee
> the kernel may give userspace. These are undocumented features of
> the generic workqueue implementation XFS uses for concurrency, and
> they are provided here purely for diagnostic and tuning purposes and
> may change at any time in the future.
> 
> Otherwise looks ok.

Done, thanks for taking a look at this.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

[PATCH 2/3] xfs: use unbounded workqueues for parallel work

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Switch the pwork workqueue to unbounded, since the current user
(quotacheck) runs lengthy scans for each work item and we don't care
about dispatching the work on a warm cpu cache or anything like that.
Also set WQ_SYSFS so that we can monitor where the wq is running.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_pwork.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/xfs_pwork.c b/fs/xfs/xfs_pwork.c
index 33fe952cdaf8..704a1c2af90c 100644
--- a/fs/xfs/xfs_pwork.c
+++ b/fs/xfs/xfs_pwork.c
@@ -70,8 +70,8 @@ xfs_pwork_init(
 #endif
 	trace_xfs_pwork_init(mp, nr_threads, current->pid);
 
-	pctl->wq = alloc_workqueue("%s-%d", WQ_FREEZABLE, nr_threads, tag,
-			current->pid);
+	pctl->wq = alloc_workqueue("%s-%d", WQ_UNBOUND | WQ_SYSFS | WQ_FREEZABLE,
+			nr_threads, tag, current->pid);
 	if (!pctl->wq)
 		return -ENOMEM;
 	pctl->work_fn = work_fn;

Re: [PATCH 2/3] xfs: use unbounded workqueues for parallel work

[Christoph Hellwig]

On Sat, Jan 23, 2021 at 10:53:14AM -0800, Darrick J. Wong wrote:
> -	pctl->wq = alloc_workqueue("%s-%d", WQ_FREEZABLE, nr_threads, tag,
> -			current->pid);
> +	pctl->wq = alloc_workqueue("%s-%d", WQ_UNBOUND | WQ_SYSFS | WQ_FREEZABLE,
> +			nr_threads, tag, current->pid);

This adds an overly long line.


But more importantly I think xfs.txt needs to grow a section that we now
can tune XFS parameters through the workqueue sysfs files, especially as
right now I have no idea how to find those based on an actual device or
XFS mount I need to adjust the parameters for.

Re: [PATCH 2/3] xfs: use unbounded workqueues for parallel work

[Darrick J. Wong]

On Sun, Jan 24, 2021 at 09:51:50AM +0000, Christoph Hellwig wrote:
> On Sat, Jan 23, 2021 at 10:53:14AM -0800, Darrick J. Wong wrote:
> > -	pctl->wq = alloc_workqueue("%s-%d", WQ_FREEZABLE, nr_threads, tag,
> > -			current->pid);
> > +	pctl->wq = alloc_workqueue("%s-%d", WQ_UNBOUND | WQ_SYSFS | WQ_FREEZABLE,
> > +			nr_threads, tag, current->pid);
> 
> This adds an overly long line.

Changed.

> But more importantly I think xfs.txt needs to grow a section that we now
> can tune XFS parameters through the workqueue sysfs files, especially as
> right now I have no idea how to find those based on an actual device or
> XFS mount I need to adjust the parameters for.

Ok, I'll add a section.

--D

[PATCH 3/3] xfs: set WQ_SYSFS on all workqueues

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Set WQ_SYSFS on all workqueues that we create so that we have a means to
monitor cpu affinity and whatnot for background workers.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_log.c       |    4 ++--
 fs/xfs/xfs_mru_cache.c |    2 +-
 fs/xfs/xfs_super.c     |   23 ++++++++++++++---------
 3 files changed, 17 insertions(+), 12 deletions(-)


diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 58699881c100..edb16843dff7 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1493,8 +1493,8 @@ xlog_alloc_log(
 	log->l_iclog->ic_prev = prev_iclog;	/* re-write 1st prev ptr */
 
 	log->l_ioend_workqueue = alloc_workqueue("xfs-log/%s",
-			WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI, 0,
-			mp->m_super->s_id);
+			WQ_SYSFS | WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI,
+			0, mp->m_super->s_id);
 	if (!log->l_ioend_workqueue)
 		goto out_free_iclog;
 
diff --git a/fs/xfs/xfs_mru_cache.c b/fs/xfs/xfs_mru_cache.c
index a06661dac5be..b6dab34e361d 100644
--- a/fs/xfs/xfs_mru_cache.c
+++ b/fs/xfs/xfs_mru_cache.c
@@ -294,7 +294,7 @@ int
 xfs_mru_cache_init(void)
 {
 	xfs_mru_reap_wq = alloc_workqueue("xfs_mru_cache",
-				WQ_MEM_RECLAIM|WQ_FREEZABLE, 1);
+				WQ_SYSFS | WQ_MEM_RECLAIM | WQ_FREEZABLE, 1);
 	if (!xfs_mru_reap_wq)
 		return -ENOMEM;
 	return 0;
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index aed74a3fc787..ebe3eba2cbbc 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -495,33 +495,37 @@ xfs_init_mount_workqueues(
 	struct xfs_mount	*mp)
 {
 	mp->m_buf_workqueue = alloc_workqueue("xfs-buf/%s",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 1, mp->m_super->s_id);
+			WQ_SYSFS | WQ_MEM_RECLAIM | WQ_FREEZABLE, 1,
+			mp->m_super->s_id);
 	if (!mp->m_buf_workqueue)
 		goto out;
 
 	mp->m_unwritten_workqueue = alloc_workqueue("xfs-conv/%s",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_super->s_id);
+			WQ_SYSFS | WQ_MEM_RECLAIM | WQ_FREEZABLE, 0,
+			mp->m_super->s_id);
 	if (!mp->m_unwritten_workqueue)
 		goto out_destroy_buf;
 
 	mp->m_cil_workqueue = alloc_workqueue("xfs-cil/%s",
-			WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_UNBOUND,
+			WQ_SYSFS | WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_UNBOUND,
 			0, mp->m_super->s_id);
 	if (!mp->m_cil_workqueue)
 		goto out_destroy_unwritten;
 
 	mp->m_reclaim_workqueue = alloc_workqueue("xfs-reclaim/%s",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_super->s_id);
+			WQ_SYSFS | WQ_MEM_RECLAIM | WQ_FREEZABLE, 0,
+			mp->m_super->s_id);
 	if (!mp->m_reclaim_workqueue)
 		goto out_destroy_cil;
 
 	mp->m_eofblocks_workqueue = alloc_workqueue("xfs-eofblocks/%s",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_super->s_id);
+			WQ_SYSFS | WQ_MEM_RECLAIM | WQ_FREEZABLE, 0,
+			mp->m_super->s_id);
 	if (!mp->m_eofblocks_workqueue)
 		goto out_destroy_reclaim;
 
-	mp->m_sync_workqueue = alloc_workqueue("xfs-sync/%s", WQ_FREEZABLE, 0,
-					       mp->m_super->s_id);
+	mp->m_sync_workqueue = alloc_workqueue("xfs-sync/%s",
+			WQ_SYSFS | WQ_FREEZABLE, 0, mp->m_super->s_id);
 	if (!mp->m_sync_workqueue)
 		goto out_destroy_eofb;
 
@@ -2085,11 +2089,12 @@ xfs_init_workqueues(void)
 	 * max_active value for this workqueue.
 	 */
 	xfs_alloc_wq = alloc_workqueue("xfsalloc",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0);
+			WQ_SYSFS | WQ_MEM_RECLAIM | WQ_FREEZABLE, 0);
 	if (!xfs_alloc_wq)
 		return -ENOMEM;
 
-	xfs_discard_wq = alloc_workqueue("xfsdiscard", WQ_UNBOUND, 0);
+	xfs_discard_wq = alloc_workqueue("xfsdiscard",
+			WQ_SYSFS | WQ_UNBOUND, 0);
 	if (!xfs_discard_wq)
 		goto out_free_alloc_wq;
 

Re: [PATCH 3/3] xfs: set WQ_SYSFS on all workqueues

[Christoph Hellwig]

>  	log->l_ioend_workqueue = alloc_workqueue("xfs-log/%s",
> -			WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI, 0,
> -			mp->m_super->s_id);
> +			WQ_SYSFS | WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI,
> +			0, mp->m_super->s_id);

This is just used for log I/O completions which are effectlively single
thread.  I don't see any reason to adjust the parameters here.

>  	if (!log->l_ioend_workqueue)
>  		goto out_free_iclog;
>  
> diff --git a/fs/xfs/xfs_mru_cache.c b/fs/xfs/xfs_mru_cache.c
> index a06661dac5be..b6dab34e361d 100644
> --- a/fs/xfs/xfs_mru_cache.c
> +++ b/fs/xfs/xfs_mru_cache.c
> @@ -294,7 +294,7 @@ int
>  xfs_mru_cache_init(void)
>  {
>  	xfs_mru_reap_wq = alloc_workqueue("xfs_mru_cache",
> -				WQ_MEM_RECLAIM|WQ_FREEZABLE, 1);
> +				WQ_SYSFS | WQ_MEM_RECLAIM | WQ_FREEZABLE, 1);
>  	if (!xfs_mru_reap_wq)
>  		return -ENOMEM;

This one also hasn't ever been something we tune, so I don't think there
is a good case for enabling WQ_SYSFS.

I've stopped here.  I think we should have a good use case for making
workqueues show up in sysfs based on that we:

 a) have resons to adjust them ever
 b) actually having them easily discoverable and documented for adminds
    to tune

Re: [PATCH 3/3] xfs: set WQ_SYSFS on all workqueues

[Darrick J. Wong]

On Sun, Jan 24, 2021 at 09:54:54AM +0000, Christoph Hellwig wrote:
> >  	log->l_ioend_workqueue = alloc_workqueue("xfs-log/%s",
> > -			WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI, 0,
> > -			mp->m_super->s_id);
> > +			WQ_SYSFS | WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI,
> > +			0, mp->m_super->s_id);
> 
> This is just used for log I/O completions which are effectlively single
> thread.  I don't see any reason to adjust the parameters here.
> 
> >  	if (!log->l_ioend_workqueue)
> >  		goto out_free_iclog;
> >  
> > diff --git a/fs/xfs/xfs_mru_cache.c b/fs/xfs/xfs_mru_cache.c
> > index a06661dac5be..b6dab34e361d 100644
> > --- a/fs/xfs/xfs_mru_cache.c
> > +++ b/fs/xfs/xfs_mru_cache.c
> > @@ -294,7 +294,7 @@ int
> >  xfs_mru_cache_init(void)
> >  {
> >  	xfs_mru_reap_wq = alloc_workqueue("xfs_mru_cache",
> > -				WQ_MEM_RECLAIM|WQ_FREEZABLE, 1);
> > +				WQ_SYSFS | WQ_MEM_RECLAIM | WQ_FREEZABLE, 1);
> >  	if (!xfs_mru_reap_wq)
> >  		return -ENOMEM;
> 
> This one also hasn't ever been something we tune, so I don't think there
> is a good case for enabling WQ_SYSFS.

Yeah, the only ones I want to push for (and hence document) are
quotacheck, background blockgc, and (in 5.13) background inode
inactivation.

> I've stopped here.  I think we should have a good use case for making
> workqueues show up in sysfs based on that we:
> 
>  a) have resons to adjust them ever
>  b) actually having them easily discoverable and documented for adminds
>     to tune

TBH I think the only workqueues we really ought to expose publicly are
the unbound ones, since they represent kernel threads that can log
transactions, and hence are known to have a performance impact that
sysadmins could tune reasonably.

Dave suggests exposing them all on a debug kernel, of course. :)

--D

[PATCH 4/3] xfs: set WQ_SYSFS on all workqueues in debug mode

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

When CONFIG_XFS_DEBUG=y, set WQ_SYSFS on all workqueues that we create
so that we (developers) have a means to monitor cpu affinity and whatnot
for background workers.  In the next patchset we'll expose knobs for
some of the workqueues publicly and document it, but not now.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_log.c       |    5 +++--
 fs/xfs/xfs_mru_cache.c |    2 +-
 fs/xfs/xfs_super.c     |   23 ++++++++++++++---------
 fs/xfs/xfs_super.h     |    6 ++++++
 4 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 58699881c100..0da019a4a7f9 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1493,8 +1493,9 @@ xlog_alloc_log(
 	log->l_iclog->ic_prev = prev_iclog;	/* re-write 1st prev ptr */
 
 	log->l_ioend_workqueue = alloc_workqueue("xfs-log/%s",
-			WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI, 0,
-			mp->m_super->s_id);
+			XFS_WQFLAGS(WQ_FREEZABLE | WQ_MEM_RECLAIM |
+				    WQ_HIGHPRI),
+			0, mp->m_super->s_id);
 	if (!log->l_ioend_workqueue)
 		goto out_free_iclog;
 
diff --git a/fs/xfs/xfs_mru_cache.c b/fs/xfs/xfs_mru_cache.c
index a06661dac5be..34c3b16f834f 100644
--- a/fs/xfs/xfs_mru_cache.c
+++ b/fs/xfs/xfs_mru_cache.c
@@ -294,7 +294,7 @@ int
 xfs_mru_cache_init(void)
 {
 	xfs_mru_reap_wq = alloc_workqueue("xfs_mru_cache",
-				WQ_MEM_RECLAIM|WQ_FREEZABLE, 1);
+			XFS_WQFLAGS(WQ_MEM_RECLAIM | WQ_FREEZABLE), 1);
 	if (!xfs_mru_reap_wq)
 		return -ENOMEM;
 	return 0;
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index aed74a3fc787..8959561351ca 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -495,33 +495,37 @@ xfs_init_mount_workqueues(
 	struct xfs_mount	*mp)
 {
 	mp->m_buf_workqueue = alloc_workqueue("xfs-buf/%s",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 1, mp->m_super->s_id);
+			XFS_WQFLAGS(WQ_FREEZABLE | WQ_MEM_RECLAIM),
+			1, mp->m_super->s_id);
 	if (!mp->m_buf_workqueue)
 		goto out;
 
 	mp->m_unwritten_workqueue = alloc_workqueue("xfs-conv/%s",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_super->s_id);
+			XFS_WQFLAGS(WQ_FREEZABLE | WQ_MEM_RECLAIM),
+			0, mp->m_super->s_id);
 	if (!mp->m_unwritten_workqueue)
 		goto out_destroy_buf;
 
 	mp->m_cil_workqueue = alloc_workqueue("xfs-cil/%s",
-			WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_UNBOUND,
+			XFS_WQFLAGS(WQ_FREEZABLE | WQ_MEM_RECLAIM | WQ_UNBOUND),
 			0, mp->m_super->s_id);
 	if (!mp->m_cil_workqueue)
 		goto out_destroy_unwritten;
 
 	mp->m_reclaim_workqueue = alloc_workqueue("xfs-reclaim/%s",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_super->s_id);
+			XFS_WQFLAGS(WQ_FREEZABLE | WQ_MEM_RECLAIM),
+			0, mp->m_super->s_id);
 	if (!mp->m_reclaim_workqueue)
 		goto out_destroy_cil;
 
 	mp->m_eofblocks_workqueue = alloc_workqueue("xfs-eofblocks/%s",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_super->s_id);
+			XFS_WQFLAGS(WQ_FREEZABLE | WQ_MEM_RECLAIM),
+			0, mp->m_super->s_id);
 	if (!mp->m_eofblocks_workqueue)
 		goto out_destroy_reclaim;
 
-	mp->m_sync_workqueue = alloc_workqueue("xfs-sync/%s", WQ_FREEZABLE, 0,
-					       mp->m_super->s_id);
+	mp->m_sync_workqueue = alloc_workqueue("xfs-sync/%s",
+			XFS_WQFLAGS(WQ_FREEZABLE), 0, mp->m_super->s_id);
 	if (!mp->m_sync_workqueue)
 		goto out_destroy_eofb;
 
@@ -2085,11 +2089,12 @@ xfs_init_workqueues(void)
 	 * max_active value for this workqueue.
 	 */
 	xfs_alloc_wq = alloc_workqueue("xfsalloc",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0);
+			XFS_WQFLAGS(WQ_MEM_RECLAIM | WQ_FREEZABLE), 0);
 	if (!xfs_alloc_wq)
 		return -ENOMEM;
 
-	xfs_discard_wq = alloc_workqueue("xfsdiscard", WQ_UNBOUND, 0);
+	xfs_discard_wq = alloc_workqueue("xfsdiscard", XFS_WQFLAGS(WQ_UNBOUND),
+			0);
 	if (!xfs_discard_wq)
 		goto out_free_alloc_wq;
 
diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
index b552cf6d3379..1ca484b8357f 100644
--- a/fs/xfs/xfs_super.h
+++ b/fs/xfs/xfs_super.h
@@ -75,6 +75,12 @@ extern void xfs_qm_exit(void);
 				XFS_ASSERT_FATAL_STRING \
 				XFS_DBG_STRING /* DBG must be last */
 
+#ifdef DEBUG
+# define XFS_WQFLAGS(wqflags)	(WQ_SYSFS | (wqflags))
+#else
+# define XFS_WQFLAGS(wqflags)	(wqflags)
+#endif
+
 struct xfs_inode;
 struct xfs_mount;
 struct xfs_buftarg;

Re: [PATCH 4/3] xfs: set WQ_SYSFS on all workqueues in debug mode

[Dave Chinner]

On Mon, Jan 25, 2021 at 09:06:19PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> When CONFIG_XFS_DEBUG=y, set WQ_SYSFS on all workqueues that we create
> so that we (developers) have a means to monitor cpu affinity and whatnot
> for background workers.  In the next patchset we'll expose knobs for
> some of the workqueues publicly and document it, but not now.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks fine to me.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/3] xfs: set WQ_SYSFS on all workqueues in debug mode

[Christoph Hellwig]

On Mon, Jan 25, 2021 at 09:06:19PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> When CONFIG_XFS_DEBUG=y, set WQ_SYSFS on all workqueues that we create
> so that we (developers) have a means to monitor cpu affinity and whatnot
> for background workers.  In the next patchset we'll expose knobs for
> some of the workqueues publicly and document it, but not now.

I don't really think this is a very good idea.  If we want something like
this it should be kernel-wide and coordinated with the workqueue 
maintainer, but I'm a little doubtful about the use case.

Re: [PATCH 4/3] xfs: set WQ_SYSFS on all workqueues in debug mode

[Dave Chinner]

On Wed, Jan 27, 2021 at 05:03:06PM +0000, Christoph Hellwig wrote:
> On Mon, Jan 25, 2021 at 09:06:19PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > When CONFIG_XFS_DEBUG=y, set WQ_SYSFS on all workqueues that we create
> > so that we (developers) have a means to monitor cpu affinity and whatnot
> > for background workers.  In the next patchset we'll expose knobs for
> > some of the workqueues publicly and document it, but not now.
> 
> I don't really think this is a very good idea.  If we want something like
> this it should be kernel-wide and coordinated with the workqueue 
> maintainer, but I'm a little doubtful about the use case.

I don't think it is particular useful kernel wide. If it was, the
maintainer wouldn't have introduced a per-workqueue flag for this
functionality.

The reality is that very few workqueues in the system can expand out
into running thousands of kworker threads like the XFS workqueues
often do. And, really, there's nothing useful a typical user can do
at this point with the workqueue knobs to "tune" the behaviour - the
visibility/control the workqueue sysfs knobs provide at this point
is really only useful to XFS developers running tests in controlled
conditions...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com