Re: r

[Josef Bacik <josef@toxicpanda.com>]

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