[PATCH RESEND v2 00/10] A few bugfix and cleancode patch for bfq

[PATCH RESEND v2 00/10] A few bugfix and cleancode patch for bfq

[Kemeng Shi]

Hi, this series contain two patches to fix bug in request injection
mechanism and some random cleancode patches.
Thanks!

---
v2:
 -improve git log.
---

Kemeng Shi (10):
  block, bfq: correctly raise inject limit in
    bfq_choose_bfqq_for_injection
  block, bfq: remove unsed parameter reason in bfq_bfqq_is_slow
  block, bfq: initialize bfqq->decrease_time_jif correctly
  block, bfq: use helper macro RQ_BFQQ to get bfqq of request
  block, bfq: remove unnecessary dereference to get async_bfqq
  block, bfq: remove redundant bfqd->rq_in_driver > 0 check in
    bfq_add_request
  block, bfq: remove redundant check in bfq_put_cooperator
  block, bfq: remove unnecessary goto tag in bfq_dispatch_rq_from_bfqq
  block, bfq: remove unused bfq_wr_max_time in struct bfq_data
  block, bfq: remove check of bfq_wr_max_softrt_rate which is always
    greater than 0

 block/bfq-iosched.c | 49 +++++++++++++++++----------------------------
 block/bfq-iosched.h |  2 --
 2 files changed, 18 insertions(+), 33 deletions(-)

-- 
2.30.0

[PATCH RESEND v2 01/10] block, bfq: correctly raise inject limit in bfq_choose_bfqq_for_injection

[Kemeng Shi]

Function bfq_choose_bfqq_for_injection may temporarily raise inject limit
to one request if current inject_limit is 0 before search of the source
queue for injection. However the search below will reset inject limit to
bfqd->in_service_queue which is zero for raised inject limit. Then the
temporarily raised inject limit never works as expected.
Assigment limit to bfqd->in_service_queue in search is needed as limit
maybe overwriten to min_t(unsigned int, 1, limit) for condition that
a large in-flight request is on non-rotational devices in found queue.
So we need to reset limit to bfqd->in_service_queue for normal case.

Actually, we have already make sure bfqd->rq_in_driver is < limit before
search, then
 -Limit is >= 1 as bfqd->rq_in_driver is >= 0. Then min_t(unsigned int,
1, limit) is always 1. So we can simply check bfqd->rq_in_driver with
1 instead of result of min_t(unsigned int, 1, limit) for larget request in
non-rotational device case to avoid overwritting limit and the bug is gone.
 -For normal case, we have already check bfqd->rq_in_driver is < limit,
so we can return found bfqq unconditionally to remove unncessary check.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 block/bfq-iosched.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index a72304c728fc..1220c41fc767 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -4641,12 +4641,10 @@ bfq_choose_bfqq_for_injection(struct bfq_data *bfqd)
 			 */
 			if (blk_queue_nonrot(bfqd->queue) &&
 			    blk_rq_sectors(bfqq->next_rq) >=
-			    BFQQ_SECT_THR_NONROT)
-				limit = min_t(unsigned int, 1, limit);
-			else
-				limit = in_serv_bfqq->inject_limit;
-
-			if (bfqd->rq_in_driver < limit) {
+			    BFQQ_SECT_THR_NONROT &&
+			    bfqd->rq_in_driver >= 1)
+				continue;
+			else {
 				bfqd->rqs_injected = true;
 				return bfqq;
 			}
-- 
2.30.0

Re: [PATCH RESEND v2 01/10] block, bfq: correctly raise inject limit in bfq_choose_bfqq_for_injection

[Jan Kara]

On Fri 23-12-22 03:16:32, Kemeng Shi wrote:
> Function bfq_choose_bfqq_for_injection may temporarily raise inject limit
> to one request if current inject_limit is 0 before search of the source
> queue for injection. However the search below will reset inject limit to
> bfqd->in_service_queue which is zero for raised inject limit. Then the
> temporarily raised inject limit never works as expected.
> Assigment limit to bfqd->in_service_queue in search is needed as limit
> maybe overwriten to min_t(unsigned int, 1, limit) for condition that
> a large in-flight request is on non-rotational devices in found queue.
> So we need to reset limit to bfqd->in_service_queue for normal case.
> 
> Actually, we have already make sure bfqd->rq_in_driver is < limit before
> search, then
>  -Limit is >= 1 as bfqd->rq_in_driver is >= 0. Then min_t(unsigned int,
> 1, limit) is always 1. So we can simply check bfqd->rq_in_driver with
> 1 instead of result of min_t(unsigned int, 1, limit) for larget request in
> non-rotational device case to avoid overwritting limit and the bug is gone.
>  -For normal case, we have already check bfqd->rq_in_driver is < limit,
> so we can return found bfqq unconditionally to remove unncessary check.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

The fix looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/bfq-iosched.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index a72304c728fc..1220c41fc767 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -4641,12 +4641,10 @@ bfq_choose_bfqq_for_injection(struct bfq_data *bfqd)
>  			 */
>  			if (blk_queue_nonrot(bfqd->queue) &&
>  			    blk_rq_sectors(bfqq->next_rq) >=
> -			    BFQQ_SECT_THR_NONROT)
> -				limit = min_t(unsigned int, 1, limit);
> -			else
> -				limit = in_serv_bfqq->inject_limit;
> -
> -			if (bfqd->rq_in_driver < limit) {
> +			    BFQQ_SECT_THR_NONROT &&
> +			    bfqd->rq_in_driver >= 1)
> +				continue;
> +			else {
>  				bfqd->rqs_injected = true;
>  				return bfqq;
>  			}
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RESEND v2 02/10] block, bfq: remove unsed parameter reason in bfq_bfqq_is_slow

[Kemeng Shi]

Parameter reason is never used, just remove it.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 block/bfq-iosched.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 1220c41fc767..5a6d9e8c329d 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -4066,8 +4066,7 @@ static void __bfq_bfqq_recalc_budget(struct bfq_data *bfqd,
  * function to evaluate the I/O speed of a process.
  */
 static bool bfq_bfqq_is_slow(struct bfq_data *bfqd, struct bfq_queue *bfqq,
-				 bool compensate, enum bfqq_expiration reason,
-				 unsigned long *delta_ms)
+				 bool compensate, unsigned long *delta_ms)
 {
 	ktime_t delta_ktime;
 	u32 delta_usecs;
@@ -4263,7 +4262,7 @@ void bfq_bfqq_expire(struct bfq_data *bfqd,
 	/*
 	 * Check whether the process is slow (see bfq_bfqq_is_slow).
 	 */
-	slow = bfq_bfqq_is_slow(bfqd, bfqq, compensate, reason, &delta);
+	slow = bfq_bfqq_is_slow(bfqd, bfqq, compensate, &delta);
 
 	/*
 	 * As above explained, charge slow (typically seeky) and
-- 
2.30.0

Re: [PATCH RESEND v2 02/10] block, bfq: remove unsed parameter reason in bfq_bfqq_is_slow

[Jan Kara]

On Fri 23-12-22 03:16:33, Kemeng Shi wrote:
> Parameter reason is never used, just remove it.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Sure. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/bfq-iosched.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 1220c41fc767..5a6d9e8c329d 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -4066,8 +4066,7 @@ static void __bfq_bfqq_recalc_budget(struct bfq_data *bfqd,
>   * function to evaluate the I/O speed of a process.
>   */
>  static bool bfq_bfqq_is_slow(struct bfq_data *bfqd, struct bfq_queue *bfqq,
> -				 bool compensate, enum bfqq_expiration reason,
> -				 unsigned long *delta_ms)
> +				 bool compensate, unsigned long *delta_ms)
>  {
>  	ktime_t delta_ktime;
>  	u32 delta_usecs;
> @@ -4263,7 +4262,7 @@ void bfq_bfqq_expire(struct bfq_data *bfqd,
>  	/*
>  	 * Check whether the process is slow (see bfq_bfqq_is_slow).
>  	 */
> -	slow = bfq_bfqq_is_slow(bfqd, bfqq, compensate, reason, &delta);
> +	slow = bfq_bfqq_is_slow(bfqd, bfqq, compensate, &delta);
>  
>  	/*
>  	 * As above explained, charge slow (typically seeky) and
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RESEND v2 03/10] block, bfq: initialize bfqq->decrease_time_jif correctly

[Kemeng Shi]

Inject limit is updated or reset when time_is_before_eq_jiffies(
decrease_time_jif + several msecs) or think-time state changes.
decrease_time_jif is initialized to 0 and will be set to current jiffies
when inject limit is updated or reset. If the jiffies is slightly greater
than LONG_MAX, time_is_after_eq_jiffies(0) will keep for a long time, so as
time_is_after_eq_jiffies(decrease_time_jif + several msecs). If the
think-time state never chages, then the injection will not work as expected
for long time.

To be more specific:
Function bfq_update_inject_limit maybe triggered when jiffies pasts
decrease_time_jif + msecs_to_jiffies(10) in bfq_add_request by setting
bfqd->wait_dispatch to true.
Function bfq_reset_inject_limit are called in two conditions:
1. jiffies pasts bfqq->decrease_time_jif + msecs_to_jiffies(1000) in
function bfq_add_request.
2. jiffies pasts bfqq->decrease_time_jif + msecs_to_jiffies(100) or
bfq think-time state change from short to long.

Fix this by initializing bfqq->decrease_time_jif to current jiffies
to trigger service injection soon when service injection conditions
are met.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 block/bfq-iosched.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 5a6d9e8c329d..9441fc98961a 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5491,6 +5491,8 @@ static void bfq_init_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq,
 
 	/* first request is almost certainly seeky */
 	bfqq->seek_history = 1;
+
+	bfqq->decrease_time_jif = jiffies;
 }
 
 static struct bfq_queue **bfq_async_queue_prio(struct bfq_data *bfqd,
-- 
2.30.0

Re: [PATCH RESEND v2 03/10] block, bfq: initialize bfqq->decrease_time_jif correctly

[Jan Kara]

On Fri 23-12-22 03:16:34, Kemeng Shi wrote:
> Inject limit is updated or reset when time_is_before_eq_jiffies(
> decrease_time_jif + several msecs) or think-time state changes.
> decrease_time_jif is initialized to 0 and will be set to current jiffies
> when inject limit is updated or reset. If the jiffies is slightly greater
> than LONG_MAX, time_is_after_eq_jiffies(0) will keep for a long time, so as
> time_is_after_eq_jiffies(decrease_time_jif + several msecs). If the
> think-time state never chages, then the injection will not work as expected
> for long time.
> 
> To be more specific:
> Function bfq_update_inject_limit maybe triggered when jiffies pasts
> decrease_time_jif + msecs_to_jiffies(10) in bfq_add_request by setting
> bfqd->wait_dispatch to true.
> Function bfq_reset_inject_limit are called in two conditions:
> 1. jiffies pasts bfqq->decrease_time_jif + msecs_to_jiffies(1000) in
> function bfq_add_request.
> 2. jiffies pasts bfqq->decrease_time_jif + msecs_to_jiffies(100) or
> bfq think-time state change from short to long.
> 
> Fix this by initializing bfqq->decrease_time_jif to current jiffies
> to trigger service injection soon when service injection conditions
> are met.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

I agree that initializing jiffies timer to 0 is just a bad practice and
current time is much saner choice. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/bfq-iosched.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 5a6d9e8c329d..9441fc98961a 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -5491,6 +5491,8 @@ static void bfq_init_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq,
>  
>  	/* first request is almost certainly seeky */
>  	bfqq->seek_history = 1;
> +
> +	bfqq->decrease_time_jif = jiffies;
>  }
>  
>  static struct bfq_queue **bfq_async_queue_prio(struct bfq_data *bfqd,
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RESEND v2 04/10] block, bfq: use helper macro RQ_BFQQ to get bfqq of request

[Kemeng Shi]

Use helper macro RQ_BFQQ to get bfqq of request.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 block/bfq-iosched.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 9441fc98961a..c3c4c83ee95f 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -6683,14 +6683,14 @@ static struct bfq_queue *bfq_init_rq(struct request *rq)
 		return NULL;
 
 	/*
-	 * Assuming that elv.priv[1] is set only if everything is set
+	 * Assuming that RQ_BFQQ(rq) is set only if everything is set
 	 * for this rq. This holds true, because this function is
 	 * invoked only for insertion or merging, and, after such
 	 * events, a request cannot be manipulated any longer before
 	 * being removed from bfq.
 	 */
-	if (rq->elv.priv[1])
-		return rq->elv.priv[1];
+	if (RQ_BFQQ(rq))
+		return RQ_BFQQ(rq);
 
 	bic = icq_to_bic(rq->elv.icq);
 
-- 
2.30.0

Re: [PATCH RESEND v2 04/10] block, bfq: use helper macro RQ_BFQQ to get bfqq of request

[Jan Kara]

On Fri 23-12-22 03:16:35, Kemeng Shi wrote:
> Use helper macro RQ_BFQQ to get bfqq of request.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Yeah, why not. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/bfq-iosched.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 9441fc98961a..c3c4c83ee95f 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -6683,14 +6683,14 @@ static struct bfq_queue *bfq_init_rq(struct request *rq)
>  		return NULL;
>  
>  	/*
> -	 * Assuming that elv.priv[1] is set only if everything is set
> +	 * Assuming that RQ_BFQQ(rq) is set only if everything is set
>  	 * for this rq. This holds true, because this function is
>  	 * invoked only for insertion or merging, and, after such
>  	 * events, a request cannot be manipulated any longer before
>  	 * being removed from bfq.
>  	 */
> -	if (rq->elv.priv[1])
> -		return rq->elv.priv[1];
> +	if (RQ_BFQQ(rq))
> +		return RQ_BFQQ(rq);
>  
>  	bic = icq_to_bic(rq->elv.icq);
>  
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RESEND v2 05/10] block, bfq: remove unnecessary dereference to get async_bfqq

[Kemeng Shi]

The async_bfqq is assigned with bfqq->bic->bfqq[0], use it directly.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 block/bfq-iosched.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index c3c4c83ee95f..ebcafe3c4c3b 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -4835,7 +4835,7 @@ static struct bfq_queue *bfq_select_queue(struct bfq_data *bfqd)
 		    icq_to_bic(async_bfqq->next_rq->elv.icq) == bfqq->bic &&
 		    bfq_serv_to_charge(async_bfqq->next_rq, async_bfqq) <=
 		    bfq_bfqq_budget_left(async_bfqq))
-			bfqq = bfqq->bic->bfqq[0];
+			bfqq = async_bfqq;
 		else if (bfqq->waker_bfqq &&
 			   bfq_bfqq_busy(bfqq->waker_bfqq) &&
 			   bfqq->waker_bfqq->next_rq &&
-- 
2.30.0

Re: [PATCH RESEND v2 05/10] block, bfq: remove unnecessary dereference to get async_bfqq

[Jan Kara]

On Fri 23-12-22 03:16:36, Kemeng Shi wrote:
> The async_bfqq is assigned with bfqq->bic->bfqq[0], use it directly.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Yeah, that is a more logical choice. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/bfq-iosched.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index c3c4c83ee95f..ebcafe3c4c3b 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -4835,7 +4835,7 @@ static struct bfq_queue *bfq_select_queue(struct bfq_data *bfqd)
>  		    icq_to_bic(async_bfqq->next_rq->elv.icq) == bfqq->bic &&
>  		    bfq_serv_to_charge(async_bfqq->next_rq, async_bfqq) <=
>  		    bfq_bfqq_budget_left(async_bfqq))
> -			bfqq = bfqq->bic->bfqq[0];
> +			bfqq = async_bfqq;
>  		else if (bfqq->waker_bfqq &&
>  			   bfq_bfqq_busy(bfqq->waker_bfqq) &&
>  			   bfqq->waker_bfqq->next_rq &&
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RESEND v2 06/10] block, bfq: remove redundant bfqd->rq_in_driver > 0 check in bfq_add_request

[Kemeng Shi]

The bfqd->rq_in_driver > 0 check is along with previous
"bfqd->rq_in_driver == 0 ||" check, so no need to re-check
bfqd->rq_in_driver > 0.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 block/bfq-iosched.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index ebcafe3c4c3b..7c91d16dbf6f 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2204,8 +2204,7 @@ static void bfq_add_request(struct request *rq)
 		 */
 		if (bfqq == bfqd->in_service_queue &&
 		    (bfqd->rq_in_driver == 0 ||
-		     (bfqq->last_serv_time_ns > 0 &&
-		      bfqd->rqs_injected && bfqd->rq_in_driver > 0)) &&
+		     (bfqq->last_serv_time_ns > 0 && bfqd->rqs_injected)) &&
 		    time_is_before_eq_jiffies(bfqq->decrease_time_jif +
 					      msecs_to_jiffies(10))) {
 			bfqd->last_empty_occupied_ns = ktime_get_ns();
-- 
2.30.0

Re: [PATCH RESEND v2 06/10] block, bfq: remove redundant bfqd->rq_in_driver > 0 check in bfq_add_request

[Jan Kara]

On Fri 23-12-22 03:16:37, Kemeng Shi wrote:
> The bfqd->rq_in_driver > 0 check is along with previous
> "bfqd->rq_in_driver == 0 ||" check, so no need to re-check
> bfqd->rq_in_driver > 0.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

I have nothing against this but OTOH in this complex condition the original
version looks more readable than the new one to me.

								Honza

> ---
>  block/bfq-iosched.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index ebcafe3c4c3b..7c91d16dbf6f 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2204,8 +2204,7 @@ static void bfq_add_request(struct request *rq)
>  		 */
>  		if (bfqq == bfqd->in_service_queue &&
>  		    (bfqd->rq_in_driver == 0 ||
> -		     (bfqq->last_serv_time_ns > 0 &&
> -		      bfqd->rqs_injected && bfqd->rq_in_driver > 0)) &&
> +		     (bfqq->last_serv_time_ns > 0 && bfqd->rqs_injected)) &&
>  		    time_is_before_eq_jiffies(bfqq->decrease_time_jif +
>  					      msecs_to_jiffies(10))) {
>  			bfqd->last_empty_occupied_ns = ktime_get_ns();
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RESEND v2 07/10] block, bfq: remove redundant check in bfq_put_cooperator

[Kemeng Shi]

We have already avoided a circular list in bfq_setup_merge (see comments
in bfq_setup_merge() for details), so bfq_queue will not appear in it's
new_bfqq list. Just remove this check.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 block/bfq-iosched.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 7c91d16dbf6f..89995815dbae 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5273,8 +5273,6 @@ void bfq_put_cooperator(struct bfq_queue *bfqq)
 	 */
 	__bfqq = bfqq->new_bfqq;
 	while (__bfqq) {
-		if (__bfqq == bfqq)
-			break;
 		next = __bfqq->new_bfqq;
 		bfq_put_queue(__bfqq);
 		__bfqq = next;
-- 
2.30.0

Re: [PATCH RESEND v2 07/10] block, bfq: remove redundant check in bfq_put_cooperator

[Jan Kara]

On Fri 23-12-22 03:16:38, Kemeng Shi wrote:
> We have already avoided a circular list in bfq_setup_merge (see comments
> in bfq_setup_merge() for details), so bfq_queue will not appear in it's
> new_bfqq list. Just remove this check.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/bfq-iosched.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 7c91d16dbf6f..89995815dbae 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -5273,8 +5273,6 @@ void bfq_put_cooperator(struct bfq_queue *bfqq)
>  	 */
>  	__bfqq = bfqq->new_bfqq;
>  	while (__bfqq) {
> -		if (__bfqq == bfqq)
> -			break;
>  		next = __bfqq->new_bfqq;
>  		bfq_put_queue(__bfqq);
>  		__bfqq = next;
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RESEND v2 08/10] block, bfq: remove unnecessary goto tag in bfq_dispatch_rq_from_bfqq

[Kemeng Shi]

We jump to tag only for returning current rq. Return directly to
remove this tag.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 block/bfq-iosched.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 89995815dbae..195cdc6be087 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -4965,7 +4965,7 @@ static struct request *bfq_dispatch_rq_from_bfqq(struct bfq_data *bfqd,
 	bfq_dispatch_remove(bfqd->queue, rq);
 
 	if (bfqq != bfqd->in_service_queue)
-		goto return_rq;
+		return rq;
 
 	/*
 	 * If weight raising has to terminate for bfqq, then next
@@ -4985,12 +4985,9 @@ static struct request *bfq_dispatch_rq_from_bfqq(struct bfq_data *bfqd,
 	 * belongs to CLASS_IDLE and other queues are waiting for
 	 * service.
 	 */
-	if (!(bfq_tot_busy_queues(bfqd) > 1 && bfq_class_idle(bfqq)))
-		goto return_rq;
+	if ((bfq_tot_busy_queues(bfqd) > 1 && bfq_class_idle(bfqq)))
+		bfq_bfqq_expire(bfqd, bfqq, false, BFQQE_BUDGET_EXHAUSTED);
 
-	bfq_bfqq_expire(bfqd, bfqq, false, BFQQE_BUDGET_EXHAUSTED);
-
-return_rq:
 	return rq;
 }
 
-- 
2.30.0

Re: [PATCH RESEND v2 08/10] block, bfq: remove unnecessary goto tag in bfq_dispatch_rq_from_bfqq

[Jan Kara]

On Fri 23-12-22 03:16:39, Kemeng Shi wrote:
> We jump to tag only for returning current rq. Return directly to
> remove this tag.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

One nit below.

> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 89995815dbae..195cdc6be087 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -4965,7 +4965,7 @@ static struct request *bfq_dispatch_rq_from_bfqq(struct bfq_data *bfqd,
>  	bfq_dispatch_remove(bfqd->queue, rq);
>  
>  	if (bfqq != bfqd->in_service_queue)
> -		goto return_rq;
> +		return rq;
>  
>  	/*
>  	 * If weight raising has to terminate for bfqq, then next
> @@ -4985,12 +4985,9 @@ static struct request *bfq_dispatch_rq_from_bfqq(struct bfq_data *bfqd,
>  	 * belongs to CLASS_IDLE and other queues are waiting for
>  	 * service.
>  	 */
> -	if (!(bfq_tot_busy_queues(bfqd) > 1 && bfq_class_idle(bfqq)))
> -		goto return_rq;
> +	if ((bfq_tot_busy_queues(bfqd) > 1 && bfq_class_idle(bfqq)))
            ^ unnecessary brace here

									Honza

> +		bfq_bfqq_expire(bfqd, bfqq, false, BFQQE_BUDGET_EXHAUSTED);
>  
> -	bfq_bfqq_expire(bfqd, bfqq, false, BFQQE_BUDGET_EXHAUSTED);
> -
> -return_rq:
>  	return rq;
>  }
>  
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RESEND v2 09/10] block, bfq: remove unused bfq_wr_max_time in struct bfq_data

[Kemeng Shi]

bfqd->bfq_wr_max_time is set to 0 in bfq_init_queue and is never changed.
It is only used in bfq_wr_duration when bfq_wr_max_time > 0 which never
meets, so bfqd->bfq_wr_max_time is not used actually. Just remove it.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 block/bfq-iosched.c | 4 ----
 block/bfq-iosched.h | 2 --
 2 files changed, 6 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 195cdc6be087..91bc68fba72d 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -1068,9 +1068,6 @@ static unsigned int bfq_wr_duration(struct bfq_data *bfqd)
 {
 	u64 dur;
 
-	if (bfqd->bfq_wr_max_time > 0)
-		return bfqd->bfq_wr_max_time;
-
 	dur = bfqd->rate_dur_prod;
 	do_div(dur, bfqd->peak_rate);
 
@@ -7079,7 +7076,6 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e)
 	 */
 	bfqd->bfq_wr_coeff = 30;
 	bfqd->bfq_wr_rt_max_time = msecs_to_jiffies(300);
-	bfqd->bfq_wr_max_time = 0;
 	bfqd->bfq_wr_min_idle_time = msecs_to_jiffies(2000);
 	bfqd->bfq_wr_min_inter_arr_async = msecs_to_jiffies(500);
 	bfqd->bfq_wr_max_softrt_rate = 7000; /*
diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
index 9fa89577322d..0b1a5438046a 100644
--- a/block/bfq-iosched.h
+++ b/block/bfq-iosched.h
@@ -719,8 +719,6 @@ struct bfq_data {
 	 * is multiplied.
 	 */
 	unsigned int bfq_wr_coeff;
-	/* maximum duration of a weight-raising period (jiffies) */
-	unsigned int bfq_wr_max_time;
 
 	/* Maximum weight-raising duration for soft real-time processes */
 	unsigned int bfq_wr_rt_max_time;
-- 
2.30.0

Re: [PATCH RESEND v2 09/10] block, bfq: remove unused bfq_wr_max_time in struct bfq_data

[Jan Kara]

On Fri 23-12-22 03:16:40, Kemeng Shi wrote:
> bfqd->bfq_wr_max_time is set to 0 in bfq_init_queue and is never changed.
> It is only used in bfq_wr_duration when bfq_wr_max_time > 0 which never
> meets, so bfqd->bfq_wr_max_time is not used actually. Just remove it.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Nice catch. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/bfq-iosched.c | 4 ----
>  block/bfq-iosched.h | 2 --
>  2 files changed, 6 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 195cdc6be087..91bc68fba72d 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -1068,9 +1068,6 @@ static unsigned int bfq_wr_duration(struct bfq_data *bfqd)
>  {
>  	u64 dur;
>  
> -	if (bfqd->bfq_wr_max_time > 0)
> -		return bfqd->bfq_wr_max_time;
> -
>  	dur = bfqd->rate_dur_prod;
>  	do_div(dur, bfqd->peak_rate);
>  
> @@ -7079,7 +7076,6 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e)
>  	 */
>  	bfqd->bfq_wr_coeff = 30;
>  	bfqd->bfq_wr_rt_max_time = msecs_to_jiffies(300);
> -	bfqd->bfq_wr_max_time = 0;
>  	bfqd->bfq_wr_min_idle_time = msecs_to_jiffies(2000);
>  	bfqd->bfq_wr_min_inter_arr_async = msecs_to_jiffies(500);
>  	bfqd->bfq_wr_max_softrt_rate = 7000; /*
> diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
> index 9fa89577322d..0b1a5438046a 100644
> --- a/block/bfq-iosched.h
> +++ b/block/bfq-iosched.h
> @@ -719,8 +719,6 @@ struct bfq_data {
>  	 * is multiplied.
>  	 */
>  	unsigned int bfq_wr_coeff;
> -	/* maximum duration of a weight-raising period (jiffies) */
> -	unsigned int bfq_wr_max_time;
>  
>  	/* Maximum weight-raising duration for soft real-time processes */
>  	unsigned int bfq_wr_rt_max_time;
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RESEND v2 10/10] block, bfq: remove check of bfq_wr_max_softrt_rate which is always greater than 0

[Kemeng Shi]

bfqd->bfq_wr_max_softrt_rate is assigned with 7000 in bfq_init_queue and
never changed. So we can remove bfqd->bfq_wr_max_softrt_rate > 0 check
which is always true.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 block/bfq-iosched.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 91bc68fba72d..00cdd42ac02a 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -1788,8 +1788,7 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd,
 	 *   to control its weight explicitly)
 	 */
 	in_burst = bfq_bfqq_in_large_burst(bfqq);
-	soft_rt = bfqd->bfq_wr_max_softrt_rate > 0 &&
-		!BFQQ_TOTALLY_SEEKY(bfqq) &&
+	soft_rt = !BFQQ_TOTALLY_SEEKY(bfqq) &&
 		!in_burst &&
 		time_is_before_jiffies(bfqq->soft_rt_next_start) &&
 		bfqq->dispatched == 0 &&
@@ -4284,8 +4283,7 @@ void bfq_bfqq_expire(struct bfq_data *bfqd,
 	if (bfqd->low_latency && bfqq->wr_coeff == 1)
 		bfqq->last_wr_start_finish = jiffies;
 
-	if (bfqd->low_latency && bfqd->bfq_wr_max_softrt_rate > 0 &&
-	    RB_EMPTY_ROOT(&bfqq->sort_list)) {
+	if (bfqd->low_latency && RB_EMPTY_ROOT(&bfqq->sort_list)) {
 		/*
 		 * If we get here, and there are no outstanding
 		 * requests, then the request pattern is isochronous
-- 
2.30.0

Re: [PATCH RESEND v2 10/10] block, bfq: remove check of bfq_wr_max_softrt_rate which is always greater than 0

[Jan Kara]

On Fri 23-12-22 03:16:41, Kemeng Shi wrote:
> bfqd->bfq_wr_max_softrt_rate is assigned with 7000 in bfq_init_queue and
> never changed. So we can remove bfqd->bfq_wr_max_softrt_rate > 0 check
> which is always true.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

I would just leave these checks for documentation purposes and for possible
experiments (e.g. disabling this logic by setting bfq_wr_max_softrt_rate to
0). Alternatively, we could just define a constant for this and
then we can remove all the checks, that would be a clean solution as well.

								Honza

> ---
>  block/bfq-iosched.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 91bc68fba72d..00cdd42ac02a 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -1788,8 +1788,7 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd,
>  	 *   to control its weight explicitly)
>  	 */
>  	in_burst = bfq_bfqq_in_large_burst(bfqq);
> -	soft_rt = bfqd->bfq_wr_max_softrt_rate > 0 &&
> -		!BFQQ_TOTALLY_SEEKY(bfqq) &&
> +	soft_rt = !BFQQ_TOTALLY_SEEKY(bfqq) &&
>  		!in_burst &&
>  		time_is_before_jiffies(bfqq->soft_rt_next_start) &&
>  		bfqq->dispatched == 0 &&
> @@ -4284,8 +4283,7 @@ void bfq_bfqq_expire(struct bfq_data *bfqd,
>  	if (bfqd->low_latency && bfqq->wr_coeff == 1)
>  		bfqq->last_wr_start_finish = jiffies;
>  
> -	if (bfqd->low_latency && bfqd->bfq_wr_max_softrt_rate > 0 &&
> -	    RB_EMPTY_ROOT(&bfqq->sort_list)) {
> +	if (bfqd->low_latency && RB_EMPTY_ROOT(&bfqq->sort_list)) {
>  		/*
>  		 * If we get here, and there are no outstanding
>  		 * requests, then the request pattern is isochronous
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR