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.
