On Wed, Mar 20, 2024 at 01:39:49PM -0400, Brian Foster wrote:
> 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 <lihongb...@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. ;)
yes, there should always be a prefix
I'm also leaning more and more towards defining /all/ enums with
x-macros; that gets us array-of-names-as-strings for free and just makes
debugging so much nicer than having to remember what integers correspond
to
