Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Feb 2022 12:17:11 -0800
From:   Omar Sandoval <osandov@osandov.com>
To:     Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
Cc:     dsterba@suse.cz, Chris Mason <clm@fb.com>,
        Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>, linux-btrfs@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH v4] btrfs: add fs state details to error messages.
Message-ID: <Yhfnx21q51xSBl32@relinquished.localdomain>
References: <a059920460fe13f773fd9a2e870ceb9a8e3a105a.1645644489.git.sweettea-kernel@dorminy.me>
 <20220224132210.GS12643@twin.jikos.cz>
 <284ccc08-8de7-9188-19d8-20f4eda56cb4@dorminy.me>
 <20220224184231.GZ12643@twin.jikos.cz>
 <20f14d85-6a07-e66d-4711-c16c6930c2a3@dorminy.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20f14d85-6a07-e66d-4711-c16c6930c2a3@dorminy.me>
Precedence: bulk

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.