Re: [PATCH 5/8] btrfs: zoned: activate metadata block group on write time

[Christoph Hellwig <hch@infradead.org>]

On Mon, Jul 24, 2023 at 01:18:34PM +0900, Naohiro Aota wrote:
> Since metadata write-out is always allocated sequentially, when we need to
> write to a non-active block group, we can wait for the ongoing IOs to
> complete, activate a new block group, and then proceed with writing to the
> new block group.

Somewhat unrelated, but isn't the same true for data as well, and maybe
in a follow on the same activation policy should apply there as well?
I guess it won't quite work without much work as reservations are tied
to a BG, but it would seem like the better scheme.

> +	if (test_bit(BTRFS_FS_ACTIVE_ZONE_TRACKING, &fs_info->flags)) {

Just a more or less cosmetic nit:  there is a lot of code in this
branch, and I wonder if the active zone tracking code would benefit
from being split out into one or more helpers.

> +		bool is_system = cache->flags & BTRFS_BLOCK_GROUP_SYSTEM;
> +
> +		spin_lock(&cache->lock);
> +		if (test_bit(BLOCK_GROUP_FLAG_ZONE_IS_ACTIVE,
> +			     &cache->runtime_flags)) {
> +			spin_unlock(&cache->lock);
> +			return true;
> +		}
> +
> +		spin_unlock(&cache->lock);

What is the point of the cache->lock crticial section here?  The
information can be out of date as soon as you drop the lock, so it
looks superflous to me.

> +		if (fs_info->treelog_bg == cache->start) {
> +			if (!btrfs_zone_activate(cache)) {
> +				int ret_fin = btrfs_zone_finish_one_bg(fs_info);
> +
> +				if (ret_fin != 1 || !btrfs_zone_activate(cache))
> +					return false;

The ret_fin variable here doesn't seem to be actually needed.

> +			}
> +		} else if ((!is_system && fs_info->active_meta_bg != cache) ||
> +			   (is_system && fs_info->active_system_bg != cache)) {
> +			struct btrfs_block_group *tgt = is_system ?
> +				fs_info->active_system_bg : fs_info->active_meta_bg;

There's a lot of checks for is_system here and later on in the
logic.  If we had a helper for this, you could just pass in a bg double
pointer argument that the callers sets to &fs_info->active_system_bg or
&fs_info->active_meta_bg and simplify a lot of the logic.