On Wed, Mar 20, 2024 at 02:29:23PM +0800, Hongbo Li wrote:
> UPTODATE means btree_path is useable, so here use
> btree_path_is_uptodate to check this status. And the old
> function btree_path_set_dirty cannot represent what it really
> does (such as if UPTODATE is passed into it, btree_path sometime
> wasn't dirty).
>
> Signed-off-by: Hongbo Li <[email protected]>
> ---
> fs/bcachefs/btree_iter.c | 16 ++++++++--------
> fs/bcachefs/btree_iter.h | 13 +++++++++----
> fs/bcachefs/btree_key_cache.c | 2 +-
> fs/bcachefs/btree_locking.c | 10 +++++-----
> fs/bcachefs/btree_locking.h | 8 ++++----
> fs/bcachefs/btree_trans_commit.c | 2 +-
> 6 files changed, 28 insertions(+), 23 deletions(-)
>
...
> diff --git a/fs/bcachefs/btree_iter.h b/fs/bcachefs/btree_iter.h
> index c76070494284..f9ed8c44ab5a 100644
> --- a/fs/bcachefs/btree_iter.h
> +++ b/fs/bcachefs/btree_iter.h
> @@ -27,10 +27,15 @@ static inline bool __btree_path_put(struct btree_path
> *path, bool intent)
> return --path->ref == 0;
> }
>
> -static inline void btree_path_set_dirty(struct btree_path *path,
> - enum btree_path_state u)
> +static inline bool btree_path_is_uptodate(struct btree_path *path)
> {
> - path->status = max_t(unsigned, path->status, u);
> + return path->status == UPTODATE;
> +}
> +
> +static inline void btree_path_update_state(struct btree_path *path,
> + enum btree_path_state u)
> +{
> + path->status = max_t(unsigned, path->status, u);
> }
>
Hmm.. I'm kind of neutral on the update_state() rename, but I like the
btree_path_is_uptodate() helper to clean up some of the wonky logic.
My initial thought was to wonder whether it's worth creating some macro
helpers for each state, but after looking through the code a bit that
seems like overkill. In fact AFAICT, the only purpose of the
NEED_TRAVERSE state is so that a lock cycle doesn't return the path back
to UPTODATE in that particular case. Otherwise nothing else actually
seems to explicitly check for NEED_TRAVERSE state.
Am I missing something there? If not, I'd say just replace the enum with
an explicit 2-bit field called something like 'path_invalid' and define
the states as corresponding flags. For example:
#define BTREE_PATH_INVALID (1 << 0)
#define BTREE_PATH_INVALID_LOCKS (1 << 1)
Then the helpers just manage the validation flags appropriately (i.e.
is_uptodate() means path_invalid == 0). I suppose you could also invert
that logic and call the field path_valid or whatever too. Thoughts, or
other ideas?
Brian
> static inline struct btree *btree_path_node(struct btree_path *path,
> @@ -219,7 +224,7 @@ int __must_check bch2_btree_path_traverse_one(struct
> btree_trans *,
> static inline int __must_check bch2_btree_path_traverse(struct btree_trans
> *trans,
> btree_path_idx_t path, unsigned flags)
> {
> - if (trans->paths[path].status < NEED_RELOCK)
> + if (btree_path_is_uptodate(&trans->paths[path]))
> return 0;
>
> return bch2_btree_path_traverse_one(trans, path, flags, _RET_IP_);
> diff --git a/fs/bcachefs/btree_key_cache.c b/fs/bcachefs/btree_key_cache.c
> index 47cf735f24a0..d178b197dc17 100644
> --- a/fs/bcachefs/btree_key_cache.c
> +++ b/fs/bcachefs/btree_key_cache.c
> @@ -540,7 +540,7 @@ bch2_btree_path_traverse_cached_slowpath(struct
> btree_trans *trans, struct btree
> set_bit(BKEY_CACHED_ACCESSED, &ck->flags);
>
> BUG_ON(btree_node_locked_type(path, 0) != btree_lock_want(path, 0));
> - BUG_ON(path->status);
> + BUG_ON(!btree_path_is_uptodate(path));
>
> return ret;
> err:
> diff --git a/fs/bcachefs/btree_locking.c b/fs/bcachefs/btree_locking.c
> index 8d8e3207ca7a..0f18aacb9b5e 100644
> --- a/fs/bcachefs/btree_locking.c
> +++ b/fs/bcachefs/btree_locking.c
> @@ -462,7 +462,7 @@ void bch2_btree_node_lock_write_nofail(struct btree_trans
> *trans,
> for (i = 0; i < BTREE_MAX_DEPTH; i++)
> if (btree_node_read_locked(linked, i)) {
> btree_node_unlock(trans, linked, i);
> - btree_path_set_dirty(linked, NEED_RELOCK);
> + btree_path_update_state(linked, NEED_RELOCK);
> }
> }
>
> @@ -505,7 +505,7 @@ static inline bool btree_path_get_locks(struct
> btree_trans *trans,
> */
> if (fail_idx >= 0) {
> __bch2_btree_path_unlock(trans, path);
> - btree_path_set_dirty(path, NEED_TRAVERSE);
> + btree_path_update_state(path, NEED_TRAVERSE);
>
> do {
> path->l[fail_idx].b = upgrade
> @@ -520,7 +520,7 @@ static inline bool btree_path_get_locks(struct
> btree_trans *trans,
>
> bch2_trans_verify_locks(trans);
>
> - return path->status < NEED_RELOCK;
> + return btree_path_is_uptodate(path);
> }
>
> bool __bch2_btree_node_relock(struct btree_trans *trans,
> @@ -621,7 +621,7 @@ int bch2_btree_path_relock_intent(struct btree_trans
> *trans,
> l++) {
> if (!bch2_btree_node_relock(trans, path, l)) {
> __bch2_btree_path_unlock(trans, path);
> - btree_path_set_dirty(path, NEED_TRAVERSE);
> + btree_path_update_state(path, NEED_TRAVERSE);
> trace_and_count(trans->c,
> trans_restart_relock_path_intent, trans, _RET_IP_, path);
> return btree_trans_restart(trans,
> BCH_ERR_transaction_restart_relock_path_intent);
> }
> @@ -865,7 +865,7 @@ void bch2_btree_path_verify_locks(struct btree_path *path)
> unsigned l;
>
> if (!path->nodes_locked) {
> - BUG_ON(path->status == UPTODATE &&
> + BUG_ON(btree_path_is_uptodate(path) &&
> btree_path_node(path, path->level));
> return;
> }
> diff --git a/fs/bcachefs/btree_locking.h b/fs/bcachefs/btree_locking.h
> index f3f03292a675..f58f769c0f19 100644
> --- a/fs/bcachefs/btree_locking.h
> +++ b/fs/bcachefs/btree_locking.h
> @@ -157,7 +157,7 @@ static inline int btree_path_highest_level_locked(struct
> btree_path *path)
> static inline void __bch2_btree_path_unlock(struct btree_trans *trans,
> struct btree_path *path)
> {
> - btree_path_set_dirty(path, NEED_RELOCK);
> + btree_path_update_state(path, NEED_RELOCK);
>
> while (path->nodes_locked)
> btree_node_unlock(trans, path,
> btree_path_lowest_level_locked(path));
> @@ -371,7 +371,7 @@ static inline int bch2_btree_path_upgrade(struct
> btree_trans *trans,
>
> if (path->locks_want < new_locks_want
> ? __bch2_btree_path_upgrade(trans, path, new_locks_want, &f)
> - : path->status == UPTODATE)
> + : btree_path_is_uptodate(path))
> return 0;
>
> trace_and_count(trans->c, trans_restart_upgrade, trans, _THIS_IP_, path,
> @@ -384,7 +384,7 @@ static inline int bch2_btree_path_upgrade(struct
> btree_trans *trans,
> static inline void btree_path_set_should_be_locked(struct btree_path *path)
> {
> EBUG_ON(!btree_node_locked(path, path->level));
> - EBUG_ON(path->status);
> + EBUG_ON(!btree_path_is_uptodate(path));
>
> path->should_be_locked = true;
> }
> @@ -401,7 +401,7 @@ static inline void btree_path_set_level_up(struct
> btree_trans *trans,
> struct btree_path *path)
> {
> __btree_path_set_level_up(trans, path, path->level++);
> - btree_path_set_dirty(path, NEED_TRAVERSE);
> + btree_path_update_state(path, NEED_TRAVERSE);
> }
>
> /* debug */
> diff --git a/fs/bcachefs/btree_trans_commit.c
> b/fs/bcachefs/btree_trans_commit.c
> index 0c94a885b567..222c52c1d949 100644
> --- a/fs/bcachefs/btree_trans_commit.c
> +++ b/fs/bcachefs/btree_trans_commit.c
> @@ -745,7 +745,7 @@ bch2_trans_commit_write_locked(struct btree_trans *trans,
> unsigned flags,
> bch2_btree_insert_key_cached(trans, flags, i);
> else {
> bch2_btree_key_cache_drop(trans, path);
> - btree_path_set_dirty(path, NEED_TRAVERSE);
> + btree_path_update_state(path, NEED_TRAVERSE);
> }
> }
>
> --
> 2.34.1
>
