Re: [PATCH 08/32] btrfs: extent_io: sink less common parameters for __set_extent_bit()

[Nikolay Borisov <nborisov@suse.com>]

On 3.11.20 г. 15:30 ч., Qu Wenruo wrote:
> For __set_extent_bit(), those parameter are less common for most
> callers:
> - exclusive_bits
> - failed_start
>   Mostly for extent locking.
> 
> - extent_changeset
>   For qgroup usage.
> 
> As a common design principle, less common parameters should have their
> default values and only callers really need them will set the parameters
> to non-default values.
> 
> Sink those parameters into a new structure, extent_io_extra_options.
> So most callers won't bother those less used parameters, and make later
> expansion easier.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

IMO I feel this is an overkill, __set_extent_Bit is really some
low-level, hidden interface which is being wrapped around by other
high-level extent bit manipulation functions. Following this logic I did
send today a patch renaming __set_extent_bit to set_extent_bit,
essentially removing a level of indirection.

Having said that what you are doing right now might make more sense for
future changes since you state it's preparatory anyway. But in any case
I believe the interface for this function is just broken if we have to
resort to such type of refactoring.

See below for one idea of extent bits handling.

> ---
>  fs/btrfs/extent-io-tree.h | 22 ++++++++++++++
>  fs/btrfs/extent_io.c      | 61 ++++++++++++++++++++++++---------------
>  2 files changed, 59 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h
> index cab4273ff8d3..c93065794567 100644
> --- a/fs/btrfs/extent-io-tree.h
> +++ b/fs/btrfs/extent-io-tree.h
> @@ -82,6 +82,28 @@ struct extent_state {
>  #endif
>  };
>  
> +/*
> + * Extra options for extent io tree operations.
> + *
> + * All of these options are initialized to 0/false/NULL by default,
> + * and most callers should utilize the wrappers other than the extra options.
> + */
> +struct extent_io_extra_options {
> +	/*
> +	 * For __set_extent_bit(), to return -EEXIST when hit an extent with
> +	 * @excl_bits set, and update @excl_failed_start.
> +	 * Utizlied by EXTENT_LOCKED wrappers.

nit: excl_bits can be removed if we simply check for the presence of
EXTENT_LOCKED in 'bits' and if the result is true then also check if the
found extent has EXTENT_LOCKED if it does -> return the failure_start,
that we we can get rid of 'excl_bits'. All uses of EXTENT_LOCKED is via
lock_extent_range except the one in the private failure tree but I
believe it should be ok.

I had a crazy idea to overload cached_state to return the failure range
and use it in lock_extent_bit but that makes it rather messy. ...

> +	 */
> +	u32 excl_bits;
> +	u64 excl_failed_start;
> +
> +	/*
> +	 * For __set/__clear_extent_bit() to record how many bytes is modified.
> +	 * For qgroup related functions.
> +	 */
> +	struct extent_changeset *changeset;
> +};
> +
>  int __init extent_state_cache_init(void);
>  void __cold extent_state_cache_exit(void);
>  

<snip>