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

["Boy Wu (吳勃誼)" <Boy.Wu@mediatek.com>]

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