diff mbox series

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

Message ID 161142799960.2173328.12558377173737512680.stgit@magnolia (mailing list archive)
State New
Headers show
Series xfs: speed up parallel workqueues | expand

Commit Message

Darrick J. Wong Jan. 23, 2021, 6:53 p.m. UTC 
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(-)

Comments

Christoph Hellwig Jan. 24, 2021, 9:54 a.m. UTC | #1 
>  	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
Darrick J. Wong Jan. 25, 2021, 11:30 p.m. UTC | #2 
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
Dave Chinner Jan. 26, 2021, 8:48 p.m. UTC | #3 
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>
diff mbox series

Patch

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;