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@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>>  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);
>>  	}
>>  }
>>  
>