Re: [PATCH 11/11] btrfs: Use btrfs_transaction::pinned_extents

[Josef Bacik <josef@toxicpanda.com>]

On 1/23/20 3:54 AM, Nikolay Borisov wrote:
> 
> 
> On 22.01.20 г. 22:21 ч., Josef Bacik wrote:
>> On 1/20/20 9:09 AM, Nikolay Borisov wrote:
>>> This commit flips the switch to start tracking/processing pinned
>>> extents on a per-transaction basis. It mostly replaces all references
>>> from btrfs_fs_info::(pinned_extents|freed_extents[]) to
>>> btrfs_transaction::pinned_extents. Two notable modifications that
>>> warrant explicit mention are changing clean_pinned_extents to get a
>>> reference to the previously running transaction. The other one is
>>> removal of call to btrfs_destroy_pinned_extent since transactions are
>>> going to be cleaned in btrfs_cleanup_one_transaction.
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>
>> I'd prefer if the excluded extent changes were separate from the pinned
>> extent changes.
>>
>>> ---
>>>    fs/btrfs/block-group.c       | 38 ++++++++++++++++++++++++------------
>>>    fs/btrfs/ctree.h             |  4 ++--
>>>    fs/btrfs/disk-io.c           | 30 +++++-----------------------
>>>    fs/btrfs/extent-io-tree.h    |  3 +--
>>>    fs/btrfs/extent-tree.c       | 31 ++++++++---------------------
>>>    fs/btrfs/free-space-cache.c  |  2 +-
>>>    fs/btrfs/tests/btrfs-tests.c |  7 ++-----
>>>    fs/btrfs/transaction.c       |  1 +
>>>    fs/btrfs/transaction.h       |  1 +
>>>    include/trace/events/btrfs.h |  3 +--
>>>    10 files changed, 47 insertions(+), 73 deletions(-)
>>>
>>> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
>>> index 48bb9e08f2e8..562dfb7dc77f 100644
>>> --- a/fs/btrfs/block-group.c
>>> +++ b/fs/btrfs/block-group.c
>>> @@ -460,7 +460,7 @@ u64 add_new_free_space(struct btrfs_block_group
>>> *block_group, u64 start, u64 end
>>>        int ret;
>>>          while (start < end) {
>>> -        ret = find_first_extent_bit(info->pinned_extents, start,
>>> +        ret = find_first_extent_bit(&info->excluded_extents, start,
>>>                            &extent_start, &extent_end,
>>>                            EXTENT_DIRTY | EXTENT_UPTODATE,
>>>                            NULL);
>>
>> We're no longer doing EXTENT_DIRTY in excluded_extents, so we don't need
>> this part.
>>
>>> @@ -1233,32 +1233,44 @@ static int inc_block_group_ro(struct
>>> btrfs_block_group *cache, int force)
>>>        return ret;
>>>    }
>>>    -static bool clean_pinned_extents(struct btrfs_block_group *bg)
>>> +static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
>>> +                 struct btrfs_block_group *bg)
>>>    {
>>>        struct btrfs_fs_info *fs_info = bg->fs_info;
>>> +    struct btrfs_transaction *prev_trans = NULL;
>>>        u64 start = bg->start;
>>>        u64 end = start + bg->length - 1;
>>>        int ret;
>>>    +    spin_lock(&fs_info->trans_lock);
>>> +    if (trans->transaction->list.prev != &fs_info->trans_list) {
>>> +        prev_trans = list_entry(trans->transaction->list.prev,
>>> +                    struct btrfs_transaction, list);
>>> +        refcount_inc(&prev_trans->use_count);
>>> +    }
>>> +    spin_unlock(&fs_info->trans_lock);
>>> +
>>>        /*
>>>         * Hold the unused_bg_unpin_mutex lock to avoid racing with
>>>         * btrfs_finish_extent_commit(). If we are at transaction N,
>>>         * another task might be running finish_extent_commit() for the
>>>         * previous transaction N - 1, and have seen a range belonging
>>> -     * to the block group in freed_extents[] before we were able to
>>> -     * clear the whole block group range from freed_extents[]. This
>>> +     * to the block group in pinned_extents before we were able to
>>> +     * clear the whole block group range from pinned_extents. This
>>>         * means that task can lookup for the block group after we
>>> -     * unpinned it from freed_extents[] and removed it, leading to
>>> +     * unpinned it from pinned_extents[] and removed it, leading to
>>>         * a BUG_ON() at unpin_extent_range().
>>>         */
>>>        mutex_lock(&fs_info->unused_bg_unpin_mutex);
>>> -    ret = clear_extent_bits(&fs_info->freed_extents[0], start, end,
>>> -              EXTENT_DIRTY);
>>> -    if (ret)
>>> -        goto failure;
>>> +    if (prev_trans) {
>>> +        ret = clear_extent_bits(&prev_trans->pinned_extents, start, end,
>>> +                    EXTENT_DIRTY);
>>> +        if (ret)
>>> +            goto failure;
>>> +    }
>>
>> You are leaking a ref to prev_trans here.
>>
>> <snip>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index 9209c7b0997c..3cb786463eb2 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -2021,10 +2021,8 @@ void btrfs_free_fs_roots(struct btrfs_fs_info
>>> *fs_info)
>>>                btrfs_drop_and_free_fs_root(fs_info, gang[i]);
>>>        }
>>>    -    if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
>>> +    if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
>>>            btrfs_free_log_root_tree(NULL, fs_info);
>>> -        btrfs_destroy_pinned_extent(fs_info, fs_info->pinned_extents);
>>> -    }
>>
>> What about the excluded extents?  We may never cache the block group
>> with one of the super mirrors in it, and thus we would leak the excluded
>> extent for it.  Thanks,
> 
> btrfs_destroy_pinned_extent didn't touch EXTENT_UPDATE (excluded
> extents) before so my removing this call doesn't change that. E.g. if
> there is a bug here where excluded extents are not cleaned up then it's
> not due to my code.
> 
> On the other hand I don't quite understand your concern w.r.t pinned
> extents. Can you elaborate?
> 

Sorry thunderbird ate my followup, we drop the excluded extents in 
btrfs_free_block_groups() if they are never cached, so you are fine here.  Thanks,

Josef