On Thu 16-05-24 16:27:25, Zhang Yi wrote: > On 2024/5/15 8:25, Jan Kara wrote: > > On Mon 13-05-24 15:21:19, Zhang Yi wrote: > > Also accessing j_commit_sequence without any > > lock is theoretically problematic wrt compiler optimization. You should have > > READ_ONCE() there and the places modifying j_commit_sequence need to use > > WRITE_ONCE(). > > > > Thanks for pointing this out, but I'm not sure if we have to need READ_ONCE() > here. IIUC, if we add READ_ONCE(), we could make sure to get the latest > j_commit_sequence, if not, there is a window (it might becomes larger) that > we could get the old value and jbd2_transaction_committed() could return false > even if the given transaction was just committed, but I think the window is > always there, so it looks like it is not a big problem, is that right? Well, all accesses to any memory should use READ_ONCE(), be protected by a lock, or use types that handle atomicity on assembly level (like atomic_t, or atomic bit operations and similar). Otherwise the compiler is free to assume the underlying memory cannot change and generate potentionally invalid code. In this case, I don't think realistically any compiler will do it but still it is a good practice and also it saves us from KCSAN warnings. If you want to know more details about possible problems, see tools/memory-model/Documentation/explanation.txt chapter "PLAIN ACCESSES AND DATA RACES". Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
