Re: [PATCH v2] btrfs: Use btrfs_transaction::pinned_extents

[Josef Bacik <josef@toxicpanda.com>]

On 1/24/20 5:35 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>
> ---
> 
> V2:
>   * Properly release transaction in clean_pinned_extents [Josef]
>   * Only check for EXTENT_UPTODATE in add_new_free_space [Josef]
> 
>   fs/btrfs/block-group.c       | 43 +++++++++++++++++++++++-------------
>   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, 50 insertions(+), 75 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 48bb9e08f2e8..91054c32d88a 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -460,10 +460,9 @@ 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);
> +					    EXTENT_UPTODATE, NULL);
>   		if (ret)
>   			break;
> 
> @@ -1233,32 +1232,46 @@ 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;
> +	}
> +
> +	btrfs_put_transaction(prev_trans);

The failure case is missing the put.  Thanks,

Josef