Re: [PATCH v2] io_uring/io-wq: fix incorrect io_wq_for_each_worker() termination logic

[Gabriel Krisman Bertazi <krisman@suse.de>]

Jens Axboe <axboe@kernel.dk> writes:

> A previous commit added this helper, and had it terminate if false is
> returned from the handler. However, that is completely opposite, it
> should abort the loop if true is returned.
>
> Fix this up by having io_wq_for_each_worker() keep iterating as long
> as false is returned, and only abort if true is returned.

The fix is good, but the API is just weird.

io_acct_for_each_worker returning true indicates an error that will
abort the wq walk.  It is a non-issue, since all the two callers cannot
fail and always return false for success :-)

Reviewed-by: Gabriel Krisman Bertazi <krisman@suse.de>


> Cc: stable@vger.kernel.org
> Fixes: 751eedc4b4b7 ("io_uring/io-wq: move worker lists to struct io_wq_acct")
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>
> ---
>
> v2: fix the actual bug, rather than work-around it for the exit
>     condition only.
>
> diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c
> index cd13d8aac3d2..6c5ef629e59a 100644
> --- a/io_uring/io-wq.c
> +++ b/io_uring/io-wq.c
> @@ -952,11 +952,11 @@ static bool io_wq_for_each_worker(struct io_wq *wq,
>  				  void *data)
>  {
>  	for (int i = 0; i < IO_WQ_ACCT_NR; i++) {
> -		if (!io_acct_for_each_worker(&wq->acct[i], func, data))
> -			return false;
> +		if (io_acct_for_each_worker(&wq->acct[i], func, data))
> +			return true;
>  	}
>  
> -	return true;
> +	return false;
>  }
>  
>  static bool io_wq_worker_wake(struct io_worker *worker, void *data)

-- 
Gabriel Krisman Bertazi