On Tue 22-03-22 12:09:08, Christoph Hellwig wrote: > On Wed, Mar 16, 2022 at 03:38:55PM +0100, Jan Kara wrote: > > Well, but another effect of READ_ONCE() / WRITE_ONCE() is that it > > effectively forces the compiler to not store any intermediate value in > > bd_openers. If you have code like bdev->bd_openers++, and bd_openers has > > value say 1, the compiler is fully within its rights if unlocked reader > > sees values, 1, 0, 3, 2. It would have to be a vicious compiler but the C > > standard allows that and some of the optimizations compilers end up doing > > result in code which is not far from this (read more about KCSAN and the > > motivation behind it for details). So data_race() annotation is *not* > > enough for unlocked bd_openers usage. > > > > > Use of atomic_t for lo->lo_disk->part0->bd_openers does not help, for > > > currently lo->lo_mutex is held in order to avoid races. That is, it is > > > disk->open_mutex which loop_clr_fd() needs to hold when accessing > > > lo->lo_disk->part0->bd_openers. > > > > It does help because with atomic_t, seeing any intermediate values is not > > possible even for unlocked readers. > > The Linux memory model guarantees atomic reads from 32-bit integers. > But if it makes everyone happier I could do a READ_ONCE here. Sure, the read is atomic wrt other CPU instructions, but it is not atomic wrt how the compiler decides to implement bdi->bd_openers++. So we need to make these bd_openers *updates* atomic so that the unlocked reads are really safe. That being said I consider the concerns mostly theoretical so I don't insist but some checker will surely complain sooner rather than later. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
