On 2020/1/4 上午12:15, David Sterba wrote:
> On Fri, Jan 03, 2020 at 04:52:59PM +0100, David Sterba wrote:
>> So it's one bit vs refcount and a lock. For the backports I'd go with
>> the bit, but this needs the barriers as mentioned in my previous reply.
>> Can you please update the patches?
>
> The idea is in the diff below (compile tested only). I found one more
> case that was not addressed by your patches, it's in
> btrfs_update_reloc_root.
But fix in btrfs_update_reloc_root() is already included in commit
d2311e698578 ("btrfs: relocation: Delay reloc tree deletion after
merge_reloc_roots").
Or would you mind to share more details about the missing check?
>
> Given that the type of the fix is the same, I'd rather do that in one
> patch. The reported stack traces are more or less the same.
To merge them into patch set is no problem, and should make backports a
little easier.
But I still didn't understand the barrier part.
If we're relying on that bit operation before accessing reloc_root, it
should be safe enough, even without memory barrier.
Would you please explain a little more?
Thanks,
Qu
>
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index af4dd49a71c7..aeba3a7506e1 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -517,6 +517,15 @@ static int update_backref_cache(struct btrfs_trans_handle *trans,
> return 1;
> }
>
> +static bool have_reloc_root(struct btrfs_root *root)
> +{
> + smp_mb__before_atomic();
> + 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,9 +534,9 @@ static int should_ignore_root(struct btrfs_root *root)
> if (!test_bit(BTRFS_ROOT_REF_COWS, &root->state))
> return 0;
>
> - reloc_root = root->reloc_root;
> - if (!reloc_root)
> + if (!have_reloc_root(root))
> return 0;
> + reloc_root = root->reloc_root;
>
> if (btrfs_root_last_snapshot(&reloc_root->root_item) ==
> root->fs_info->running_transaction->transid - 1)
> @@ -1439,6 +1448,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();
> if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
> return 0;
>
> @@ -1478,8 +1488,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 +1498,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);
> + smp_mb__after_atomic();
> __del_reloc_root(reloc_root);
> }
>
> @@ -2201,6 +2211,7 @@ static int clean_dirty_subvols(struct reloc_control *rc)
> if (ret2 < 0 && !ret)
> ret = ret2;
> }
> + smp_mb__before_atomic();
> clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
> btrfs_put_fs_root(root);
> } else {
> @@ -4730,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)
> @@ -4764,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;
>
Attachment:
signature.asc
Description: OpenPGP digital signature
