On 2020/1/7 上午10:30, Qu Wenruo wrote:
>
>
> 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()?
Typo, it's set_bit()/clear_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
