Date:   Mon, 28 Feb 2022 17:48:10 +0100
From:   David Sterba <dsterba@...e.cz>
To:     Sweet Tea Dorminy <sweettea-kernel@...miny.me>
Cc:     dsterba@...e.cz, Chris Mason <clm@...com>,
        Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-team@...com
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

There's a difference between technically allowed to do and actually
doing it. The article sections says that no such compiler is known with
some exception of an old one that got fixed. Also as I read the example
of the invented write, it's very specific and I don't se how it applies
here at all.

> >> 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. 

You're giving 'volatile' too much credit for guaranteeing safety for
interprocess operation, it's just a compiler hint to avoid
optimizations, it's not a synchronization primitive. In some cases
lockless reads are safe, as long as there are no updates or potential
reloads between reads that could be changed by another thread. We use
READ_ONCE in many cases, compiler in most cases generates the same
instructions as without it and it's merely an annotation and a way of
syntactic comment to denote special handling.

> 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 we're only reading here, that why I don't see any real risk. We read
a variable, potentially changed from other thread, and actually changed
by atomic bit operations. Where are the invented writes going to come
from, can't we rely on the bit handling primitives?

If you're example above would be real, we'd either lack proper locking
or there's a serious problem with compiler that would blow up in many
places (like inventing newly set bits in flags or random values
appearing in regular variables).

> But if it's too theoretical, I'm happy to drop it and amend my paranoia 
> level.

I think it's highly theoretical but the READ_ONCE should be there as a
matter of clean code practice, so I've updated the code.

Powered by blists - more mailing lists