On Sat, Jul 09, 2022 at 11:36:45AM +0000, Johannes Thumshirn wrote:
> On 09.07.22 01:21, Naohiro Aota wrote:
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 3194eca41635..cedc94a7d5b2 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -2021,10 +2021,16 @@ noinline_for_stack bool find_lock_delalloc_range(struct inode *inode,
> > struct page *locked_page, u64 *start,
> > u64 *end)
> > {
> > + struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> > struct extent_io_tree *tree = &BTRFS_I(inode)->io_tree;
> > const u64 orig_start = *start;
> > const u64 orig_end = *end;
> > - u64 max_bytes = BTRFS_MAX_EXTENT_SIZE;
> > +#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> > + /* The sanity tests may not set a valid fs_info. */
> > + u64 max_bytes = fs_info ? fs_info->max_extent_size : BTRFS_MAX_EXTENT_SIZE;
> > +#else
> > + u64 max_bytes = fs_info->max_extent_size;
> > +#endif
>
> Do we really need the ifdef here? I don't think there will be a lot
> of performance penalty from the 1 compare that we safe with the ifdef.
You're right that performce-wise it does not make much improvement
however with the explicit ifdef it's clear that it's there for tests,
otherwise finding the conditional fs_info check would be hard to spot. A
search for CONFIG_BTRFS_FS_RUN_SANITY_TESTS would find it. On the other
hand the whole function is EXPORT_FOR_TESTS so we can expect exceptions
for tests. So this could be the pattern to follow should we need it in
the future (ie. no ifdef).
