Hello, On Mon, Jul 08, 2024 at 02:00:42AM +0000, Boy Wu (吳勃誼) wrote: > When accessing /sys/fs/cgroup/io.stat, blkcg_print_stat will be called, > and there is a small chance that four processes on each CPU core are > accessing /sys/fs/cgroup/io.stat, which means four CPUs are calling > blkcg_print_stat. As a result, blkcg_fill_root_iostats will be called > simultaneously. However, u64_stats_update_begin_irqsave and > u64_stats_update_end_irqrestore are not protect by spin_locks, so there > is a small chance that the sync.seq.sequence will be an odd number > after u64_stats_update_end_irqrestore due to the concurrent CPUs acess, > because sync.seq.sequence plus one is not an atomic operation. > > do_raw_write_seqcount_begin(): > /usr/src/kernel/common/include/linux/seqlock.h:469 > c05e5cfc: e5963030 ldr r3, [r6, #48] ; 0x30 > c05e5d00: e2833001 add r3, r3, #1 > c05e5d04: e5863030 str r3, [r6, #48] ; 0x30 > /usr/src/kernel/common/include/linux/seqlock.h:470 > c05e5d08: f57ff05a dmb ishst > > do_raw_write_seqcount_end(): > /usr/src/kernel/common/include/linux/seqlock.h:489 > c05e5d30: f57ff05a dmb ishst > /usr/src/kernel/common/include/linux/seqlock.h:490 > c05e5d34: e5963030 ldr r3, [r6, #48] ; 0x30 > c05e5d38: e2833001 add r3, r3, #1 > c05e5d3c: e5863030 str r3, [r6, #48] ; 0x30 > > To prevent this problem, I added spin_locks in blkcg_fill_root_iostats, > and this solution works fine to me when I use the stress-ng --sysfs > test. Ah, okay, so, there are two usages of u64_sync synchronization - one is for blkg->iostat_cpu and the other for blkg->iostat. The former makes sense and is safe as there can ever be only one updater with irq disabled. The latter doesn't work as there can be multiple updaters and should be removed (and replaced with spinlock). There's already blkg_stat_lock which probably was added for a similar reason in __blkcg_rstat_flush(). Can you please update the patch to remove the u64_sync usage for blkg->iostat and replace it with blkg_stat_lock? Thanks. -- tejun
