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

[Nikolay Borisov <nborisov@suse.com>]

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?

> 
> Josef