Re: r

[David Sterba <dsterba@suse.cz>]

On Mon, Jan 06, 2020 at 11:33:51AM -0500, Josef Bacik wrote:
> This took me a minute to figure out, but from what I can tell you are doing the 
> mb's around the BTRFS_ROOT_DEAD_RELOC_TREE flag so that in clean_dirty_subvols() 
> where we clear the bit and then set root->reloc_root = NULL we are sure to 
> either see the bit or that reloc_root == NULL.
> 
> That's fine, but man all these random memory barriers around the bit messing 
> make 0 sense and confuse the issue, what we really want is the 
> smp_mb__after_atomic() in clean_dirty_subvols() and the smp_mb__before_atomic() 
> in have_reloc_root().

The barriers around test_bit are required, test_bit could be reordered
as it's not a RMW operation. I suggest reding docs/atomic_t.rst on that
topic.

> But instead since we really want to know the right answer for root->reloc_root, 
> and we clear that _before_ we clear the BTRFS_ROOT_DEAD_RELOC_TREE let's just do 
> READ_ONCE/WRITE_ONCE everywhere we access the reloc_root.  In fact you could just do

But READ/WRITE_ONCE don't guarantee CPU-ordering, only that compiler
will not reload the variable in case it's used more than once.

> static struct btrfs_root get_reloc_root(struct btrfs_root *root)
> {
> 	if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
> 		return NULL;
> 	return READ_ONCE(root->reloc_root);

Use of READ_ONCE has no effect here and produces the same buggy code as
we have now.

I sent the code to Qu in the previous discussion as work in progress,
with uncommented barriers, expecting that they will be documented in the
final version. So don't blame him, I should have not let barriers
reasoning left only on him. I'll comment under the patch.