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@xxxxxxxx>
>
> 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
