On 2020/1/7 上午2:15, David Sterba wrote:
> On Sat, Jan 04, 2020 at 09:56:02PM +0800, Qu Wenruo wrote:
>> }
>>
>> +/*
>> + * Check if this subvolume tree has valid reloc(*) tree.
>> + *
>> + * *: Reloc tree after swap is considered dead, thus not considered as valid.
>> + * This is enough for most callers, as they don't distinguish dead reloc
>> + * root from no reloc root.
>> + * But should_ignore_root() below is a special case.
>> + */
>> +static bool have_reloc_root(struct btrfs_root *root)
>> +{
>> + smp_mb__before_atomic();
>
> That one should be the easiest, to get an up to date value of the bit,
> sync before reading it. Similar to smp_rmb.
Yep, if we just go plain rmb/wmb everything would be much easier to
understand.
But since full rmb/wmb is expensive and in this case we're only address
certain arches which doesn't follower Total Store Order, we still need
to use that variant.
One more question.
Why not use before_atomic() and after_atomic() to surround the
set_bit()/test_bit()?
If before and after acts as rmb/wmb, then we don't really need this
before_atomic call aroudn test_bit()
>
>> + if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
>> + return false;
>> + if (!root->reloc_root)
>> + return false;
>> + return true;
>> +}
>>
>> static int should_ignore_root(struct btrfs_root *root)
>> {
>> @@ -525,6 +542,11 @@ static int should_ignore_root(struct btrfs_root *root)
>> if (!test_bit(BTRFS_ROOT_REF_COWS, &root->state))
>> return 0;
>>
>> + /* This root has been merged with its reloc tree, we can ignore it */
>> + smp_mb__before_atomic();
>
> This could be replaced by have_reloc_root but the reloc_root has to be
> check twice in that function. Here it was slightly optimized as it
> partially opencodes have_reloc_root. For clarity and fewer standalone
> barriers using the helper might be better.
The problem is, have_reloc_root() returns false if either:
- DEAD_RELOC_TREE is set
- no reloc_root
What we really want is, if bit set, return 1, but if no reloc root,
return 0 instead.
So we can't use that helper at all.
>
>> + if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
>> + return 1;
>> +
>> reloc_root = root->reloc_root;
>> if (!reloc_root)
>> return 0;
>> @@ -1439,6 +1461,7 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans,
>> * The subvolume has reloc tree but the swap is finished, no need to
>> * create/update the dead reloc tree
>> */
>> + smp_mb__before_atomic();
>
> Another partial have_reloc_root, could be used here as well with
> additional reloc_tree check.
Same problem as should_ignore_root().
Or we need to do extra reloc_root out of the helper.
>
>> if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
>> return 0;
>>
>> @@ -1478,8 +1501,7 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
>> struct btrfs_root_item *root_item;
>> int ret;
>>
>> - if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state) ||
>> - !root->reloc_root)
>> + if (!have_reloc_root(root))
>> goto out;
>>
>> reloc_root = root->reloc_root;
>> @@ -1489,6 +1511,7 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
>> if (fs_info->reloc_ctl->merge_reloc_tree &&
>> btrfs_root_refs(root_item) == 0) {
>> set_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
>
> First set the bit, so anybody who properly uses barriers before checking
> the bit will see it set
Still the same question, why not use before and after version around
set_bit()/clear_bit() so test_bit() doesn't need extra before_atomic call?
>
>> + smp_mb__after_atomic();
>
> since the reloc_root pointer is not safe to be accessed since this point
>
>> __del_reloc_root(reloc_root);
>> }
>>
>> @@ -2202,6 +2225,7 @@ static int clean_dirty_subvols(struct reloc_control *rc)
>> ret = ret2;
>> }
>> clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
>
> This one looks misplaced and reverse, root->reloc_root is set to NULL a
> few lines before and the barrier must be between this and clear_bit.
Got the point.
> This was not in my proposed version, why did you change that?
I thought the clear_bit() must be visible for all later operations, thus
the after_atomic() is needed.
But forgot the reloc_root is set to NULL.
So in that case, I guess we need both barriers.
Thanks,
Qu
>
>
>
>> + smp_mb__after_atomic();
>> btrfs_put_fs_root(root);
>> } else {
>> /* Orphan reloc tree, just clean it up */
>> @@ -4717,7 +4741,7 @@ void btrfs_reloc_pre_snapshot(struct btrfs_pending_snapshot *pending,
>> struct btrfs_root *root = pending->root;
>> struct reloc_control *rc = root->fs_info->reloc_ctl;
>>
>> - if (!root->reloc_root || !rc)
>> + if (!rc || !have_reloc_root(root))
>> return;
>>
>> if (!rc->merge_reloc_tree)
>> @@ -4751,7 +4775,7 @@ int btrfs_reloc_post_snapshot(struct btrfs_trans_handle *trans,
>> struct reloc_control *rc = root->fs_info->reloc_ctl;
>> int ret;
>>
>> - if (!root->reloc_root || !rc)
>> + if (!rc || !have_reloc_root(root))
>> return 0;
>>
>> rc = root->fs_info->reloc_ctl;
>> --
>> 2.24.1
Attachment:
signature.asc
Description: OpenPGP digital signature
