[PATCH v2] blk-cgroup: Replace u64_sync with blkg_stat_lock for stats update

[PATCH v2] blk-cgroup: Replace u64_sync with blkg_stat_lock for stats update

[boy.wu]

From: Boy Wu <boy.wu@mediatek.com>

In 32bit SMP systems, if multiple CPUs call blkcg_print_stat,
which may cause blkcg_fill_root_iostats to have a concurrent problem
on the seqlock in u64_stats_update, which will cause a deadlock
on u64_stats_fetch_begin in blkcg_print_one_stat.

Thus use blkg_stat_lock to replace u64_sync.

Fixes: ef45fe470e1e ("blk-cgroup: show global disk stats in root cgroup io.stat")
Signed-off-by: Boy Wu <boy.wu@mediatek.com>
---
Change in v2:
 - update commit message
 - Remove u64_sync
 - Replace spin_lock_irq with guard statement
 - Replace blkg->q->queue_lock with blkg_stat_lock
---
 block/blk-cgroup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 85b3b9051455..18b47ee1a640 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -952,7 +952,6 @@ static void blkcg_fill_root_iostats(void)
 		struct blkcg_gq *blkg = bdev->bd_disk->queue->root_blkg;
 		struct blkg_iostat tmp;
 		int cpu;
-		unsigned long flags;
 
 		memset(&tmp, 0, sizeof(tmp));
 		for_each_possible_cpu(cpu) {
@@ -974,9 +973,10 @@ static void blkcg_fill_root_iostats(void)
 				cpu_dkstats->sectors[STAT_DISCARD] << 9;
 		}
 
-		flags = u64_stats_update_begin_irqsave(&blkg->iostat.sync);
+#if BITS_PER_LONG == 32
+		guard(raw_spinlock_irqsave)(&blkg_stat_lock);
+#endif
 		blkg_iostat_set(&blkg->iostat.cur, &tmp);
-		u64_stats_update_end_irqrestore(&blkg->iostat.sync, flags);
 	}
 }
 
-- 
2.18.0

Re: [PATCH v2] blk-cgroup: Replace u64_sync with blkg_stat_lock for stats update

[Tejun Heo]

Hello,

On Wed, Jul 10, 2024 at 02:13:34PM +0800, boy.wu wrote:
...
> @@ -952,7 +952,6 @@ static void blkcg_fill_root_iostats(void)
>  		struct blkcg_gq *blkg = bdev->bd_disk->queue->root_blkg;
>  		struct blkg_iostat tmp;
>  		int cpu;
> -		unsigned long flags;
>  
>  		memset(&tmp, 0, sizeof(tmp));
>  		for_each_possible_cpu(cpu) {
> @@ -974,9 +973,10 @@ static void blkcg_fill_root_iostats(void)
>  				cpu_dkstats->sectors[STAT_DISCARD] << 9;
>  		}
>  
> -		flags = u64_stats_update_begin_irqsave(&blkg->iostat.sync);
> +#if BITS_PER_LONG == 32
> +		guard(raw_spinlock_irqsave)(&blkg_stat_lock);
> +#endif
>  		blkg_iostat_set(&blkg->iostat.cur, &tmp);
> -		u64_stats_update_end_irqrestore(&blkg->iostat.sync, flags);

Isn't the problem shared across other blkg->iostat.sync users too? Also,
maybe, we can just grab the spinlock without testing for 32bit. blkg->iostat
(unlike the per-cpu counterpart) isn't accessed that frequently, so keeping
it simple and consistent probably makes more sense, right?

Thanks.

-- 
tejun

Re: [PATCH v2] blk-cgroup: Replace u64_sync with blkg_stat_lock for stats update

[Boy Wu (吳勃誼)]

On Wed, 2024-07-10 at 12:12 -1000, Tejun Heo wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hello,
> 
> On Wed, Jul 10, 2024 at 02:13:34PM +0800, boy.wu wrote:
> ...
> > @@ -952,7 +952,6 @@ static void blkcg_fill_root_iostats(void)
> >  struct blkcg_gq *blkg = bdev->bd_disk->queue->root_blkg;
> >  struct blkg_iostat tmp;
> >  int cpu;
> > -unsigned long flags;
> >  
> >  memset(&tmp, 0, sizeof(tmp));
> >  for_each_possible_cpu(cpu) {
> > @@ -974,9 +973,10 @@ static void blkcg_fill_root_iostats(void)
> >  cpu_dkstats->sectors[STAT_DISCARD] << 9;
> >  }
> >  
> > -flags = u64_stats_update_begin_irqsave(&blkg->iostat.sync);
> > +#if BITS_PER_LONG == 32
> > +guard(raw_spinlock_irqsave)(&blkg_stat_lock);
> > +#endif
> >  blkg_iostat_set(&blkg->iostat.cur, &tmp);
> > -u64_stats_update_end_irqrestore(&blkg->iostat.sync, flags);
> 
> Isn't the problem shared across other blkg->iostat.sync users too? 

There are three places for iostat update sync.
1. The sync in blkcg_iostats_update is already protected by
blkg_stat_lock.
2. The sync in blkcg_fill_root_iostats is where we added it in the
patch.
3. The sync in blk_cgroup_bio_start is per CPU, and I don't think it
causes a concurrent problem.

> Also,
> maybe, we can just grab the spinlock without testing for 32bit. blkg-
> >iostat
> (unlike the per-cpu counterpart) isn't accessed that frequently, so
> keeping
> it simple and consistent probably makes more sense, right?

I can remove the 32bit only define, but I think we need to add back the
u64 sync, because the spin lock and the u64 sync serve different
purposes. The spin lock is for handling concurrent problems from
different CPUs updating stats, and u64 sync is for updating 64 bits
data and fetching 64 bits data from different CPUs in 32bit SMP
systems.

> 
> Thanks.
> 
> -- 
> tejun

--
boy.wu

Re: [PATCH v2] blk-cgroup: Replace u64_sync with blkg_stat_lock for stats update

[tj]

Hello,

On Thu, Jul 11, 2024 at 02:25:29AM +0000, Boy Wu (吳勃誼) wrote:
...
> I can remove the 32bit only define, but I think we need to add back the
> u64 sync, because the spin lock and the u64 sync serve different
> purposes. The spin lock is for handling concurrent problems from
> different CPUs updating stats, and u64 sync is for updating 64 bits
> data and fetching 64 bits data from different CPUs in 32bit SMP
> systems.

Hmm... so what I'm suggesting is using u64_sync for the per-cpu stat
structure as they are guaranteed to have only one updater with irq disabled
and use a spinlock for the shared iostat which can have multiple updaters
and isn't accessed that frequently.

Thanks.

-- 
tejun

Re: [PATCH v2] blk-cgroup: Replace u64_sync with blkg_stat_lock for stats update

[Boy Wu (吳勃誼)]

On Thu, 2024-07-11 at 11:02 -1000, tj@kernel.org wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hello,
> 
> On Thu, Jul 11, 2024 at 02:25:29AM +0000, Boy Wu (吳勃誼) wrote:
> ...
> > I can remove the 32bit only define, but I think we need to add back
> the
> > u64 sync, because the spin lock and the u64 sync serve different
> > purposes. The spin lock is for handling concurrent problems from
> > different CPUs updating stats, and u64 sync is for updating 64 bits
> > data and fetching 64 bits data from different CPUs in 32bit SMP
> > systems.
> 
> Hmm... so what I'm suggesting is using u64_sync for the per-cpu stat
> structure as they are guaranteed to have only one updater with irq
> disabled
> and use a spinlock for the shared iostat which can have multiple
> updaters
> and isn't accessed that frequently.
> 
> Thanks.
> 
> -- 
> tejun

I agree, but for multiple updaters, we not only need a spin lock but
also need u64_sync for 32bit SMP systems because u64_stats_fetch is not
protected by the spin lock blkg_stat_lock. If removing u64 sync, then
one CPU fetches data while another CPU is updating, may get a 64 bits
data with only 32 bits updated, while the other 32 bits are not updated
yet. We can see that blkcg_iostats_update is protected by both u64_sync
and the spin lock blkg_stat_lock in __blkcg_rstat_flush.
Thus, I think we should keep the u64_sync and just add the spin
lock blkg_stat_lock, not replace u64_sync with the spin lock.

--
Boy.Wu

Re: [PATCH v2] blk-cgroup: Replace u64_sync with blkg_stat_lock for stats update

[tj]

Hello, Boy.

On Fri, Jul 12, 2024 at 01:39:51AM +0000, Boy Wu (吳勃誼) wrote:
...
> I agree, but for multiple updaters, we not only need a spin lock but
> also need u64_sync for 32bit SMP systems because u64_stats_fetch is not
> protected by the spin lock blkg_stat_lock. If removing u64 sync, then
> one CPU fetches data while another CPU is updating, may get a 64 bits
> data with only 32 bits updated, while the other 32 bits are not updated
> yet. We can see that blkcg_iostats_update is protected by both u64_sync
> and the spin lock blkg_stat_lock in __blkcg_rstat_flush.
> Thus, I think we should keep the u64_sync and just add the spin
> lock blkg_stat_lock, not replace u64_sync with the spin lock.

I don't get it. The only reader of blkg->iostat is blkcg_print_one_stat().
It can just grab the same spin lock that the updaters use, right? Why do we
also need u64_sync for blkg->iostat?

Thanks.

-- 
tejun

Re: [PATCH v2] blk-cgroup: Replace u64_sync with blkg_stat_lock for stats update

[Boy Wu (吳勃誼)]

On Fri, 2024-07-12 at 08:38 -1000, tj@kernel.org wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hello, Boy.
> 
> On Fri, Jul 12, 2024 at 01:39:51AM +0000, Boy Wu (吳勃誼) wrote:
> ...
> > I agree, but for multiple updaters, we not only need a spin lock
> but
> > also need u64_sync for 32bit SMP systems because u64_stats_fetch is
> not
> > protected by the spin lock blkg_stat_lock. If removing u64 sync,
> then
> > one CPU fetches data while another CPU is updating, may get a 64
> bits
> > data with only 32 bits updated, while the other 32 bits are not
> updated
> > yet. We can see that blkcg_iostats_update is protected by both
> u64_sync
> > and the spin lock blkg_stat_lock in __blkcg_rstat_flush.
> > Thus, I think we should keep the u64_sync and just add the spin
> > lock blkg_stat_lock, not replace u64_sync with the spin lock.
> 
> I don't get it. The only reader of blkg->iostat is
> blkcg_print_one_stat().
> It can just grab the same spin lock that the updaters use, right? Why
> do we
> also need u64_sync for blkg->iostat?
> 
> Thanks.
> 
> -- 
> tejun

I think I get your idea. You want to replace all the u64 sync for
iostat. However, I have one question: why use blkg_stat_lock instead of
adding a spin lock for each iostat like iostat.spinlock? We don't need
to lock between updating different iostats, but only lock when updating
the same iostat.

--
Boy.Wu

Re: [PATCH v2] blk-cgroup: Replace u64_sync with blkg_stat_lock for stats update

[tj]

Hello,

On Mon, Jul 15, 2024 at 07:15:24AM +0000, Boy Wu (吳勃誼) wrote:
> I think I get your idea. You want to replace all the u64 sync for
> iostat. However, I have one question: why use blkg_stat_lock instead of
> adding a spin lock for each iostat like iostat.spinlock? We don't need
> to lock between updating different iostats, but only lock when updating
> the same iostat.

Oh yeah, that'd be even better.

Thanks.

-- 
tejun