[PATCH] blk-iolatency: use spin_lock_irqsave() in iolatency_clear_scaling()

[PATCH] blk-iolatency: use spin_lock_irqsave() in iolatency_clear_scaling()

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

child_lat.lock is acquired with spin_lock_irqsave() in both
iolatency_check_latencies() (called from the blkcg_iolatency_done_bio()
softirq path) and blkiolatency_timer_fn() (timer softirq).
iolatency_clear_scaling() instead uses a plain spin_lock(), which only
happens to be safe because both of its callers enter with interrupts
already disabled:

  * iolatency_set_limit() runs under queue_lock via blkg_conf_prep(),
    which returns with the lock held and interrupts disabled;
  * iolatency_pd_offline() runs under queue_lock from blkg_destroy() and
    blkcg_deactivate_policy(), both of which take it with spin_lock_irq().

Take the lock with spin_lock_irqsave()/spin_unlock_irqrestore() so the
locking is self-contained and consistent with the other two sites, instead
of relying on an undocumented caller precondition. No functional change.

Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 block/blk-iolatency.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
index cef02b6c5fa9..68b74258cd95 100644
--- a/block/blk-iolatency.c
+++ b/block/blk-iolatency.c
@@ -811,16 +811,17 @@ static void iolatency_clear_scaling(struct blkcg_gq *blkg)
 	if (blkg->parent) {
 		struct iolatency_grp *iolat = blkg_to_lat(blkg->parent);
 		struct child_latency_info *lat_info;
+		unsigned long flags;
 		if (!iolat)
 			return;
 
 		lat_info = &iolat->child_lat;
-		spin_lock(&lat_info->lock);
+		spin_lock_irqsave(&lat_info->lock, flags);
 		atomic_set(&lat_info->scale_cookie, DEFAULT_SCALE_COOKIE);
 		lat_info->last_scale_event = 0;
 		lat_info->scale_grp = NULL;
 		lat_info->scale_lat = 0;
-		spin_unlock(&lat_info->lock);
+		spin_unlock_irqrestore(&lat_info->lock, flags);
 	}
 }
 
-- 
2.43.0

Re: [PATCH] blk-iolatency: use spin_lock_irqsave() in iolatency_clear_scaling()

[Leon Hwang]

On 17/7/26 12:06, Tao Cui wrote:
> From: Tao Cui <cuitao@kylinos.cn>
> 
> child_lat.lock is acquired with spin_lock_irqsave() in both
> iolatency_check_latencies() (called from the blkcg_iolatency_done_bio()
> softirq path) and blkiolatency_timer_fn() (timer softirq).
> iolatency_clear_scaling() instead uses a plain spin_lock(), which only
> happens to be safe because both of its callers enter with interrupts
> already disabled:
> 
>   * iolatency_set_limit() runs under queue_lock via blkg_conf_prep(),
>     which returns with the lock held and interrupts disabled;
>   * iolatency_pd_offline() runs under queue_lock from blkg_destroy() and
>     blkcg_deactivate_policy(), both of which take it with spin_lock_irq().
> 
> Take the lock with spin_lock_irqsave()/spin_unlock_irqrestore() so the
> locking is self-contained and consistent with the other two sites, instead
> of relying on an undocumented caller precondition. No functional change.
> 
> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> ---
>  block/blk-iolatency.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
> index cef02b6c5fa9..68b74258cd95 100644
> --- a/block/blk-iolatency.c
> +++ b/block/blk-iolatency.c
> @@ -811,16 +811,17 @@ static void iolatency_clear_scaling(struct blkcg_gq *blkg)
>  	if (blkg->parent) {
>  		struct iolatency_grp *iolat = blkg_to_lat(blkg->parent);
>  		struct child_latency_info *lat_info;
> +		unsigned long flags;
>  		if (!iolat)
>  			return;
>  
>  		lat_info = &iolat->child_lat;
> -		spin_lock(&lat_info->lock);
> +		spin_lock_irqsave(&lat_info->lock, flags);

Better to use guard(spinlock_irqsave)(&lat_info->lock).

Thanks,
Leon

>  		atomic_set(&lat_info->scale_cookie, DEFAULT_SCALE_COOKIE);
>  		lat_info->last_scale_event = 0;
>  		lat_info->scale_grp = NULL;
>  		lat_info->scale_lat = 0;
> -		spin_unlock(&lat_info->lock);
> +		spin_unlock_irqrestore(&lat_info->lock, flags);
>  	}
>  }
>  

Re: [PATCH] blk-iolatency: use spin_lock_irqsave() in iolatency_clear_scaling()

[Tao Cui]

在 2026/7/17 13:36, Leon Hwang 写道:
> On 17/7/26 12:06, Tao Cui wrote:
>> From: Tao Cui <cuitao@kylinos.cn>
>>
>> child_lat.lock is acquired with spin_lock_irqsave() in both
>> iolatency_check_latencies() (called from the blkcg_iolatency_done_bio()
>> softirq path) and blkiolatency_timer_fn() (timer softirq).
>> iolatency_clear_scaling() instead uses a plain spin_lock(), which only
>> happens to be safe because both of its callers enter with interrupts
>> already disabled:
>>
>>   * iolatency_set_limit() runs under queue_lock via blkg_conf_prep(),
>>     which returns with the lock held and interrupts disabled;
>>   * iolatency_pd_offline() runs under queue_lock from blkg_destroy() and
>>     blkcg_deactivate_policy(), both of which take it with spin_lock_irq().
>>
>> Take the lock with spin_lock_irqsave()/spin_unlock_irqrestore() so the
>> locking is self-contained and consistent with the other two sites, instead
>> of relying on an undocumented caller precondition. No functional change.
>>
>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
>> ---
>>  block/blk-iolatency.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
>> index cef02b6c5fa9..68b74258cd95 100644
>> --- a/block/blk-iolatency.c
>> +++ b/block/blk-iolatency.c
>> @@ -811,16 +811,17 @@ static void iolatency_clear_scaling(struct blkcg_gq *blkg)
>>  	if (blkg->parent) {
>>  		struct iolatency_grp *iolat = blkg_to_lat(blkg->parent);
>>  		struct child_latency_info *lat_info;
>> +		unsigned long flags;
>>  		if (!iolat)
>>  			return;
>>  
>>  		lat_info = &iolat->child_lat;
>> -		spin_lock(&lat_info->lock);
>> +		spin_lock_irqsave(&lat_info->lock, flags);
> 
> Better to use guard(spinlock_irqsave)(&lat_info->lock).
> 

Good point, thanks.  Will switch to guard(spinlock_irqsave)() in v2.

Thanks,
Tao
> Thanks,
> Leon
> 
>>  		atomic_set(&lat_info->scale_cookie, DEFAULT_SCALE_COOKIE);
>>  		lat_info->last_scale_event = 0;
>>  		lat_info->scale_grp = NULL;
>>  		lat_info->scale_lat = 0;
>> -		spin_unlock(&lat_info->lock);
>> +		spin_unlock_irqrestore(&lat_info->lock, flags);
>>  	}
>>  }
>>  
>