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

[Leon Hwang <leon.hwang@linux.dev>]

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