Re: [PATCH RFC 1/2] bcachefs: use btree_path_state to represent btree_path status

[Brian Foster <bfoster@redhat.com>]

On Wed, Mar 20, 2024 at 02:29:22PM +0800, Hongbo Li wrote:
> First, the old structure cannot clearly represent the state changes
> of btree_path (such as BTREE_ITER_xxx). Secondly, the member (
> btree_path->uptodate) cannot express its purpose intuitively. It's
> essentially a state value if I understand correctly. Using this way
> can makes the representation of member variables more reasonable.
> 
> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> ---
>  fs/bcachefs/btree_iter.c         | 22 +++++++++++-----------
>  fs/bcachefs/btree_iter.h         |  6 +++---
>  fs/bcachefs/btree_key_cache.c    | 12 ++++++------
>  fs/bcachefs/btree_locking.c      | 14 +++++++-------
>  fs/bcachefs/btree_locking.h      |  8 ++++----
>  fs/bcachefs/btree_trans_commit.c |  2 +-
>  fs/bcachefs/btree_types.h        | 10 +++++-----
>  7 files changed, 37 insertions(+), 37 deletions(-)
> 
...
> diff --git a/fs/bcachefs/btree_types.h b/fs/bcachefs/btree_types.h
> index 9404d96c38f3..7146d6cf9fba 100644
> --- a/fs/bcachefs/btree_types.h
> +++ b/fs/bcachefs/btree_types.h
> @@ -218,10 +218,10 @@ static const __maybe_unused u16 BTREE_ITER_CACHED_NOFILL	= 1 << 13;
>  static const __maybe_unused u16 BTREE_ITER_KEY_CACHE_FILL	= 1 << 14;
>  #define __BTREE_ITER_FLAGS_END					       15
>  
> -enum btree_path_uptodate {
> -	BTREE_ITER_UPTODATE		= 0,
> -	BTREE_ITER_NEED_RELOCK		= 1,
> -	BTREE_ITER_NEED_TRAVERSE	= 2,
> +enum btree_path_state {
> +        UPTODATE		= 0,
> +        NEED_RELOCK		= 1,
> +        NEED_TRAVERSE		= 2
>  };
>  
>  #if defined(CONFIG_BCACHEFS_LOCK_TIME_STATS) || defined(CONFIG_BCACHEFS_DEBUG)
> @@ -241,7 +241,7 @@ struct btree_path {
>  	enum btree_id		btree_id:5;
>  	bool			cached:1;
>  	bool			preserve:1;
> -	enum btree_path_uptodate uptodate:2;
> +	enum btree_path_state   status:2;

Personally I don't mind the field rename, but the loss of any prefix on
the flags urks me a bit. Any reason not to use something like
BTREE_PATH_UPTODATE..?

It also would be nice to be consistent between the field and enum names.
Perhaps rename the field to 'state' if it's of type btree_path_state?

Finally, if we are going to tweak this, it seems like a nice opportunity
to add a comment above btree_path_state to (briefly) explain what each
state means and perhaps how they relate.

Just my .02 of course. ;)

Brian

>  	/*
>  	 * When true, failing to relock this path will cause the transaction to
>  	 * restart:
> -- 
> 2.34.1
>