Messages in this thread | | | Date | Thu, 24 Feb 2022 12:17:11 -0800 | From | Omar Sandoval <> | Subject | Re: [PATCH v4] btrfs: add fs state details to error messages. |
| |
On Thu, Feb 24, 2022 at 03:09:08PM -0500, Sweet Tea Dorminy wrote: > > > All the other interactions with info->fs_state are test/set/clear_bit, > > > which treat the argument as volatile and are therefore safe to do from > > > multiple threads. Without the READ_ONCE (reading it as a volatile), > > > the compiler or cpu could turn the reads of info->fs_state in > > > for_each_set_bit() into writes of random stuff into info->fs_state, > > > potentially clearing the state bits or filling them with garbage. > > I'm not sure I'm missing something, but I find the above hard to > > believe. Concurrent access to a variable from multiple threads may not > > produce consistent results, but random writes should not happen when > > we're just reading. > > Maybe I've been reading too many articles about the things compilers are > technically allowed to do. But as per the following link, the C standard > does permit compilers inventing writes except to atomics and volatiles: > https://lwn.net/Articles/793253/#Invented%20Stores > > > > > > Even if this is right, it'd be rare, but it would be exceeding weird > > > for a message to be logged listing an error and then future messages > > > be logged without any such state, or with a random collection of > > > garbage states. > > How would that happen? The volatile keyword is only a compiler hint not > > to do optimizations on the variable, what actually happens on the CPU > > level depends if the instruction is locked or not, so different threads > > may read different bits. > > You seem to imply that once a variable is not used with volatile > > semantics, even just for read, the result could lead to random writes > > because it's otherwise undefined. > > Pretty much; once a variable is read without READ_ONCE, it's unsafe to write > a new value on another thread that depends on the old value. Imagine a > compiler which invents stores; then if you are both reading and setting a > variable 'a' on different threads, the following could happen: > > thread 1 (reads) thread 2 (modifies) > > reads a into tmp > > stores junk into a > > reads junk from a > > stores tmp into a > > writes junk | 2 to a > > > Now a contains junk indefinitely. > > > But if it's too theoretical, I'm happy to drop it and amend my paranoia > level.
I agree with Sweet Tea here. Even if it's very theoretical, it costs us nothing to do the "correct" thing here.
| |