Re: [PATCH v4 2/4] btrfs: Refactor clustered extent allocation into find_free_extent_clustered()

[David Sterba <dsterba@suse.cz>]

On Wed, Oct 17, 2018 at 02:56:04PM +0800, Qu Wenruo wrote:
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -7261,6 +7261,115 @@ struct find_free_extent_ctl {
>  	u64 found_offset;
>  };
>  
> +

No extra newlines.

> +/*
> + * Helper function for find_free_extent().
> + *
> + * Return -ENOENT to inform caller that we need fallback to unclustered mode.
> + * Return -EAGAIN to inform caller that we need to re-search this block group
> + * Return >0 to inform caller that we find nothing
> + * Return 0 means we have found a location and set ffe_ctl->found_offset.
> + */
> +static int find_free_extent_clustered(struct btrfs_block_group_cache *bg,
> +		struct btrfs_free_cluster *last_ptr,
> +		struct find_free_extent_ctl *ffe_ctl,
> +		struct btrfs_block_group_cache **cluster_bg_ret)
> +{
> +	struct btrfs_fs_info *fs_info = bg->fs_info;
> +	struct btrfs_block_group_cache *cluster_bg;
> +	u64 aligned_cluster;
> +	u64 offset;
> +	int ret;
> +
> +	cluster_bg = btrfs_lock_cluster(bg, last_ptr, ffe_ctl->delalloc);
> +	if (!cluster_bg)
> +		goto refill_cluster;
> +	if (cluster_bg != bg && (cluster_bg->ro ||
> +	    !block_group_bits(cluster_bg, ffe_ctl->flags)))
> +		goto release_cluster;
> +
> +	offset = btrfs_alloc_from_cluster(cluster_bg, last_ptr,
> +			ffe_ctl->num_bytes, cluster_bg->key.objectid,
> +			&ffe_ctl->max_extent_size);
> +	if (offset) {
> +		/* we have a block, we're done */
> +		spin_unlock(&last_ptr->refill_lock);
> +		trace_btrfs_reserve_extent_cluster(cluster_bg,
> +				ffe_ctl->search_start, ffe_ctl->num_bytes);
> +		*cluster_bg_ret = cluster_bg;
> +		ffe_ctl->found_offset = offset;
> +		return 0;
> +	}
> +	WARN_ON(last_ptr->block_group != cluster_bg);
> +release_cluster:
> +	/* If we are on LOOP_NO_EMPTY_SIZE, we can't set up a new clusters, so

If you move or update comment that does not follow our preferred style,
please fix it.

I'll fix both and maybe more that I find while committing the patches.

> +	 * lets just skip it and let the allocator find whatever block it can
> +	 * find. If we reach this point, we will have tried the cluster
> +	 * allocator plenty of times and not have found anything, so we are
> +	 * likely way too fragmented for the clustering stuff to find anything.
> +	 *
> +	 * However, if the cluster is taken from the current block group,
> +	 * release the cluster first, so that we stand a better chance of
> +	 * succeeding in the unclustered allocation.
> +	 */