On 1/6/20 1:04 PM, David Sterba wrote:
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.
Hmm didn't follow smp_read_barrier_depends() all the way down, I assumed it at
least protected from re-ordering. Looks like it only does something on alpha.
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.
There's still just too many of them, like I said before we're only worried about
either BTRFS_ROOT_DEAD_RELOC_TREE or !root->reloc_root. So I guess instead do
something like
static struct btrfs_root *get_reloc_root(struct btrfs_root *root)
{
if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
return NULL;
smp_mb__after_atomic();
return root->reloc_root;
}
And then in clean_dirty_subvols() do the smp_mb__before_atomic() before the
clear_bit. There's no reason for the random mb's around the other test_bit's.
Thanks,
Josef
