[PATCH v2] block/blk-iocost: read ioc_pd_stat params inside ioc->lock

[PATCH v2] block/blk-iocost: read ioc_pd_stat params inside ioc->lock

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

ioc_pd_stat() reads ioc->enabled, ioc->vtime_base_rate, and
iocg->last_stat without holding ioc->lock, which trips KCSAN since
ioc_adjust_base_vrate() and iocg_flush_stat_upward() write those
fields under ioc->lock.

Commit 35198e323001 ("blk-iocost: read params inside lock in sysfs
apis") fixed the same issue in ioc_qos_prfill() and
ioc_cost_model_prfill(), but ioc_pd_stat() was missed.

Add spin_lock_irqsave/irqrestore(&ioc->lock) around the reads.
Use irqsave/irqrestore instead of the irq variant used by
ioc_qos_prfill() because ioc_pd_stat() is called from
blkcg_print_stat() which already disables IRQs via
guard(spinlock_irq)(&blkcg->lock); an unconditional spin_unlock_irq
would wrongly re-enable them.

Fixes: 35198e323001 ("blk-iocost: read params inside lock in sysfs apis")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>

---

Changes in v2:
- Use spin_lock_irqsave/irqrestore instead of spin_lock_irq/irq,
  since the caller (blkcg_print_stat) may already have IRQs disabled.
  (Sashiko review)

Link: https://lore.kernel.org/all/20260802150913.31977-1-cui.tao@linux.dev/
---
 block/blk-iocost.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 8b2aeba2e1e3..977fe9ecff69 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3092,9 +3092,12 @@ static void ioc_pd_stat(struct blkg_policy_data *pd, struct seq_file *s)
 {
 	struct ioc_gq *iocg = pd_to_iocg(pd);
 	struct ioc *ioc = iocg->ioc;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ioc->lock, flags);
 
 	if (!ioc->enabled)
-		return;
+		goto out;
 
 	if (iocg->level == 0) {
 		unsigned vp10k = DIV64_U64_ROUND_CLOSEST(
@@ -3110,6 +3113,8 @@ static void ioc_pd_stat(struct blkg_policy_data *pd, struct seq_file *s)
 			iocg->last_stat.wait_us,
 			iocg->last_stat.indebt_us,
 			iocg->last_stat.indelay_us);
+out:
+	spin_unlock_irqrestore(&ioc->lock, flags);
 }
 
 static u64 ioc_weight_prfill(struct seq_file *sf, struct blkg_policy_data *pd,
-- 
2.43.0

Re: [PATCH v2] block/blk-iocost: read ioc_pd_stat params inside ioc->lock

[Tejun Heo]

On Mon, Aug 03, 2026 at 09:22:01PM +0800, Tao Cui wrote:
> @@ -3092,9 +3092,12 @@ static void ioc_pd_stat(struct blkg_policy_data *pd, struct seq_file *s)
>  {
>  	struct ioc_gq *iocg = pd_to_iocg(pd);
>  	struct ioc *ioc = iocg->ioc;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ioc->lock, flags);
>  
>  	if (!ioc->enabled)
> -		return;
> +		goto out;
>  
>  	if (iocg->level == 0) {
>  		unsigned vp10k = DIV64_U64_ROUND_CLOSEST(
> @@ -3110,6 +3113,8 @@ static void ioc_pd_stat(struct blkg_policy_data *pd, struct seq_file *s)
>  			iocg->last_stat.wait_us,
>  			iocg->last_stat.indebt_us,
>  			iocg->last_stat.indelay_us);
> +out:
> +	spin_unlock_irqrestore(&ioc->lock, flags);
>  }

There's nothing in this function that requires synchronized reads. Might as
well just annotate them with data_race(). Otherwise, this can lead to a lot
of lock operations when there are a lot of cgroups and high frequency stat
reads.

Thanks.

-- 
tejun

Re: [PATCH v2] block/blk-iocost: read ioc_pd_stat params inside ioc->lock

[Tao Cui]

在 2026/8/4 00:47, Tejun Heo 写道:
> On Mon, Aug 03, 2026 at 09:22:01PM +0800, Tao Cui wrote:
>> @@ -3092,9 +3092,12 @@ static void ioc_pd_stat(struct blkg_policy_data *pd, struct seq_file *s)
>>  {
>>  	struct ioc_gq *iocg = pd_to_iocg(pd);
>>  	struct ioc *ioc = iocg->ioc;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&ioc->lock, flags);
>>  
>>  	if (!ioc->enabled)
>> -		return;
>> +		goto out;
>>  
>>  	if (iocg->level == 0) {
>>  		unsigned vp10k = DIV64_U64_ROUND_CLOSEST(
>> @@ -3110,6 +3113,8 @@ static void ioc_pd_stat(struct blkg_policy_data *pd, struct seq_file *s)
>>  			iocg->last_stat.wait_us,
>>  			iocg->last_stat.indebt_us,
>>  			iocg->last_stat.indelay_us);
>> +out:
>> +	spin_unlock_irqrestore(&ioc->lock, flags);
>>  }
> 
> There's nothing in this function that requires synchronized reads. Might as
> well just annotate them with data_race(). Otherwise, this can lead to a lot
> of lock operations when there are a lot of cgroups and high frequency stat
> reads.
> 
Agreed, there's nothing in this function that requires synchronized
reads, these are all stat values where stale reads are harmless.
Will switch to data_race() in v3.

Thanks,
Tao

> Thanks.
>