Messages in this thread | | | Date | Thu, 24 Feb 2022 19:42:31 +0100 | From | David Sterba <> | Subject | Re: [PATCH v4] btrfs: add fs state details to error messages. |
| |
On Thu, Feb 24, 2022 at 11:10:59AM -0500, Sweet Tea Dorminy wrote: > Awesome, thank you. I realized this morning that it might be technically > slightly racy actually and would propose something like the following > > static void btrfs_state_to_string(const struct btrfs_fs_info *info, char *buf) > { > unsigned int bit; > + unsigned long fs_state = READ_ONCE(info->fs_state); > unsigned int states_printed = 0; > char *curr = buf; > > memcpy(curr, STATE_STRING_PREFACE, sizeof(STATE_STRING_PREFACE)); > curr += sizeof(STATE_STRING_PREFACE) - 1; > > - for_each_set_bit(bit, &info->fs_state, sizeof(info->fs_state)) { > + for_each_set_bit(bit, fs_state, sizeof(fs_state)) { > > > 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. The worst thing that could happen is a missing status bit reported, which is not a problem.
> 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.
| |