[PATCH 0/4] Self-test cleanups

[PATCH 0/4] Self-test cleanups

[David Sterba]

I've noticed a new use of the STATIC macro in Nikolay's patches cleaning
up delayed refs. My series removes the macro and does a other minor
reorganization to reduce ifdefs used in the test code.

David Sterba (4):
  btrfs: tests: add separate stub for find_lock_delalloc_range
  btrfs: tests: move testing members of struct btrfs_root to the end
  btrfs: tests: group declarations of self-test helpers
  btrfs: tests: polish ifdefs around testing helper

 fs/btrfs/ctree.h                 | 28 +++++++++++-----------------
 fs/btrfs/extent_io.c             | 13 ++++++++++++-
 fs/btrfs/extent_io.h             |  2 +-
 fs/btrfs/tests/extent-io-tests.c | 10 +++++-----
 4 files changed, 29 insertions(+), 24 deletions(-)

-- 
2.18.0

[PATCH 1/4] btrfs: tests: add separate stub for find_lock_delalloc_range

[David Sterba]

The helper find_lock_delalloc_range is now conditionally built static,
dpending on whether the self-tests are enabled or not. There's a macro
that is suppsed to hide the export, used only once. To discourage
further use, drop it an add a public stub for the helper required by
tests.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.h                 |  6 ------
 fs/btrfs/extent_io.c             | 13 ++++++++++++-
 fs/btrfs/extent_io.h             |  2 +-
 fs/btrfs/tests/extent-io-tests.c | 10 +++++-----
 4 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 2cddfe7806a4..45b7029d0f23 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -41,12 +41,6 @@ extern struct kmem_cache *btrfs_path_cachep;
 extern struct kmem_cache *btrfs_free_space_cachep;
 struct btrfs_ordered_sum;
 
-#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-#define STATIC noinline
-#else
-#define STATIC static noinline
-#endif
-
 #define BTRFS_MAGIC 0x4D5F53665248425FULL /* ascii _BHRfS_M, no null */
 
 #define BTRFS_MAX_MIRRORS 3
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 4dd6faab02bb..06e280d8750c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1568,7 +1568,7 @@ static noinline int lock_delalloc_pages(struct inode *inode,
  *
  * 1 is returned if we find something, 0 if nothing was in the tree
  */
-STATIC u64 find_lock_delalloc_range(struct inode *inode,
+static u64 find_lock_delalloc_range(struct inode *inode,
 				    struct extent_io_tree *tree,
 				    struct page *locked_page, u64 *start,
 				    u64 *end, u64 max_bytes)
@@ -1648,6 +1648,17 @@ STATIC u64 find_lock_delalloc_range(struct inode *inode,
 	return found;
 }
 
+#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
+u64 btrfs_find_lock_delalloc_range(struct inode *inode,
+				    struct extent_io_tree *tree,
+				    struct page *locked_page, u64 *start,
+				    u64 *end, u64 max_bytes)
+{
+	return find_lock_delalloc_range(inode, tree, locked_page, start, end,
+			max_bytes);
+}
+#endif
+
 static int __process_pages_contig(struct address_space *mapping,
 				  struct page *locked_page,
 				  pgoff_t start_index, pgoff_t end_index,
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index b4d03e677e1d..1a7fdcbca49b 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -546,7 +546,7 @@ int free_io_failure(struct extent_io_tree *failure_tree,
 		    struct extent_io_tree *io_tree,
 		    struct io_failure_record *rec);
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-noinline u64 find_lock_delalloc_range(struct inode *inode,
+u64 btrfs_find_lock_delalloc_range(struct inode *inode,
 				      struct extent_io_tree *tree,
 				      struct page *locked_page, u64 *start,
 				      u64 *end, u64 max_bytes);
diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
index d9269a531a4d..9e0f4a01be14 100644
--- a/fs/btrfs/tests/extent-io-tests.c
+++ b/fs/btrfs/tests/extent-io-tests.c
@@ -106,7 +106,7 @@ static int test_find_delalloc(u32 sectorsize)
 	set_extent_delalloc(&tmp, 0, sectorsize - 1, 0, NULL);
 	start = 0;
 	end = 0;
-	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (!found) {
 		test_err("should have found at least one delalloc");
@@ -137,7 +137,7 @@ static int test_find_delalloc(u32 sectorsize)
 	set_extent_delalloc(&tmp, sectorsize, max_bytes - 1, 0, NULL);
 	start = test_start;
 	end = 0;
-	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (!found) {
 		test_err("couldn't find delalloc in our range");
@@ -171,7 +171,7 @@ static int test_find_delalloc(u32 sectorsize)
 	}
 	start = test_start;
 	end = 0;
-	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (found) {
 		test_err("found range when we shouldn't have");
@@ -192,7 +192,7 @@ static int test_find_delalloc(u32 sectorsize)
 	set_extent_delalloc(&tmp, max_bytes, total_dirty - 1, 0, NULL);
 	start = test_start;
 	end = 0;
-	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (!found) {
 		test_err("didn't find our range");
@@ -233,7 +233,7 @@ static int test_find_delalloc(u32 sectorsize)
 	 * this changes at any point in the future we will need to fix this
 	 * tests expected behavior.
 	 */
-	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (!found) {
 		test_err("didn't find our range");
-- 
2.18.0

[PATCH 2/4] btrfs: tests: move testing members of struct btrfs_root to the end

[David Sterba]

The data used only for tests are better placed at the end of the
structure so that they don't change the structure layout. All new
members of btrfs_root should be placed before.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 45b7029d0f23..ee05857cc8ac 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1203,11 +1203,6 @@ struct btrfs_root {
 
 	u64 highest_objectid;
 
-#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-	/* only used with CONFIG_BTRFS_FS_RUN_SANITY_TESTS is enabled */
-	u64 alloc_bytenr;
-#endif
-
 	u64 defrag_trans_start;
 	struct btrfs_key defrag_progress;
 	struct btrfs_key defrag_max;
@@ -1280,6 +1275,10 @@ struct btrfs_root {
 	spinlock_t qgroup_meta_rsv_lock;
 	u64 qgroup_meta_rsv_pertrans;
 	u64 qgroup_meta_rsv_prealloc;
+
+#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
+	u64 alloc_bytenr;
+#endif
 };
 
 struct btrfs_file_private {
-- 
2.18.0

[PATCH 3/4] btrfs: tests: group declarations of self-test helpers

[David Sterba]

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index ee05857cc8ac..32d2fce4ac53 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3194,9 +3194,6 @@ int btrfs_prealloc_file_range_trans(struct inode *inode,
 				    u64 start, u64 num_bytes, u64 min_size,
 				    loff_t actual_len, u64 *alloc_hint);
 extern const struct dentry_operations btrfs_dentry_operations;
-#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-void btrfs_test_inode_set_ops(struct inode *inode);
-#endif
 
 /* ioctl.c */
 long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
@@ -3709,6 +3706,7 @@ static inline int btrfs_defrag_cancelled(struct btrfs_fs_info *fs_info)
 
 /* Sanity test specific functions */
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
+void btrfs_test_inode_set_ops(struct inode *inode);
 void btrfs_test_destroy_inode(struct inode *inode);
 #endif
 
-- 
2.18.0

[PATCH 4/4] btrfs: tests: polish ifdefs around testing helper

[David Sterba]

Avoid the inline ifdefs and use two sections for self-tests enabled and
disabled.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 32d2fce4ac53..8dafc7bb6ad8 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3708,17 +3708,20 @@ static inline int btrfs_defrag_cancelled(struct btrfs_fs_info *fs_info)
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 void btrfs_test_inode_set_ops(struct inode *inode);
 void btrfs_test_destroy_inode(struct inode *inode);
-#endif
 
 static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
 {
-#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 	if (unlikely(test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO,
 			      &fs_info->fs_state)))
 		return 1;
-#endif
 	return 0;
 }
+#else
+static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
+{
+	return 0;
+}
+#endif
 
 static inline void cond_wake_up(struct wait_queue_head *wq)
 {
-- 
2.18.0

Re: [PATCH 2/4] btrfs: tests: move testing members of struct btrfs_root to the end

[Omar Sandoval]

On Mon, Sep 10, 2018 at 07:22:26PM +0200, David Sterba wrote:
> The data used only for tests are better placed at the end of the
> structure so that they don't change the structure layout. All new
> members of btrfs_root should be placed before.

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ctree.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 45b7029d0f23..ee05857cc8ac 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1203,11 +1203,6 @@ struct btrfs_root {
>  
>  	u64 highest_objectid;
>  
> -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> -	/* only used with CONFIG_BTRFS_FS_RUN_SANITY_TESTS is enabled */
> -	u64 alloc_bytenr;
> -#endif
> -
>  	u64 defrag_trans_start;
>  	struct btrfs_key defrag_progress;
>  	struct btrfs_key defrag_max;
> @@ -1280,6 +1275,10 @@ struct btrfs_root {
>  	spinlock_t qgroup_meta_rsv_lock;
>  	u64 qgroup_meta_rsv_pertrans;
>  	u64 qgroup_meta_rsv_prealloc;
> +
> +#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> +	u64 alloc_bytenr;
> +#endif
>  };
>  
>  struct btrfs_file_private {
> -- 
> 2.18.0
> 

Re: [PATCH 1/4] btrfs: tests: add separate stub for find_lock_delalloc_range

[Omar Sandoval]

On Mon, Sep 10, 2018 at 07:22:24PM +0200, David Sterba wrote:
> The helper find_lock_delalloc_range is now conditionally built static,
> dpending on whether the self-tests are enabled or not. There's a macro
> that is suppsed to hide the export, used only once. To discourage
> further use, drop it an add a public stub for the helper required by
> tests.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ctree.h                 |  6 ------
>  fs/btrfs/extent_io.c             | 13 ++++++++++++-
>  fs/btrfs/extent_io.h             |  2 +-
>  fs/btrfs/tests/extent-io-tests.c | 10 +++++-----
>  4 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 2cddfe7806a4..45b7029d0f23 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -41,12 +41,6 @@ extern struct kmem_cache *btrfs_path_cachep;
>  extern struct kmem_cache *btrfs_free_space_cachep;
>  struct btrfs_ordered_sum;
>  
> -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> -#define STATIC noinline
> -#else
> -#define STATIC static noinline
> -#endif
> -
>  #define BTRFS_MAGIC 0x4D5F53665248425FULL /* ascii _BHRfS_M, no null */
>  
>  #define BTRFS_MAX_MIRRORS 3
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 4dd6faab02bb..06e280d8750c 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1568,7 +1568,7 @@ static noinline int lock_delalloc_pages(struct inode *inode,
>   *
>   * 1 is returned if we find something, 0 if nothing was in the tree
>   */
> -STATIC u64 find_lock_delalloc_range(struct inode *inode,
> +static u64 find_lock_delalloc_range(struct inode *inode,

Now this won't be noinline, was that intentional? It has been since way
back in d352ac68148b ("Btrfs: add and improve comments"), presumably
because Chris wanted it in stack traces.

Re: [PATCH 3/4] btrfs: tests: group declarations of self-test helpers

[Omar Sandoval]

On Mon, Sep 10, 2018 at 07:22:29PM +0200, David Sterba wrote:

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ctree.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index ee05857cc8ac..32d2fce4ac53 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3194,9 +3194,6 @@ int btrfs_prealloc_file_range_trans(struct inode *inode,
>  				    u64 start, u64 num_bytes, u64 min_size,
>  				    loff_t actual_len, u64 *alloc_hint);
>  extern const struct dentry_operations btrfs_dentry_operations;
> -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> -void btrfs_test_inode_set_ops(struct inode *inode);
> -#endif
>  
>  /* ioctl.c */
>  long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
> @@ -3709,6 +3706,7 @@ static inline int btrfs_defrag_cancelled(struct btrfs_fs_info *fs_info)
>  
>  /* Sanity test specific functions */
>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> +void btrfs_test_inode_set_ops(struct inode *inode);
>  void btrfs_test_destroy_inode(struct inode *inode);
>  #endif
>  
> -- 
> 2.18.0
> 

Re: [PATCH 4/4] btrfs: tests: polish ifdefs around testing helper

[Omar Sandoval]

On Mon, Sep 10, 2018 at 07:22:31PM +0200, David Sterba wrote:
> Avoid the inline ifdefs and use two sections for self-tests enabled and
> disabled.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ctree.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 32d2fce4ac53..8dafc7bb6ad8 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3708,17 +3708,20 @@ static inline int btrfs_defrag_cancelled(struct btrfs_fs_info *fs_info)
>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>  void btrfs_test_inode_set_ops(struct inode *inode);
>  void btrfs_test_destroy_inode(struct inode *inode);
> -#endif
>  
>  static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
>  {
> -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>  	if (unlikely(test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO,
>  			      &fs_info->fs_state)))
>  		return 1;
> -#endif
>  	return 0;

How about just:

	return test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);

We can probably get away without the unlikely() considering that no one
sane is going to run a kernel with CONFIG_BTRFS_FS_RUN_SANITY_TESTS in
production.

Re: [PATCH 1/4] btrfs: tests: add separate stub for find_lock_delalloc_range

[David Sterba]

On Mon, Sep 10, 2018 at 04:40:55PM -0700, Omar Sandoval wrote:
> On Mon, Sep 10, 2018 at 07:22:24PM +0200, David Sterba wrote:
> > The helper find_lock_delalloc_range is now conditionally built static,
> > dpending on whether the self-tests are enabled or not. There's a macro
> > that is suppsed to hide the export, used only once. To discourage
> > further use, drop it an add a public stub for the helper required by
> > tests.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >  fs/btrfs/ctree.h                 |  6 ------
> >  fs/btrfs/extent_io.c             | 13 ++++++++++++-
> >  fs/btrfs/extent_io.h             |  2 +-
> >  fs/btrfs/tests/extent-io-tests.c | 10 +++++-----
> >  4 files changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 2cddfe7806a4..45b7029d0f23 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -41,12 +41,6 @@ extern struct kmem_cache *btrfs_path_cachep;
> >  extern struct kmem_cache *btrfs_free_space_cachep;
> >  struct btrfs_ordered_sum;
> >  
> > -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> > -#define STATIC noinline
> > -#else
> > -#define STATIC static noinline
> > -#endif
> > -
> >  #define BTRFS_MAGIC 0x4D5F53665248425FULL /* ascii _BHRfS_M, no null */
> >  
> >  #define BTRFS_MAX_MIRRORS 3
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 4dd6faab02bb..06e280d8750c 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -1568,7 +1568,7 @@ static noinline int lock_delalloc_pages(struct inode *inode,
> >   *
> >   * 1 is returned if we find something, 0 if nothing was in the tree
> >   */
> > -STATIC u64 find_lock_delalloc_range(struct inode *inode,
> > +static u64 find_lock_delalloc_range(struct inode *inode,
> 
> Now this won't be noinline, was that intentional? It has been since way
> back in d352ac68148b ("Btrfs: add and improve comments"), presumably
> because Chris wanted it in stack traces.

Heh, the commit is from 2008, besides others removes the original TODO,
I think there were way more important things to do at that time than
noinline, but you're right.

Removing noinline was not intentional and it would be better to see it
on the stack. As there's only one caller, the function will most likely
be inlined from writepage_delalloc. I'll update it to
noinline_for_stack.

Re: [PATCH 4/4] btrfs: tests: polish ifdefs around testing helper

[David Sterba]

On Mon, Sep 10, 2018 at 04:43:29PM -0700, Omar Sandoval wrote:
> On Mon, Sep 10, 2018 at 07:22:31PM +0200, David Sterba wrote:
> > Avoid the inline ifdefs and use two sections for self-tests enabled and
> > disabled.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >  fs/btrfs/ctree.h | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 32d2fce4ac53..8dafc7bb6ad8 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -3708,17 +3708,20 @@ static inline int btrfs_defrag_cancelled(struct btrfs_fs_info *fs_info)
> >  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> >  void btrfs_test_inode_set_ops(struct inode *inode);
> >  void btrfs_test_destroy_inode(struct inode *inode);
> > -#endif
> >  
> >  static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
> >  {
> > -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> >  	if (unlikely(test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO,
> >  			      &fs_info->fs_state)))
> >  		return 1;
> > -#endif
> >  	return 0;
> 
> How about just:
> 
> 	return test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);
> 
> We can probably get away without the unlikely() considering that no one
> sane is going to run a kernel with CONFIG_BTRFS_FS_RUN_SANITY_TESTS in
> production.

The unlikely can go away, sure.

I would still like to remove the test_bit call when tests are compiled
out. There are about 10 calls to btrfs_is_testing in various core
functions, followed by further statements. This would have a
(negligible) runtime penalty but generates effectively unused code on
production builds.

The static inline function returning 0 allows to optimize out the unused
code, so smaller code, fewer inctructions, etc.

Re: [PATCH 4/4] btrfs: tests: polish ifdefs around testing helper

[Omar Sandoval]

On Tue, Sep 11, 2018 at 11:22:51AM +0200, David Sterba wrote:
> On Mon, Sep 10, 2018 at 04:43:29PM -0700, Omar Sandoval wrote:
> > On Mon, Sep 10, 2018 at 07:22:31PM +0200, David Sterba wrote:
> > > Avoid the inline ifdefs and use two sections for self-tests enabled and
> > > disabled.
> > > 
> > > Signed-off-by: David Sterba <dsterba@suse.com>
> > > ---
> > >  fs/btrfs/ctree.h | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > > index 32d2fce4ac53..8dafc7bb6ad8 100644
> > > --- a/fs/btrfs/ctree.h
> > > +++ b/fs/btrfs/ctree.h
> > > @@ -3708,17 +3708,20 @@ static inline int btrfs_defrag_cancelled(struct btrfs_fs_info *fs_info)
> > >  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> > >  void btrfs_test_inode_set_ops(struct inode *inode);
> > >  void btrfs_test_destroy_inode(struct inode *inode);
> > > -#endif
> > >  
> > >  static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
> > >  {
> > > -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> > >  	if (unlikely(test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO,
> > >  			      &fs_info->fs_state)))
> > >  		return 1;
> > > -#endif
> > >  	return 0;
> > 
> > How about just:
> > 
> > 	return test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);
> > 
> > We can probably get away without the unlikely() considering that no one
> > sane is going to run a kernel with CONFIG_BTRFS_FS_RUN_SANITY_TESTS in
> > production.
> 
> The unlikely can go away, sure.
> 
> I would still like to remove the test_bit call when tests are compiled
> out. There are about 10 calls to btrfs_is_testing in various core
> functions, followed by further statements. This would have a
> (negligible) runtime penalty but generates effectively unused code on
> production builds.
> 
> The static inline function returning 0 allows to optimize out the unused
> code, so smaller code, fewer inctructions, etc.

Absolutely, I just mean that the CONFIG_BTRFS_FS_RUN_SANITY_TESTS
version can be cleaner:

#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
{
	return test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);
}
#else
static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
{
	return 0;
}
#endif

I find `if (1) return 1; else return 0;` really icky.

Re: [PATCH 4/4] btrfs: tests: polish ifdefs around testing helper

[David Sterba]

On Tue, Sep 11, 2018 at 12:14:47PM -0700, Omar Sandoval wrote:
> > The unlikely can go away, sure.
> > 
> > I would still like to remove the test_bit call when tests are compiled
> > out. There are about 10 calls to btrfs_is_testing in various core
> > functions, followed by further statements. This would have a
> > (negligible) runtime penalty but generates effectively unused code on
> > production builds.
> > 
> > The static inline function returning 0 allows to optimize out the unused
> > code, so smaller code, fewer inctructions, etc.
> 
> Absolutely, I just mean that the CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> version can be cleaner:
> 
> #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
> {
> 	return test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);
> }
> #else
> static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
> {
> 	return 0;
> }
> #endif
> 
> I find `if (1) return 1; else return 0;` really icky.

I see what you mean, will fix it.

[PATCH 1/4 v2] btrfs: tests: add separate stub for find_lock_delalloc_range

[David Sterba]

The helper find_lock_delalloc_range is now conditionally built static,
dpending on whether the self-tests are enabled or not. There's a macro
that is supposed to hide the export, used only once. To discourage
further use, drop it an add a public wrapper for the helper needed by
tests.

Signed-off-by: David Sterba <dsterba@suse.com>
---

v2:
- add noinline_for_stack back
 fs/btrfs/ctree.h                 |  6 ------
 fs/btrfs/extent_io.c             | 13 ++++++++++++-
 fs/btrfs/extent_io.h             |  2 +-
 fs/btrfs/tests/extent-io-tests.c | 10 +++++-----
 4 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 2cddfe7806a4..45b7029d0f23 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -41,12 +41,6 @@ extern struct kmem_cache *btrfs_path_cachep;
 extern struct kmem_cache *btrfs_free_space_cachep;
 struct btrfs_ordered_sum;
 
-#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-#define STATIC noinline
-#else
-#define STATIC static noinline
-#endif
-
 #define BTRFS_MAGIC 0x4D5F53665248425FULL /* ascii _BHRfS_M, no null */
 
 #define BTRFS_MAX_MIRRORS 3
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 4dd6faab02bb..93108b18b231 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1568,7 +1568,7 @@ static noinline int lock_delalloc_pages(struct inode *inode,
  *
  * 1 is returned if we find something, 0 if nothing was in the tree
  */
-STATIC u64 find_lock_delalloc_range(struct inode *inode,
+static noinline_for_stack u64 find_lock_delalloc_range(struct inode *inode,
 				    struct extent_io_tree *tree,
 				    struct page *locked_page, u64 *start,
 				    u64 *end, u64 max_bytes)
@@ -1648,6 +1648,17 @@ STATIC u64 find_lock_delalloc_range(struct inode *inode,
 	return found;
 }
 
+#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
+u64 btrfs_find_lock_delalloc_range(struct inode *inode,
+				    struct extent_io_tree *tree,
+				    struct page *locked_page, u64 *start,
+				    u64 *end, u64 max_bytes)
+{
+	return find_lock_delalloc_range(inode, tree, locked_page, start, end,
+			max_bytes);
+}
+#endif
+
 static int __process_pages_contig(struct address_space *mapping,
 				  struct page *locked_page,
 				  pgoff_t start_index, pgoff_t end_index,
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index b4d03e677e1d..1a7fdcbca49b 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -546,7 +546,7 @@ int free_io_failure(struct extent_io_tree *failure_tree,
 		    struct extent_io_tree *io_tree,
 		    struct io_failure_record *rec);
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-noinline u64 find_lock_delalloc_range(struct inode *inode,
+u64 btrfs_find_lock_delalloc_range(struct inode *inode,
 				      struct extent_io_tree *tree,
 				      struct page *locked_page, u64 *start,
 				      u64 *end, u64 max_bytes);
diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
index d9269a531a4d..9e0f4a01be14 100644
--- a/fs/btrfs/tests/extent-io-tests.c
+++ b/fs/btrfs/tests/extent-io-tests.c
@@ -106,7 +106,7 @@ static int test_find_delalloc(u32 sectorsize)
 	set_extent_delalloc(&tmp, 0, sectorsize - 1, 0, NULL);
 	start = 0;
 	end = 0;
-	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (!found) {
 		test_err("should have found at least one delalloc");
@@ -137,7 +137,7 @@ static int test_find_delalloc(u32 sectorsize)
 	set_extent_delalloc(&tmp, sectorsize, max_bytes - 1, 0, NULL);
 	start = test_start;
 	end = 0;
-	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (!found) {
 		test_err("couldn't find delalloc in our range");
@@ -171,7 +171,7 @@ static int test_find_delalloc(u32 sectorsize)
 	}
 	start = test_start;
 	end = 0;
-	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (found) {
 		test_err("found range when we shouldn't have");
@@ -192,7 +192,7 @@ static int test_find_delalloc(u32 sectorsize)
 	set_extent_delalloc(&tmp, max_bytes, total_dirty - 1, 0, NULL);
 	start = test_start;
 	end = 0;
-	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (!found) {
 		test_err("didn't find our range");
@@ -233,7 +233,7 @@ static int test_find_delalloc(u32 sectorsize)
 	 * this changes at any point in the future we will need to fix this
 	 * tests expected behavior.
 	 */
-	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (!found) {
 		test_err("didn't find our range");
-- 
2.18.0

[PATCH 4/4 v2] btrfs: tests: polish ifdefs around testing helper

[David Sterba]

Avoid the inline ifdefs and use two sections for self-tests enabled and
disabled.

Though there could be no ifdef and unconditional test_bit of
BTRFS_FS_STATE_DUMMY_FS_INFO, the static inline can help to optimize out
any code that would depend on conditions using btrfs_is_testing.

As this is only for the testing code, drop unlikely().

Signed-off-by: David Sterba <dsterba@suse.com>
---

v2:
- remove unlikely
- simplify to: return test_bit(...)

 fs/btrfs/ctree.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 32d2fce4ac53..1656ada9200b 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3708,17 +3708,17 @@ static inline int btrfs_defrag_cancelled(struct btrfs_fs_info *fs_info)
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 void btrfs_test_inode_set_ops(struct inode *inode);
 void btrfs_test_destroy_inode(struct inode *inode);
-#endif
 
 static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
 {
-#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-	if (unlikely(test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO,
-			      &fs_info->fs_state)))
-		return 1;
-#endif
+	return test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);
+}
+#else
+static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
+{
 	return 0;
 }
+#endif
 
 static inline void cond_wake_up(struct wait_queue_head *wq)
 {
-- 
2.18.0

Re: [PATCH 4/4 v2] btrfs: tests: polish ifdefs around testing helper

[Omar Sandoval]

On Fri, Sep 14, 2018 at 06:42:03PM +0200, David Sterba wrote:
> Avoid the inline ifdefs and use two sections for self-tests enabled and
> disabled.
> 
> Though there could be no ifdef and unconditional test_bit of
> BTRFS_FS_STATE_DUMMY_FS_INFO, the static inline can help to optimize out
> any code that would depend on conditions using btrfs_is_testing.
> 
> As this is only for the testing code, drop unlikely().

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> 
> v2:
> - remove unlikely
> - simplify to: return test_bit(...)
> 
>  fs/btrfs/ctree.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 32d2fce4ac53..1656ada9200b 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3708,17 +3708,17 @@ static inline int btrfs_defrag_cancelled(struct btrfs_fs_info *fs_info)
>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>  void btrfs_test_inode_set_ops(struct inode *inode);
>  void btrfs_test_destroy_inode(struct inode *inode);
> -#endif
>  
>  static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
>  {
> -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> -	if (unlikely(test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO,
> -			      &fs_info->fs_state)))
> -		return 1;
> -#endif
> +	return test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);
> +}
> +#else
> +static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
> +{
>  	return 0;
>  }
> +#endif
>  
>  static inline void cond_wake_up(struct wait_queue_head *wq)
>  {
> -- 
> 2.18.0
> 

Re: [PATCH 1/4 v2] btrfs: tests: add separate stub for find_lock_delalloc_range

[Omar Sandoval]

On Fri, Sep 14, 2018 at 06:38:44PM +0200, David Sterba wrote:
> The helper find_lock_delalloc_range is now conditionally built static,
> dpending on whether the self-tests are enabled or not. There's a macro
> that is supposed to hide the export, used only once. To discourage
> further use, drop it an add a public wrapper for the helper needed by
> tests.

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> 
> v2:
> - add noinline_for_stack back
>  fs/btrfs/ctree.h                 |  6 ------
>  fs/btrfs/extent_io.c             | 13 ++++++++++++-
>  fs/btrfs/extent_io.h             |  2 +-
>  fs/btrfs/tests/extent-io-tests.c | 10 +++++-----
>  4 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 2cddfe7806a4..45b7029d0f23 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -41,12 +41,6 @@ extern struct kmem_cache *btrfs_path_cachep;
>  extern struct kmem_cache *btrfs_free_space_cachep;
>  struct btrfs_ordered_sum;
>  
> -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> -#define STATIC noinline
> -#else
> -#define STATIC static noinline
> -#endif
> -
>  #define BTRFS_MAGIC 0x4D5F53665248425FULL /* ascii _BHRfS_M, no null */
>  
>  #define BTRFS_MAX_MIRRORS 3
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 4dd6faab02bb..93108b18b231 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1568,7 +1568,7 @@ static noinline int lock_delalloc_pages(struct inode *inode,
>   *
>   * 1 is returned if we find something, 0 if nothing was in the tree
>   */
> -STATIC u64 find_lock_delalloc_range(struct inode *inode,
> +static noinline_for_stack u64 find_lock_delalloc_range(struct inode *inode,
>  				    struct extent_io_tree *tree,
>  				    struct page *locked_page, u64 *start,
>  				    u64 *end, u64 max_bytes)
> @@ -1648,6 +1648,17 @@ STATIC u64 find_lock_delalloc_range(struct inode *inode,
>  	return found;
>  }
>  
> +#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> +u64 btrfs_find_lock_delalloc_range(struct inode *inode,
> +				    struct extent_io_tree *tree,
> +				    struct page *locked_page, u64 *start,
> +				    u64 *end, u64 max_bytes)
> +{
> +	return find_lock_delalloc_range(inode, tree, locked_page, start, end,
> +			max_bytes);
> +}
> +#endif
> +
>  static int __process_pages_contig(struct address_space *mapping,
>  				  struct page *locked_page,
>  				  pgoff_t start_index, pgoff_t end_index,
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index b4d03e677e1d..1a7fdcbca49b 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -546,7 +546,7 @@ int free_io_failure(struct extent_io_tree *failure_tree,
>  		    struct extent_io_tree *io_tree,
>  		    struct io_failure_record *rec);
>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> -noinline u64 find_lock_delalloc_range(struct inode *inode,
> +u64 btrfs_find_lock_delalloc_range(struct inode *inode,
>  				      struct extent_io_tree *tree,
>  				      struct page *locked_page, u64 *start,
>  				      u64 *end, u64 max_bytes);
> diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
> index d9269a531a4d..9e0f4a01be14 100644
> --- a/fs/btrfs/tests/extent-io-tests.c
> +++ b/fs/btrfs/tests/extent-io-tests.c
> @@ -106,7 +106,7 @@ static int test_find_delalloc(u32 sectorsize)
>  	set_extent_delalloc(&tmp, 0, sectorsize - 1, 0, NULL);
>  	start = 0;
>  	end = 0;
> -	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> +	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
>  					 &end, max_bytes);
>  	if (!found) {
>  		test_err("should have found at least one delalloc");
> @@ -137,7 +137,7 @@ static int test_find_delalloc(u32 sectorsize)
>  	set_extent_delalloc(&tmp, sectorsize, max_bytes - 1, 0, NULL);
>  	start = test_start;
>  	end = 0;
> -	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> +	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
>  					 &end, max_bytes);
>  	if (!found) {
>  		test_err("couldn't find delalloc in our range");
> @@ -171,7 +171,7 @@ static int test_find_delalloc(u32 sectorsize)
>  	}
>  	start = test_start;
>  	end = 0;
> -	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> +	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
>  					 &end, max_bytes);
>  	if (found) {
>  		test_err("found range when we shouldn't have");
> @@ -192,7 +192,7 @@ static int test_find_delalloc(u32 sectorsize)
>  	set_extent_delalloc(&tmp, max_bytes, total_dirty - 1, 0, NULL);
>  	start = test_start;
>  	end = 0;
> -	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> +	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
>  					 &end, max_bytes);
>  	if (!found) {
>  		test_err("didn't find our range");
> @@ -233,7 +233,7 @@ static int test_find_delalloc(u32 sectorsize)
>  	 * this changes at any point in the future we will need to fix this
>  	 * tests expected behavior.
>  	 */
> -	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> +	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
>  					 &end, max_bytes);
>  	if (!found) {
>  		test_err("didn't find our range");
> -- 
> 2.18.0
>