Re: [PATCH v2] btrfs: relocation: Fix KASAN reports caused by extended reloc tree lifespan

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2020/1/8 下午11:11, David Sterba wrote:
> On Wed, Jan 08, 2020 at 04:08:41PM +0100, David Sterba wrote:
>>>>> +static bool have_reloc_root(struct btrfs_root *root)
>>>>> +{
>>>>> +    if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
>>>>> +        return false;
>>>>
>>>> You still need a smp_mb__after_atomic() here, test_bit is unordered.
>>>
>>> Nope, that won't do anything, since smp_mb__(After|before)_atomic only
>>> orders RMW operations and test_bit is not an RMW operation. From
>>> atomic_bitops.txt:
>>>
>>>
>>> Non-RMW ops:
>>>
>>>
>>>
>>>   test_bit()
>>>
>>> Furthermore from atomic_t.txt:
>>>
>>> The barriers:
>>>
>>>
>>>
>>>   smp_mb__{before,after}_atomic()
>>>
>>>
>>>
>>> only apply to the RMW atomic ops and can be used to augment/upgrade the
>>>
>>> ordering inherent to the op.
>>
>> The way I read it is more like smp_rmb/smp_wmb, but for bits in this
>> case, so the smp_mb__before/after_atomic was only a syntactic sugar to
>> match that it's atomic bitops. I realize this could have caused some
>> confusion, however I still think that some sort of barrier is needed.
>
> There's an existing pattern used for serializing set/clear of
> BTRFS_ROOT_IN_TRANS_SETUP (record_root_in_trans,
> btrfs_record_root_in_trans).
>
> Once upon a time there were barriers like smp_mb__before_clear_bit but
> they got unified to just smp_mb__before_atomic for all set/clear
> operations, so I believe I was not all wrong with using them.
>
I used to believe the same fairy tail, that mb() works like a flush(),
but we're not living in a fairy tail.

What mb really does is keep certain ordering from happening.
And ordering means, we have at least *2* different memory accesses.


It's not to ensure every reader get the same result, as there is no way
to do that. Read can happen whenever they want.


So before we talk about mb, we first need to know which 2 memory
accesses we're talking about.
If there is even no 2 memory access, then there is no need for mb().

E.g. for the mb implied by spinlock(), it's not to ensure the spinlock
counter reader, but to ensure the memory ordering between the spinlock
counter itself and the memory it's protecting.

So for memory barrier around test_bit(), as long as the compiler is not
doing reordering, we don't need extra mb.

And if the compiler is really doing re-ordering for the
have_reloc_root(), then the compiler is doing something wrong, as that
would makes the test_bit() meaningless.

Thanks,
Qu