Re: [PATCH v4] btrfs: add fs state details to error messages.

[David Sterba <dsterba@suse.cz>]

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.