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 <[email protected]> > --- > 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 >
