[PATCH] block/bfq: fix ifdef for CONFIG_BFQ_GROUP_IOSCHED=y

[PATCH] block/bfq: fix ifdef for CONFIG_BFQ_GROUP_IOSCHED=y

[Konstantin Khlebnikov]

Replace BFQ_GROUP_IOSCHED_ENABLED with CONFIG_BFQ_GROUP_IOSCHED.
Code under these ifdefs never worked, something might be broken.

Fixes: 0471559c2fbd ("block, bfq: add/remove entity weights correctly")
Fixes: 73d58118498b ("block, bfq: consider also ioprio classes in symmetry detection")
Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 block/bfq-iosched.c |    2 +-
 block/bfq-wf2q.c    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 4c592496a16a..fac188dd78fa 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -674,7 +674,7 @@ static bool bfq_symmetric_scenario(struct bfq_data *bfqd)
 	 * at least two nodes.
 	 */
 	return !(varied_queue_weights || multiple_classes_busy
-#ifdef BFQ_GROUP_IOSCHED_ENABLED
+#ifdef CONFIG_BFQ_GROUP_IOSCHED
 	       || bfqd->num_groups_with_pending_reqs > 0
 #endif
 		);
diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
index 63311d1ff1ed..a11bef75483d 100644
--- a/block/bfq-wf2q.c
+++ b/block/bfq-wf2q.c
@@ -1012,7 +1012,7 @@ static void __bfq_activate_entity(struct bfq_entity *entity,
 		entity->on_st = true;
 	}
 
-#ifdef BFQ_GROUP_IOSCHED_ENABLED
+#ifdef CONFIG_BFQ_GROUP_IOSCHED
 	if (!bfq_entity_to_bfqq(entity)) { /* bfq_group */
 		struct bfq_group *bfqg =
 			container_of(entity, struct bfq_group, entity);

Re: [PATCH] block/bfq: fix ifdef for CONFIG_BFQ_GROUP_IOSCHED=y

[Holger Hoffstätte]

On 3/29/19 3:01 PM, Konstantin Khlebnikov wrote:
> Replace BFQ_GROUP_IOSCHED_ENABLED with CONFIG_BFQ_GROUP_IOSCHED.
> Code under these ifdefs never worked, something might be broken.
> 
> Fixes: 0471559c2fbd ("block, bfq: add/remove entity weights correctly")
> Fixes: 73d58118498b ("block, bfq: consider also ioprio classes in symmetry detection")
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> ---
>   block/bfq-iosched.c |    2 +-
>   block/bfq-wf2q.c    |    2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 4c592496a16a..fac188dd78fa 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -674,7 +674,7 @@ static bool bfq_symmetric_scenario(struct bfq_data *bfqd)
>   	 * at least two nodes.
>   	 */
>   	return !(varied_queue_weights || multiple_classes_busy
> -#ifdef BFQ_GROUP_IOSCHED_ENABLED
> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
>   	       || bfqd->num_groups_with_pending_reqs > 0
>   #endif
>   		);
> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
> index 63311d1ff1ed..a11bef75483d 100644
> --- a/block/bfq-wf2q.c
> +++ b/block/bfq-wf2q.c
> @@ -1012,7 +1012,7 @@ static void __bfq_activate_entity(struct bfq_entity *entity,
>   		entity->on_st = true;
>   	}
>   
> -#ifdef BFQ_GROUP_IOSCHED_ENABLED
> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
>   	if (!bfq_entity_to_bfqq(entity)) { /* bfq_group */
>   		struct bfq_group *bfqg =
>   			container_of(entity, struct bfq_group, entity);
> 
> 

Good catch! I run without group scheduling and therefore didn't notice these
stray defines earlier. For 5.1 it should merge cleanly; adding this on top of
the pending 5.2 BFQ patches required a small context fixup in hunk #1 due to
"block, bfq: do not idle for lowest-weight queues".

Reviewed-by: Holger Hoffstätte <holger@applied-asynchrony.com>

cheers,
Holger

Re: [PATCH] block/bfq: fix ifdef for CONFIG_BFQ_GROUP_IOSCHED=y

[Jens Axboe]

On 3/29/19 9:56 AM, Holger Hoffstätte wrote:
> On 3/29/19 3:01 PM, Konstantin Khlebnikov wrote:
>> Replace BFQ_GROUP_IOSCHED_ENABLED with CONFIG_BFQ_GROUP_IOSCHED.
>> Code under these ifdefs never worked, something might be broken.
>>
>> Fixes: 0471559c2fbd ("block, bfq: add/remove entity weights correctly")
>> Fixes: 73d58118498b ("block, bfq: consider also ioprio classes in symmetry detection")
>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>> ---
>>   block/bfq-iosched.c |    2 +-
>>   block/bfq-wf2q.c    |    2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>> index 4c592496a16a..fac188dd78fa 100644
>> --- a/block/bfq-iosched.c
>> +++ b/block/bfq-iosched.c
>> @@ -674,7 +674,7 @@ static bool bfq_symmetric_scenario(struct bfq_data *bfqd)
>>   	 * at least two nodes.
>>   	 */
>>   	return !(varied_queue_weights || multiple_classes_busy
>> -#ifdef BFQ_GROUP_IOSCHED_ENABLED
>> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
>>   	       || bfqd->num_groups_with_pending_reqs > 0
>>   #endif
>>   		);
>> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
>> index 63311d1ff1ed..a11bef75483d 100644
>> --- a/block/bfq-wf2q.c
>> +++ b/block/bfq-wf2q.c
>> @@ -1012,7 +1012,7 @@ static void __bfq_activate_entity(struct bfq_entity *entity,
>>   		entity->on_st = true;
>>   	}
>>   
>> -#ifdef BFQ_GROUP_IOSCHED_ENABLED
>> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
>>   	if (!bfq_entity_to_bfqq(entity)) { /* bfq_group */
>>   		struct bfq_group *bfqg =
>>   			container_of(entity, struct bfq_group, entity);
>>
>>
> 
> Good catch! I run without group scheduling and therefore didn't notice these
> stray defines earlier. For 5.1 it should merge cleanly; adding this on top of
> the pending 5.2 BFQ patches required a small context fixup in hunk #1 due to
> "block, bfq: do not idle for lowest-weight queues".

I'm hesitant to apply this, since the group scheduling stuff has obviously never
been tested.

Hence it should go through some actual testing first, which means it's a 5.2
candidate, not 5.1.

-- 
Jens Axboe

Re: [PATCH] block/bfq: fix ifdef for CONFIG_BFQ_GROUP_IOSCHED=y

[Holger Hoffstätte]

On 3/29/19 4:58 PM, Jens Axboe wrote:
> On 3/29/19 9:56 AM, Holger Hoffstätte wrote:
>> On 3/29/19 3:01 PM, Konstantin Khlebnikov wrote:
>>> Replace BFQ_GROUP_IOSCHED_ENABLED with CONFIG_BFQ_GROUP_IOSCHED.
>>> Code under these ifdefs never worked, something might be broken.
>>>
>>> Fixes: 0471559c2fbd ("block, bfq: add/remove entity weights correctly")
>>> Fixes: 73d58118498b ("block, bfq: consider also ioprio classes in symmetry detection")
>>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>>> ---
>>>    block/bfq-iosched.c |    2 +-
>>>    block/bfq-wf2q.c    |    2 +-
>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>>> index 4c592496a16a..fac188dd78fa 100644
>>> --- a/block/bfq-iosched.c
>>> +++ b/block/bfq-iosched.c
>>> @@ -674,7 +674,7 @@ static bool bfq_symmetric_scenario(struct bfq_data *bfqd)
>>>    	 * at least two nodes.
>>>    	 */
>>>    	return !(varied_queue_weights || multiple_classes_busy
>>> -#ifdef BFQ_GROUP_IOSCHED_ENABLED
>>> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
>>>    	       || bfqd->num_groups_with_pending_reqs > 0
>>>    #endif
>>>    		);
>>> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
>>> index 63311d1ff1ed..a11bef75483d 100644
>>> --- a/block/bfq-wf2q.c
>>> +++ b/block/bfq-wf2q.c
>>> @@ -1012,7 +1012,7 @@ static void __bfq_activate_entity(struct bfq_entity *entity,
>>>    		entity->on_st = true;
>>>    	}
>>>    
>>> -#ifdef BFQ_GROUP_IOSCHED_ENABLED
>>> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
>>>    	if (!bfq_entity_to_bfqq(entity)) { /* bfq_group */
>>>    		struct bfq_group *bfqg =
>>>    			container_of(entity, struct bfq_group, entity);
>>>
>>>
>>
>> Good catch! I run without group scheduling and therefore didn't notice these
>> stray defines earlier. For 5.1 it should merge cleanly; adding this on top of
>> the pending 5.2 BFQ patches required a small context fixup in hunk #1 due to
>> "block, bfq: do not idle for lowest-weight queues".
> 
> I'm hesitant to apply this, since the group scheduling stuff has obviously never
> been tested.

This is simply a regression in 5.1 caused by 73d58118498b - nothing else,
and as such this fix needs to go into 5.1 as well. I'm sure Paolo will agree.
What you so ominously  call "the group scheduling stuff" has been there and
shipping in mainline since day 1 of the BFQ merge, and it works fine in 5.0.

cheers
Holger

Re: [PATCH] block/bfq: fix ifdef for CONFIG_BFQ_GROUP_IOSCHED=y

[Jens Axboe]

On 3/29/19 10:12 AM, Holger Hoffstätte wrote:
> On 3/29/19 4:58 PM, Jens Axboe wrote:
>> On 3/29/19 9:56 AM, Holger Hoffstätte wrote:
>>> On 3/29/19 3:01 PM, Konstantin Khlebnikov wrote:
>>>> Replace BFQ_GROUP_IOSCHED_ENABLED with CONFIG_BFQ_GROUP_IOSCHED.
>>>> Code under these ifdefs never worked, something might be broken.
>>>>
>>>> Fixes: 0471559c2fbd ("block, bfq: add/remove entity weights correctly")
>>>> Fixes: 73d58118498b ("block, bfq: consider also ioprio classes in symmetry detection")
>>>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>>>> ---
>>>>    block/bfq-iosched.c |    2 +-
>>>>    block/bfq-wf2q.c    |    2 +-
>>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>>>> index 4c592496a16a..fac188dd78fa 100644
>>>> --- a/block/bfq-iosched.c
>>>> +++ b/block/bfq-iosched.c
>>>> @@ -674,7 +674,7 @@ static bool bfq_symmetric_scenario(struct bfq_data *bfqd)
>>>>    	 * at least two nodes.
>>>>    	 */
>>>>    	return !(varied_queue_weights || multiple_classes_busy
>>>> -#ifdef BFQ_GROUP_IOSCHED_ENABLED
>>>> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
>>>>    	       || bfqd->num_groups_with_pending_reqs > 0
>>>>    #endif
>>>>    		);
>>>> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
>>>> index 63311d1ff1ed..a11bef75483d 100644
>>>> --- a/block/bfq-wf2q.c
>>>> +++ b/block/bfq-wf2q.c
>>>> @@ -1012,7 +1012,7 @@ static void __bfq_activate_entity(struct bfq_entity *entity,
>>>>    		entity->on_st = true;
>>>>    	}
>>>>    
>>>> -#ifdef BFQ_GROUP_IOSCHED_ENABLED
>>>> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
>>>>    	if (!bfq_entity_to_bfqq(entity)) { /* bfq_group */
>>>>    		struct bfq_group *bfqg =
>>>>    			container_of(entity, struct bfq_group, entity);
>>>>
>>>>
>>>
>>> Good catch! I run without group scheduling and therefore didn't notice these
>>> stray defines earlier. For 5.1 it should merge cleanly; adding this on top of
>>> the pending 5.2 BFQ patches required a small context fixup in hunk #1 due to
>>> "block, bfq: do not idle for lowest-weight queues".
>>
>> I'm hesitant to apply this, since the group scheduling stuff has obviously never
>> been tested.
> 
> This is simply a regression in 5.1 caused by 73d58118498b - nothing else,
> and as such this fix needs to go into 5.1 as well. I'm sure Paolo will agree.
> What you so ominously  call "the group scheduling stuff" has been there and
> shipping in mainline since day 1 of the BFQ merge, and it works fine in 5.0.

If that's the case (I didn't check how far back it went), then yes, it should
of course go into 5.1.

The ominous nature of my reply I'll chalk up to your interpretation

-- 
Jens Axboe

Re: [PATCH] block/bfq: fix ifdef for CONFIG_BFQ_GROUP_IOSCHED=y

[Holger Hoffstätte]

On 3/29/19 5:15 PM, Jens Axboe wrote:
>>>> Good catch! I run without group scheduling and therefore didn't notice these
>>>> stray defines earlier. For 5.1 it should merge cleanly; adding this on top of
>>>> the pending 5.2 BFQ patches required a small context fixup in hunk #1 due to
>>>> "block, bfq: do not idle for lowest-weight queues".
>>>
>>> I'm hesitant to apply this, since the group scheduling stuff has obviously never
>>> been tested.
>>
>> This is simply a regression in 5.1 caused by 73d58118498b - nothing else,
>> and as such this fix needs to go into 5.1 as well. I'm sure Paolo will agree.
>> What you so ominously  call "the group scheduling stuff" has been there and
>> shipping in mainline since day 1 of the BFQ merge, and it works fine in 5.0.
> 
> If that's the case (I didn't check how far back it went), then yes, it should
> of course go into 5.1.

Yay.

> The ominous nature of my reply I'll chalk up to your interpretation

Fair enough ;)

A more interesting question is why upstream uses undefined defines
for patches. That's a first-rate self-grenade if I've ever seen one,
and obviously something that is easily missed. Paolo?

-h

Re: [PATCH] block/bfq: fix ifdef for CONFIG_BFQ_GROUP_IOSCHED=y

[Paolo Valente]

> Il giorno 29 mar 2019, alle ore 17:44, Holger Hoffstätte <holger@applied-asynchrony.com> ha scritto:
> 
> On 3/29/19 5:15 PM, Jens Axboe wrote:
>>>>> Good catch! I run without group scheduling and therefore didn't notice these
>>>>> stray defines earlier. For 5.1 it should merge cleanly; adding this on top of
>>>>> the pending 5.2 BFQ patches required a small context fixup in hunk #1 due to
>>>>> "block, bfq: do not idle for lowest-weight queues".
>>>> 
>>>> I'm hesitant to apply this, since the group scheduling stuff has obviously never
>>>> been tested.
>>> 
>>> This is simply a regression in 5.1 caused by 73d58118498b - nothing else,
>>> and as such this fix needs to go into 5.1 as well. I'm sure Paolo will agree.
>>> What you so ominously  call "the group scheduling stuff" has been there and
>>> shipping in mainline since day 1 of the BFQ merge, and it works fine in 5.0.
>> If that's the case (I didn't check how far back it went), then yes, it should
>> of course go into 5.1.
> 
> Yay.
> 
>> The ominous nature of my reply I'll chalk up to your interpretation
> 
> Fair enough ;)
> 
> A more interesting question is why upstream uses undefined defines
> for patches. That's a first-rate self-grenade if I've ever seen one,
> and obviously something that is easily missed. Paolo?
> 

Paolo feels a little bit ashamed for this mistake :)

This horrible typo may also be the cause of the crashes recently
reported on this list.  I've just asked to try this fix:
https://lore.kernel.org/linux-block/626EAE58-63C1-4ABA-9040-9D9A61F74A0D@linaro.org/T/

And yes, I agree that this fix should be applied to 5.1.  Thank you
Konstantin for spotting and removing this bomb.

Thanks,
Paolo

> -h