Re: [PATCH v2 00/11] btrfs: extent tree v2, support for global roots

[David Sterba <dsterba@suse.cz>]

On Wed, Jan 12, 2022 at 05:09:15PM +0200, Nikolay Borisov wrote:
> Overall this is a low-risk series and generally looks LGTM. I have a
> couple of points which apply too all patches and I'm gonna list them here:
> 
> 1. This is minor but might be a bit more informative - instead of
> returning -EINVAL when an operation which is forbidden returning
> -EOPNOTSUPP is closer to what is actually meant. ANd not that something
> is invalid per-se. This is a minor point given for now this is purely a
> developer feature.

Makes sense to use EOPNOTSUPP.

> 2. The extent tree would be in development for quite some time, perhaps
> at least 3-4 release if not more, during that time the code will be
> sprinkled with the checks for the forbidden ops and even everyone will
> be paying the (arguably small cost) of having them in the code. My point
> is can't those compatibility checks be also gated behind
> CONFIG_BTRFS_DEBUG? Eventually they will be removed but until this time
> comes we'll have them in the respective call paths.

That's a good point, we could make it compile-time condition with some
magic that should remove any cost of the checks. Something like

	if (EXTENT_V2_ENABLED(fs_info)) {
		...
	}

where the macro is

#ifdef CONFIG_BTRFS_DEBUG
#define EXTENT_V2_ENABLED(fs_info) 	btrfs_incompat(fs_info, EXTENT_TREE_V2)
#else
#define EXTENT_V2_ENABLED(fs_info)	false
#endif