[PATCH 1/2] libfrog: Prevent unnecessary waking of worker thread when using bounded workqueues

[PATCH 1/2] libfrog: Prevent unnecessary waking of worker thread when using bounded workqueues

[Chandan Babu R]

When woken up, a worker will pick a work item from the workqueue and wake up
another worker when the current workqueue is a bounded workqueue and if there
is atleast one more work item remains to be processed.

The commit 12838bda12e669 ("libfrog: fix overly sleep workqueues") prevented
single-threaded processing of work items by waking up sleeping workers when a
work item is added to the workqueue. Hence the earlier described mechanism of
waking workers is no longer required.

Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>
---
 libfrog/workqueue.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/libfrog/workqueue.c b/libfrog/workqueue.c
index db5b3f68b..9384270ff 100644
--- a/libfrog/workqueue.c
+++ b/libfrog/workqueue.c
@@ -51,10 +51,6 @@ workqueue_thread(void *arg)
 		wq->next_item = wi->next;
 		wq->item_count--;
 
-		if (wq->max_queued && wq->next_item) {
-			/* more work, wake up another worker */
-			pthread_cond_signal(&wq->wakeup);
-		}
 		wq->active_threads++;
 		pthread_mutex_unlock(&wq->lock);
 
-- 
2.45.2

[PATCH 2/2] repair/prefetch.c: Create one workqueue with multiple workers

[Chandan Babu R]

When xfs_repair is executed with a non-zero value for ag_stride,
do_inode_prefetch() create multiple workqueues with each of them having just
one worker thread.

Since commit 12838bda12e669 ("libfrog: fix overly sleep workqueues"), a
workqueue can process multiple work items concurrently. Hence, this commit
replaces the above logic with just one workqueue having multiple workers.

Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>
---
 repair/prefetch.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/repair/prefetch.c b/repair/prefetch.c
index 5ecf19ae9..8cd3416fa 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -1024,7 +1024,6 @@ do_inode_prefetch(
 {
 	int			i;
 	struct workqueue	queue;
-	struct workqueue	*queues;
 	int			queues_started = 0;
 
 	/*
@@ -1056,7 +1055,7 @@ do_inode_prefetch(
 	/*
 	 * create one worker thread for each segment of the volume
 	 */
-	queues = malloc(thread_count * sizeof(struct workqueue));
+	create_work_queue(&queue, mp, thread_count);
 	for (i = 0; i < thread_count; i++) {
 		struct pf_work_args *wargs;
 
@@ -1067,8 +1066,7 @@ do_inode_prefetch(
 		wargs->dirs_only = dirs_only;
 		wargs->func = func;
 
-		create_work_queue(&queues[i], mp, 1);
-		queue_work(&queues[i], prefetch_ag_range_work, 0, wargs);
+		queue_work(&queue, prefetch_ag_range_work, 0, wargs);
 		queues_started++;
 
 		if (wargs->end_ag >= mp->m_sb.sb_agcount)
@@ -1078,9 +1076,7 @@ do_inode_prefetch(
 	/*
 	 * wait for workers to complete
 	 */
-	for (i = 0; i < queues_started; i++)
-		destroy_work_queue(&queues[i]);
-	free(queues);
+	destroy_work_queue(&queue);
 }
 
 void
-- 
2.45.2

Re: [PATCH 2/2] repair/prefetch.c: Create one workqueue with multiple workers

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] repair/prefetch.c: Create one workqueue with multiple workers

[Darrick J. Wong]

On Tue, Nov 04, 2025 at 02:44:37PM +0530, Chandan Babu R wrote:
> When xfs_repair is executed with a non-zero value for ag_stride,
> do_inode_prefetch() create multiple workqueues with each of them having just
> one worker thread.
> 
> Since commit 12838bda12e669 ("libfrog: fix overly sleep workqueues"), a
> workqueue can process multiple work items concurrently. Hence, this commit
> replaces the above logic with just one workqueue having multiple workers.
> 
> Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>

Yeah, this also makes sense to me.  I wonder if this odd code was some
sort of workaround for the pre-12838bd workqueue behavior?

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  repair/prefetch.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/repair/prefetch.c b/repair/prefetch.c
> index 5ecf19ae9..8cd3416fa 100644
> --- a/repair/prefetch.c
> +++ b/repair/prefetch.c
> @@ -1024,7 +1024,6 @@ do_inode_prefetch(
>  {
>  	int			i;
>  	struct workqueue	queue;
> -	struct workqueue	*queues;
>  	int			queues_started = 0;
>  
>  	/*
> @@ -1056,7 +1055,7 @@ do_inode_prefetch(
>  	/*
>  	 * create one worker thread for each segment of the volume
>  	 */
> -	queues = malloc(thread_count * sizeof(struct workqueue));
> +	create_work_queue(&queue, mp, thread_count);
>  	for (i = 0; i < thread_count; i++) {
>  		struct pf_work_args *wargs;
>  
> @@ -1067,8 +1066,7 @@ do_inode_prefetch(
>  		wargs->dirs_only = dirs_only;
>  		wargs->func = func;
>  
> -		create_work_queue(&queues[i], mp, 1);
> -		queue_work(&queues[i], prefetch_ag_range_work, 0, wargs);
> +		queue_work(&queue, prefetch_ag_range_work, 0, wargs);
>  		queues_started++;
>  
>  		if (wargs->end_ag >= mp->m_sb.sb_agcount)
> @@ -1078,9 +1076,7 @@ do_inode_prefetch(
>  	/*
>  	 * wait for workers to complete
>  	 */
> -	for (i = 0; i < queues_started; i++)
> -		destroy_work_queue(&queues[i]);
> -	free(queues);
> +	destroy_work_queue(&queue);
>  }
>  
>  void
> -- 
> 2.45.2
> 
> 

Re: [PATCH 2/2] repair/prefetch.c: Create one workqueue with multiple workers

[Chandan Babu R]

On Tue, Nov 04, 2025 at 08:55:34 AM -0800, Darrick J. Wong wrote:
> On Tue, Nov 04, 2025 at 02:44:37PM +0530, Chandan Babu R wrote:
>> When xfs_repair is executed with a non-zero value for ag_stride,
>> do_inode_prefetch() create multiple workqueues with each of them having just
>> one worker thread.
>> 
>> Since commit 12838bda12e669 ("libfrog: fix overly sleep workqueues"), a
>> workqueue can process multiple work items concurrently. Hence, this commit
>> replaces the above logic with just one workqueue having multiple workers.
>> 
>> Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>
>
> Yeah, this also makes sense to me.  I wonder if this odd code was some
> sort of workaround for the pre-12838bd workqueue behavior?
>

The old implementation of workqueue which was present in repair/threads.c
would wakeup a worker thread only when inserting a new work item to an empty
work list. So, yes, this most likely is the reason for creating multiple
workqueues.

-- 
Chandan

Re: [PATCH 1/2] libfrog: Prevent unnecessary waking of worker thread when using bounded workqueues

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] libfrog: Prevent unnecessary waking of worker thread when using bounded workqueues

[Darrick J. Wong]

On Tue, Nov 04, 2025 at 02:44:36PM +0530, Chandan Babu R wrote:
> When woken up, a worker will pick a work item from the workqueue and wake up
> another worker when the current workqueue is a bounded workqueue and if there
> is atleast one more work item remains to be processed.
> 
> The commit 12838bda12e669 ("libfrog: fix overly sleep workqueues") prevented
> single-threaded processing of work items by waking up sleeping workers when a
> work item is added to the workqueue. Hence the earlier described mechanism of
> waking workers is no longer required.
> 
> Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>

That makes sense to me, sorry for forgetting to remove this hunk.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  libfrog/workqueue.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/libfrog/workqueue.c b/libfrog/workqueue.c
> index db5b3f68b..9384270ff 100644
> --- a/libfrog/workqueue.c
> +++ b/libfrog/workqueue.c
> @@ -51,10 +51,6 @@ workqueue_thread(void *arg)
>  		wq->next_item = wi->next;
>  		wq->item_count--;
>  
> -		if (wq->max_queued && wq->next_item) {
> -			/* more work, wake up another worker */
> -			pthread_cond_signal(&wq->wakeup);
> -		}
>  		wq->active_threads++;
>  		pthread_mutex_unlock(&wq->lock);
>  
> -- 
> 2.45.2
> 
>