On Wed, Mar 20, 2024 at 01:41:09PM -0400, Brian Foster wrote: > 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.
*nod* Rather than doing some renaming I'd like to see further work in this area of the code being done based on some deep profiling, studying of ways to simplify the codepaths, etc. - just renaming the helpers a bit, we can do that if it makes things clearer but it's not terribly important. By the time you've spent enough time with code like this to do deep and interesting work - this is just details that you'll skip over when you're reading. I can take a few renaming patches if they make things clearer, but let's not go overboard. > > 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? I generally (and here) prefer an enum to multiple flags. It might be possible to get rid of path->uptodate entirely; that's something I was considering in the past. "has locks" is redundant, we always have either all or none of the locks we want so checking if path->nodes_locked is nonzero should always tell you if a path is uptodate. for "path needs traverse" - path_set_pos() ensures that a path has a node at the current level iff it's traversed, i.e. !IS_ERR_OR_NULL(path->l[path->level].b) means a path doesn't need to be traversed (still needs to be locked). but again - the btree_path stuff is tricky and subtle, I would advise against fooling around with this code, unless you're willing to invest the time and effort into really studying it and finding some substantive improvements. There are performance improvements to be had in the btree iterator code, so there is work to be done here, but right now it's low priority - we've got higher level performance bugs that are much more serious than anything going on in this code.
