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.
