Message-ID: <CAL3q7H5TVPG-KiwZfEmndAQh8g+CT1tMPSYcT1Omhc3om6EHEg@mail.gmail.com>
Date: Wed, 30 Apr 2025 13:33:52 +0100
From: Filipe Manana <fdmanana@...nel.org>
To: Daniel Vacek <neelx@...e.com>
Cc: Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>, 
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs: remove extent buffer's redundant `len` member field

On Wed, Apr 30, 2025 at 1:06 PM Daniel Vacek <neelx@...e.com> wrote:
>
> On Wed, 30 Apr 2025 at 12:26, Filipe Manana <fdmanana@...nel.org> wrote:
> >
> > On Wed, Apr 30, 2025 at 9:50 AM Daniel Vacek <neelx@...e.com> wrote:
> > >
> > > On Wed, 30 Apr 2025 at 10:34, Filipe Manana <fdmanana@...nel.org> wrote:
> > > >
> > > > On Wed, Apr 30, 2025 at 9:26 AM Daniel Vacek <neelx@...e.com> wrote:
> > > > >
> > > > > On Wed, 30 Apr 2025 at 10:06, Filipe Manana <fdmanana@...nel.org> wrote:
> > > > > >
> > > > > > On Tue, Apr 29, 2025 at 4:19 PM Daniel Vacek <neelx@...e.com> wrote:
> > > > > > >
> > > > > > > Even super block nowadays uses nodesize for eb->len. This is since commits
> > > > > > >
> > > > > > > 551561c34663 ("btrfs: don't pass nodesize to __alloc_extent_buffer()")
> > > > > > > da17066c4047 ("btrfs: pull node/sector/stripe sizes out of root and into fs_info")
> > > > > > > ce3e69847e3e ("btrfs: sink parameter len to alloc_extent_buffer")
> > > > > > > a83fffb75d09 ("btrfs: sink blocksize parameter to btrfs_find_create_tree_block")
> > > > > > >
> > > > > > > With these the eb->len is not really useful anymore. Let's use the nodesize
> > > > > > > directly where applicable.
> > > > > > >
> > > > > > > Signed-off-by: Daniel Vacek <neelx@...e.com>
> > > > > > > ---
> > > > > > > [RFC]
> > > > > > >  * Shall the eb_len() helper better be called eb_nodesize()? Or even rather
> > > > > > >    opencoded and not used at all?
> ...
> > > > > > > +static inline u32 eb_len(const struct extent_buffer *eb)
> > > > > > > +{
> > > > > > > +       return eb->fs_info->nodesize;
> > > > > > > +}
> > > > > >
> > > > > > Please always add a "btrfs_" prefix to the name of exported functions.
> > > > >
> > > > > It's static inline, not exported. But I'm happy just opencoding it
> > > > > instead. Thanks.
> > > >
> > > > Exported in the sense that it's in a header and visible to any C files
> > > > that include it, not in the sense of being exported with
> > > > EXPORT_SYMBOL_GPL() for example.
> > > > This is our coding style convention:
> > > >
> > > > https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#function-declarations
> > > >
> > > > static functions inside a C file can omit the prefix.
> > >
> > > Nah, thanks again. I was not aware of that. Will keep it in mind.
> > >
> > > Still, it doesn't make sense to me to be honest. I mean specifically
> > > with this example. The header file is also private to btrfs, no public
> > > API. Personally I wouldn't differentiate if it's a source or a header
> > > file. The code can be freely moved around. And with the prefix the
> > > code would end up more bloated and less readable, IMO. But let's not
> > > start any flamewars here.
> >
> > I'd disagree about less readability. Reading code that calls a
> > function with the btrfs prefix makes it clear it's a btrfs specific
> > function.
> > Looking at ext4 and xfs, functions declared or defined in their
> > headers have a "ext4_", "ext_" or "xfs_" prefix.
>
> I see. Makes sense.
> Does this also apply to preprocessor macros? I don't see them
> mentioned in the development notes.
> I'm asking as I did consider using a macro which would look a bit
> cleaner perhaps, just one line instead of four. But it would also miss
> the type checking.
> So I guess the naming convention should also apply to macros, right?
>
> Finally quickly checking I see a lot of functions like eg.
> free_extent_buffer(), free_extent_buffer_stale() and many others
> violating the rule. I guess we should also clean up and rename them,
> right?

Haven't you seen patchsets from me in the last few weeks renaming
functions from extent-io-tree.h and extent_map.h?

You'll see examples of where the prefix is missing, and this happens
for very old code, we rename things from time to time.
In those two cases I was motivated due to the need to add more
functions soon, or having added some new ones not long ago, to make
everything consistent in those headers/modules by making sure all have
the "btrfs_" prefix.



>
> > > > > > In this case I don't think adding this helper adds any value.
> > > > > > We can just do eb->fs_info->nodesize everywhere and in some places we
> > > > > > already have fs_info in a local variable and can just do
> > > > > > fs_info->nodesize.
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > > > +
> > > > > > >  /* Note: this can be used in for loops without caching the value in a variable. */
> > > > > > >  static inline int __pure num_extent_pages(const struct extent_buffer *eb)
> > > > > > >  {
> > > > > > >         /*
> > > > > > >          * For sectorsize == PAGE_SIZE case, since nodesize is always aligned to
> > > > > > > -        * sectorsize, it's just eb->len >> PAGE_SHIFT.
> > > > > > > +        * sectorsize, it's just nodesize >> PAGE_SHIFT.
> > > > > > >          *
> > > > > > >          * For sectorsize < PAGE_SIZE case, we could have nodesize < PAGE_SIZE,
> > > > > > >          * thus have to ensure we get at least one page.
> > > > > > >          */
> > > > > > > -       return (eb->len >> PAGE_SHIFT) ?: 1;
> > > > > > > +       return (eb_len(eb) >> PAGE_SHIFT) ?: 1;
> > > > > > >  }
> > > > > > >
> > > > > > >  /*
> > > > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > > > > > > index 68fac77fb95d1..6be2d56d44917 100644
> > > > > > > --- a/fs/btrfs/ioctl.c
> > > > > > > +++ b/fs/btrfs/ioctl.c
> > > > > > > @@ -598,7 +598,7 @@ static noinline int create_subvol(struct mnt_idmap *idmap,
> > > > > > >         btrfs_set_root_generation(root_item, trans->transid);
> > > > > > >         btrfs_set_root_level(root_item, 0);
> > > > > > >         btrfs_set_root_refs(root_item, 1);
> > > > > > > -       btrfs_set_root_used(root_item, leaf->len);
> > > > > > > +       btrfs_set_root_used(root_item, fs_info->nodesize);
> > > > > > >         btrfs_set_root_last_snapshot(root_item, 0);
> > > > > > >
> > > > > > >         btrfs_set_root_generation_v2(root_item,
> > > > > > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> > > > > > > index 6287e71ebad5f..5086485a4ae21 100644
> > > > > > > --- a/fs/btrfs/relocation.c
> > > > > > > +++ b/fs/btrfs/relocation.c
> > > > > > > @@ -4352,7 +4352,7 @@ int btrfs_reloc_cow_block(struct btrfs_trans_handle *trans,
> > > > > > >                         mark_block_processed(rc, node);
> > > > > > >
> > > > > > >                 if (first_cow && level > 0)
> > > > > > > -                       rc->nodes_relocated += buf->len;
> > > > > > > +                       rc->nodes_relocated += fs_info->nodesize;
> > > > > > >         }
> > > > > > >
> > > > > > >         if (level == 0 && first_cow && rc->stage == UPDATE_DATA_PTRS)
> > > > > > > diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
> > > > > > > index d4f0192334936..711792f32e9ce 100644
> > > > > > > --- a/fs/btrfs/subpage.c
> > > > > > > +++ b/fs/btrfs/subpage.c
> > > > > > > @@ -631,7 +631,7 @@ void btrfs_meta_folio_set_##name(struct folio *folio, const struct extent_buffer
> > > > > > >                 folio_set_func(folio);                                  \
> > > > > > >                 return;                                                 \
> > > > > > >         }                                                               \
> > > > > > > -       btrfs_subpage_set_##name(eb->fs_info, folio, eb->start, eb->len); \
> > > > > > > +       btrfs_subpage_set_##name(eb->fs_info, folio, eb->start, eb_len(eb)); \
> > > > > > >  }                                                                      \
> > > > > > >  void btrfs_meta_folio_clear_##name(struct folio *folio, const struct extent_buffer *eb) \
> > > > > > >  {                                                                      \
> > > > > > > @@ -639,13 +639,13 @@ void btrfs_meta_folio_clear_##name(struct folio *folio, const struct extent_buff
> > > > > > >                 folio_clear_func(folio);                                \
> > > > > > >                 return;                                                 \
> > > > > > >         }                                                               \
> > > > > > > -       btrfs_subpage_clear_##name(eb->fs_info, folio, eb->start, eb->len); \
> > > > > > > +       btrfs_subpage_clear_##name(eb->fs_info, folio, eb->start, eb_len(eb)); \
> > > > > > >  }                                                                      \
> > > > > > >  bool btrfs_meta_folio_test_##name(struct folio *folio, const struct extent_buffer *eb) \
> > > > > > >  {                                                                      \
> > > > > > >         if (!btrfs_meta_is_subpage(eb->fs_info))                        \
> > > > > > >                 return folio_test_func(folio);                          \
> > > > > > > -       return btrfs_subpage_test_##name(eb->fs_info, folio, eb->start, eb->len); \
> > > > > > > +       return btrfs_subpage_test_##name(eb->fs_info, folio, eb->start, eb_len(eb)); \
> > > > > > >  }
> > > > > > >  IMPLEMENT_BTRFS_PAGE_OPS(uptodate, folio_mark_uptodate, folio_clear_uptodate,
> > > > > > >                          folio_test_uptodate);
> > > > > > > @@ -765,7 +765,7 @@ bool btrfs_meta_folio_clear_and_test_dirty(struct folio *folio, const struct ext
> > > > > > >                 return true;
> > > > > > >         }
> > > > > > >
> > > > > > > -       last = btrfs_subpage_clear_and_test_dirty(eb->fs_info, folio, eb->start, eb->len);
> > > > > > > +       last = btrfs_subpage_clear_and_test_dirty(eb->fs_info, folio, eb->start, eb_len(eb));
> > > > > > >         if (last) {
> > > > > > >                 folio_clear_dirty_for_io(folio);
> > > > > > >                 return true;
> > > > > > > diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
> > > > > > > index 00da54f0164c9..657f8f1d9263e 100644
> > > > > > > --- a/fs/btrfs/tests/extent-io-tests.c
> > > > > > > +++ b/fs/btrfs/tests/extent-io-tests.c
> > > > > > > @@ -342,7 +342,7 @@ static int check_eb_bitmap(unsigned long *bitmap, struct extent_buffer *eb)
> > > > > > >  {
> > > > > > >         unsigned long i;
> > > > > > >
> > > > > > > -       for (i = 0; i < eb->len * BITS_PER_BYTE; i++) {
> > > > > > > +       for (i = 0; i < eb_len(eb) * BITS_PER_BYTE; i++) {
> > > > > > >                 int bit, bit1;
> > > > > > >
> > > > > > >                 bit = !!test_bit(i, bitmap);
> > > > > > > @@ -411,7 +411,7 @@ static int test_bitmap_clear(const char *name, unsigned long *bitmap,
> > > > > > >  static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb)
> > > > > > >  {
> > > > > > >         unsigned long i, j;
> > > > > > > -       unsigned long byte_len = eb->len;
> > > > > > > +       unsigned long byte_len = eb_len(eb);
> > > > > > >         u32 x;
> > > > > > >         int ret;
> > > > > > >
> > > > > > > @@ -670,7 +670,7 @@ static int test_find_first_clear_extent_bit(void)
> > > > > > >  static void dump_eb_and_memory_contents(struct extent_buffer *eb, void *memory,
> > > > > > >                                         const char *test_name)
> > > > > > >  {
> > > > > > > -       for (int i = 0; i < eb->len; i++) {
> > > > > > > +       for (int i = 0; i < eb_len(eb); i++) {
> > > > > > >                 struct page *page = folio_page(eb->folios[i >> PAGE_SHIFT], 0);
> > > > > > >                 void *addr = page_address(page) + offset_in_page(i);
> > > > > > >
> > > > > > > @@ -686,7 +686,7 @@ static void dump_eb_and_memory_contents(struct extent_buffer *eb, void *memory,
> > > > > > >  static int verify_eb_and_memory(struct extent_buffer *eb, void *memory,
> > > > > > >                                 const char *test_name)
> > > > > > >  {
> > > > > > > -       for (int i = 0; i < (eb->len >> PAGE_SHIFT); i++) {
> > > > > > > +       for (int i = 0; i < (eb_len(eb) >> PAGE_SHIFT); i++) {
> > > > > > >                 void *eb_addr = folio_address(eb->folios[i]);
> > > > > > >
> > > > > > >                 if (memcmp(memory + (i << PAGE_SHIFT), eb_addr, PAGE_SIZE) != 0) {
> > > > > > > @@ -703,8 +703,8 @@ static int verify_eb_and_memory(struct extent_buffer *eb, void *memory,
> > > > > > >   */
> > > > > > >  static void init_eb_and_memory(struct extent_buffer *eb, void *memory)
> > > > > > >  {
> > > > > > > -       get_random_bytes(memory, eb->len);
> > > > > > > -       write_extent_buffer(eb, memory, 0, eb->len);
> > > > > > > +       get_random_bytes(memory, eb_len(eb));
> > > > > > > +       write_extent_buffer(eb, memory, 0, eb_len(eb));
> > > > > > >  }
> > > > > > >
> > > > > > >  static int test_eb_mem_ops(u32 sectorsize, u32 nodesize)
> > > > > > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> > > > > > > index 9d42bf2bfd746..c7a8cdd87c509 100644
> > > > > > > --- a/fs/btrfs/zoned.c
> > > > > > > +++ b/fs/btrfs/zoned.c
> > > > > > > @@ -2422,7 +2422,7 @@ void btrfs_schedule_zone_finish_bg(struct btrfs_block_group *bg,
> > > > > > >                                    struct extent_buffer *eb)
> > > > > > >  {
> > > > > > >         if (!test_bit(BLOCK_GROUP_FLAG_SEQUENTIAL_ZONE, &bg->runtime_flags) ||
> > > > > > > -           eb->start + eb->len * 2 <= bg->start + bg->zone_capacity)
> > > > > > > +           eb->start + eb_len(eb) * 2 <= bg->start + bg->zone_capacity)
> > > > > > >                 return;
> > > > > > >
> > > > > > >         if (WARN_ON(bg->zone_finish_work.func == btrfs_zone_finish_endio_workfn)) {
> > > > > > > --
> > > > > > > 2.47.2
> > > > > > >
> > > > > > >

Powered by blists - more mailing lists