[PATCH v2 0/5] btrfs: fix compiler warning with make W=1

[PATCH v2 0/5] btrfs: fix compiler warning with make W=1

[Johannes Thumshirn]

This patchset fixes most of the compiler warnings encountered when building
btrfs with make W=1.

There are two more compiler warnings left in raid56.c:
  CC [M]  fs/btrfs/raid56.o
fs/btrfs/raid56.c: In function ‘finish_rmw’:
fs/btrfs/raid56.c:1185:6: warning: variable ‘p_stripe’ set but not used [-Wunused-but-set-variable]
  int p_stripe = -1;
      ^
fs/btrfs/raid56.c: In function ‘finish_parity_scrub’:
fs/btrfs/raid56.c:2343:6: warning: variable ‘p_stripe’ set but not used [-Wunused-but-set-variable]
  int p_stripe = -1;
      ^
but I'm currently unsure how an appropriate fix would look like. As far as I
can tell these variables have always been unused since they have been
introduced.

There are still warnings left emitted by kernel-doc but these are subject to
another patchset, this one only addresses the warnings generated by gcc.

Changes to v1:
* Added Reviews
* Dropped already applied patches 4 + 5
* Added EXPORT_FOR_TESTS + users

Johannes Thumshirn (5):
  btrfs: remove unused drop_on_err in btrfs_mkdir()
  btrfs: remove set but not used variable err in btrfs_add_link
  btrfs: remove unused function btrfs_sysfs_feature_update()
  btrfs: introduce EXPORT_FOR_TESTS macro
  btrfs: use EXPORT_FOR_TESTS for conditionally shared functions

 fs/btrfs/ctree.h                 |  6 ++++++
 fs/btrfs/extent_io.c             | 14 ++------------
 fs/btrfs/extent_io.h             |  7 +++----
 fs/btrfs/free-space-tree.c       |  8 ++++++--
 fs/btrfs/inode.c                 | 16 ++++++----------
 fs/btrfs/sysfs.c                 | 33 ---------------------------------
 fs/btrfs/sysfs.h                 |  2 --
 fs/btrfs/tests/extent-io-tests.c | 10 +++++-----
 8 files changed, 28 insertions(+), 68 deletions(-)

-- 
2.16.4

[PATCH v2 1/5] btrfs: remove unused drop_on_err in btrfs_mkdir()

[Johannes Thumshirn]

Up to commit 32955c5422a8 (btrfs: switch to discard_new_inode()) the
drop_on_err variable in btrfs_mkdir() was used to check whether the inode had
to be dropped via iput().

After commit 32955c5422a8 (btrfs: switch to discard_new_inode())
discard_new_inode() is called when err is set and inode is non NULL. Therefore
drop_on_err is not used anymore and thus causes a warning when building with
-Wunused-but-set-variable.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Reviewed-by: Omar Sandoval <osandov@fb.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 2585200cb43a..9becf8543489 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6645,7 +6645,6 @@ static int btrfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root = BTRFS_I(dir)->root;
 	int err = 0;
-	int drop_on_err = 0;
 	u64 objectid = 0;
 	u64 index = 0;
 
@@ -6671,7 +6670,6 @@ static int btrfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 		goto out_fail;
 	}
 
-	drop_on_err = 1;
 	/* these must be set before we unlock the inode */
 	inode->i_op = &btrfs_dir_inode_operations;
 	inode->i_fop = &btrfs_dir_file_operations;
@@ -6692,7 +6690,6 @@ static int btrfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 		goto out_fail;
 
 	d_instantiate_new(dentry, inode);
-	drop_on_err = 0;
 
 out_fail:
 	btrfs_end_transaction(trans);
-- 
2.16.4

[PATCH v2 2/5] btrfs: remove set but not used variable err in btrfs_add_link

[Johannes Thumshirn]

err holds the return value of either btrfs_del_root_ref() or
btrfs_del_inode_ref() but it hasn't been checked since it's introduction with
commit fe66a05a0679 (Btrfs: improve error handling for btrfs_insert_dir_item
callers) in 2012.

As the error value hasn't been of any interest for 6 years we can just drop it
as well.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 9becf8543489..7f56a235c0b1 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6395,17 +6395,16 @@ int btrfs_add_link(struct btrfs_trans_handle *trans,
 fail_dir_item:
 	if (unlikely(ino == BTRFS_FIRST_FREE_OBJECTID)) {
 		u64 local_index;
-		int err;
-		err = btrfs_del_root_ref(trans, key.objectid,
-					 root->root_key.objectid, parent_ino,
-					 &local_index, name, name_len);
+
+		btrfs_del_root_ref(trans, key.objectid,
+				   root->root_key.objectid, parent_ino,
+				   &local_index, name, name_len);
 
 	} else if (add_backref) {
 		u64 local_index;
-		int err;
 
-		err = btrfs_del_inode_ref(trans, root, name, name_len,
-					  ino, parent_ino, &local_index);
+		btrfs_del_inode_ref(trans, root, name, name_len,
+				    ino, parent_ino, &local_index);
 	}
 	return ret;
 }
-- 
2.16.4

[PATCH v2 3/5] btrfs: remove unused function btrfs_sysfs_feature_update()

[Johannes Thumshirn]

btrfs_sysfs_feature_update() was introduced with commit 444e75169872 (btrfs:
sysfs: introduce helper for syncing bits with sysfs files) to provide a helper
which was used in 14e46e04958d (btrfs: synchronize incompat feature bits with
sysfs files).

But commit e410e34fad91 (Revert "btrfs: synchronize incompat feature bits with
sysfs files") reverted 14e46e04958d so btrfs_sysfs_feature_update() ended up
as an unused function.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Reviewed-by: Omar Sandoval <osandov@fb.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/sysfs.c | 33 ---------------------------------
 fs/btrfs/sysfs.h |  2 --
 2 files changed, 35 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 3717c864ba23..a22a7c5f75eb 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -858,39 +858,6 @@ int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info)
 	return error;
 }
 
-
-/*
- * Change per-fs features in /sys/fs/btrfs/UUID/features to match current
- * values in superblock. Call after any changes to incompat/compat_ro flags
- */
-void btrfs_sysfs_feature_update(struct btrfs_fs_info *fs_info,
-		u64 bit, enum btrfs_feature_set set)
-{
-	struct btrfs_fs_devices *fs_devs;
-	struct kobject *fsid_kobj;
-	u64 features;
-	int ret;
-
-	if (!fs_info)
-		return;
-
-	features = get_features(fs_info, set);
-	ASSERT(bit & supported_feature_masks[set]);
-
-	fs_devs = fs_info->fs_devices;
-	fsid_kobj = &fs_devs->fsid_kobj;
-
-	if (!fsid_kobj->state_initialized)
-		return;
-
-	/*
-	 * FIXME: this is too heavy to update just one value, ideally we'd like
-	 * to use sysfs_update_group but some refactoring is needed first.
-	 */
-	sysfs_remove_group(fsid_kobj, &btrfs_feature_attr_group);
-	ret = sysfs_create_group(fsid_kobj, &btrfs_feature_attr_group);
-}
-
 static int btrfs_init_debugfs(void)
 {
 #ifdef CONFIG_DEBUG_FS
diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
index c6ee600aff89..93feedde8485 100644
--- a/fs/btrfs/sysfs.h
+++ b/fs/btrfs/sysfs.h
@@ -88,7 +88,5 @@ int btrfs_sysfs_add_fsid(struct btrfs_fs_devices *fs_devs,
 				struct kobject *parent);
 int btrfs_sysfs_add_device(struct btrfs_fs_devices *fs_devs);
 void btrfs_sysfs_remove_fsid(struct btrfs_fs_devices *fs_devs);
-void btrfs_sysfs_feature_update(struct btrfs_fs_info *fs_info,
-		u64 bit, enum btrfs_feature_set set);
 
 #endif
-- 
2.16.4

[PATCH v2 4/5] btrfs: introduce EXPORT_FOR_TESTS macro

[Johannes Thumshirn]

Depending on whether CONFIG_BTRFS_FS_RUN_SANITY_TESTS is set, some BTRFS
functions are either local to the file they are implemented in and thus
should be declared static or are called from within the test implementation
defined in a different file.

Introduce an EXPORT_FOR_TESTS macro which depending on
CONFIG_BTRFS_FS_RUN_SANITY_TESTS either adds the 'static' keyword to a
function or not.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/ctree.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index e62824cae00a..5119dacab981 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3471,6 +3471,12 @@ static inline void assfail(const char *expr, const char *file, int line)
 #define ASSERT(expr)	((void)0)
 #endif
 
+#ifndef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
+#define EXPORT_FOR_TESTS static
+#else
+#define EXPORT_FOR_TESTS
+#endif
+
 __cold
 static inline void btrfs_print_v0_err(struct btrfs_fs_info *fs_info)
 {
-- 
2.16.4

[PATCH v2 5/5] btrfs: use EXPORT_FOR_TESTS for conditionally shared functions

[Johannes Thumshirn]

Several functions in BTRFS are only used inside the source file they are
declared if CONFIG_BTRFS_FS_RUN_SANITY_TESTS is not defined. However if
CONFIG_BTRFS_FS_RUN_SANITY_TESTS is defined these functions are shared with
the unit tests code.

Before the introduction of the EXPORT_FOR_TESTS macro, these functions
could not be declared as static and the compiler had a harder task when
optimizing and inlining them.

As we have EXPORT_FOR_TESTS now, use it where appropriate to support the
compiler.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/extent_io.c             | 14 ++------------
 fs/btrfs/extent_io.h             |  7 +++----
 fs/btrfs/free-space-tree.c       |  8 ++++++--
 fs/btrfs/tests/extent-io-tests.c | 10 +++++-----
 4 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 926bf30c2f2e..16344d5b8d68 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1556,7 +1556,8 @@ static noinline int lock_delalloc_pages(struct inode *inode,
  *
  * 1 is returned if we find something, 0 if nothing was in the tree
  */
-static noinline_for_stack u64 find_lock_delalloc_range(struct inode *inode,
+EXPORT_FOR_TESTS
+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)
@@ -1636,17 +1637,6 @@ static noinline_for_stack 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 d96fd534f144..a03204a81751 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -522,10 +522,9 @@ 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
-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);
+u64 find_lock_delalloc_range(struct inode *inode, struct extent_io_tree *tree,
+			     struct page *locked_page, u64 *start,
+			     u64 *end, u64 max_bytes);
 #endif
 struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
 					       u64 start);
diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index d6736595ec57..cacdebc26fa0 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -74,7 +74,7 @@ static int add_new_free_space_info(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-struct btrfs_free_space_info *
+EXPORT_FOR_TESTS struct btrfs_free_space_info *
 search_free_space_info(struct btrfs_trans_handle *trans,
 		       struct btrfs_fs_info *fs_info,
 		       struct btrfs_block_group_cache *block_group,
@@ -176,6 +176,7 @@ static void le_bitmap_set(unsigned long *map, unsigned int start, int len)
 	}
 }
 
+EXPORT_FOR_TESTS
 int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 				  struct btrfs_block_group_cache *block_group,
 				  struct btrfs_path *path)
@@ -315,6 +316,7 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
+EXPORT_FOR_TESTS
 int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 				  struct btrfs_block_group_cache *block_group,
 				  struct btrfs_path *path)
@@ -487,6 +489,7 @@ static int update_free_space_extent_count(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
+EXPORT_FOR_TESTS
 int free_space_test_bit(struct btrfs_block_group_cache *block_group,
 			struct btrfs_path *path, u64 offset)
 {
@@ -775,6 +778,7 @@ static int remove_free_space_extent(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
+EXPORT_FOR_TESTS
 int __remove_from_free_space_tree(struct btrfs_trans_handle *trans,
 				  struct btrfs_block_group_cache *block_group,
 				  struct btrfs_path *path, u64 start, u64 size)
@@ -968,7 +972,7 @@ static int add_free_space_extent(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-int __add_to_free_space_tree(struct btrfs_trans_handle *trans,
+EXPORT_FOR_TESTS int __add_to_free_space_tree(struct btrfs_trans_handle *trans,
 			     struct btrfs_block_group_cache *block_group,
 			     struct btrfs_path *path, u64 start, u64 size)
 {
diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
index a99dc04331b4..ab7771b2a5fb 100644
--- a/fs/btrfs/tests/extent-io-tests.c
+++ b/fs/btrfs/tests/extent-io-tests.c
@@ -107,7 +107,7 @@ static int test_find_delalloc(u32 sectorsize)
 	set_extent_delalloc(&tmp, 0, sectorsize - 1, 0, NULL);
 	start = 0;
 	end = 0;
-	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (!found) {
 		test_err("should have found at least one delalloc");
@@ -138,7 +138,7 @@ static int test_find_delalloc(u32 sectorsize)
 	set_extent_delalloc(&tmp, sectorsize, max_bytes - 1, 0, NULL);
 	start = test_start;
 	end = 0;
-	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (!found) {
 		test_err("couldn't find delalloc in our range");
@@ -172,7 +172,7 @@ static int test_find_delalloc(u32 sectorsize)
 	}
 	start = test_start;
 	end = 0;
-	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (found) {
 		test_err("found range when we shouldn't have");
@@ -193,7 +193,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 = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (!found) {
 		test_err("didn't find our range");
@@ -234,7 +234,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 = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
+	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
 					 &end, max_bytes);
 	if (!found) {
 		test_err("didn't find our range");
-- 
2.16.4

Re: [PATCH v2 4/5] btrfs: introduce EXPORT_FOR_TESTS macro

[Nikolay Borisov]

On 19.11.18 г. 11:38 ч., Johannes Thumshirn wrote:
> Depending on whether CONFIG_BTRFS_FS_RUN_SANITY_TESTS is set, some BTRFS
> functions are either local to the file they are implemented in and thus
> should be declared static or are called from within the test implementation
> defined in a different file.
> 
> Introduce an EXPORT_FOR_TESTS macro which depending on
> CONFIG_BTRFS_FS_RUN_SANITY_TESTS either adds the 'static' keyword to a
> function or not.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/ctree.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index e62824cae00a..5119dacab981 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3471,6 +3471,12 @@ static inline void assfail(const char *expr, const char *file, int line)
>  #define ASSERT(expr)	((void)0)
>  #endif
>  
> +#ifndef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> +#define EXPORT_FOR_TESTS static
> +#else
> +#define EXPORT_FOR_TESTS
> +#endif
> +
>  __cold
>  static inline void btrfs_print_v0_err(struct btrfs_fs_info *fs_info)
>  {
> 

Re: [PATCH v2 5/5] btrfs: use EXPORT_FOR_TESTS for conditionally shared functions

[Nikolay Borisov]

On 19.11.18 г. 11:38 ч., Johannes Thumshirn wrote:
> Several functions in BTRFS are only used inside the source file they are
> declared if CONFIG_BTRFS_FS_RUN_SANITY_TESTS is not defined. However if
> CONFIG_BTRFS_FS_RUN_SANITY_TESTS is defined these functions are shared with
> the unit tests code.
> 
> Before the introduction of the EXPORT_FOR_TESTS macro, these functions
> could not be declared as static and the compiler had a harder task when
> optimizing and inlining them.
> 
> As we have EXPORT_FOR_TESTS now, use it where appropriate to support the
> compiler.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/extent_io.c             | 14 ++------------
>  fs/btrfs/extent_io.h             |  7 +++----
>  fs/btrfs/free-space-tree.c       |  8 ++++++--
>  fs/btrfs/tests/extent-io-tests.c | 10 +++++-----
>  4 files changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 926bf30c2f2e..16344d5b8d68 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1556,7 +1556,8 @@ static noinline int lock_delalloc_pages(struct inode *inode,
>   *
>   * 1 is returned if we find something, 0 if nothing was in the tree
>   */
> -static noinline_for_stack u64 find_lock_delalloc_range(struct inode *inode,
> +EXPORT_FOR_TESTS
> +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)
> @@ -1636,17 +1637,6 @@ static noinline_for_stack 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 d96fd534f144..a03204a81751 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -522,10 +522,9 @@ 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
> -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);
> +u64 find_lock_delalloc_range(struct inode *inode, struct extent_io_tree *tree,
> +			     struct page *locked_page, u64 *start,
> +			     u64 *end, u64 max_bytes);
>  #endif
>  struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
>  					       u64 start);
> diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> index d6736595ec57..cacdebc26fa0 100644
> --- a/fs/btrfs/free-space-tree.c
> +++ b/fs/btrfs/free-space-tree.c
> @@ -74,7 +74,7 @@ static int add_new_free_space_info(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> -struct btrfs_free_space_info *
> +EXPORT_FOR_TESTS struct btrfs_free_space_info *
>  search_free_space_info(struct btrfs_trans_handle *trans,
>  		       struct btrfs_fs_info *fs_info,
>  		       struct btrfs_block_group_cache *block_group,
> @@ -176,6 +176,7 @@ static void le_bitmap_set(unsigned long *map, unsigned int start, int len)
>  	}
>  }
>  
> +EXPORT_FOR_TESTS
>  int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
>  				  struct btrfs_block_group_cache *block_group,
>  				  struct btrfs_path *path)
> @@ -315,6 +316,7 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> +EXPORT_FOR_TESTS
>  int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
>  				  struct btrfs_block_group_cache *block_group,
>  				  struct btrfs_path *path)
> @@ -487,6 +489,7 @@ static int update_free_space_extent_count(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> +EXPORT_FOR_TESTS
>  int free_space_test_bit(struct btrfs_block_group_cache *block_group,
>  			struct btrfs_path *path, u64 offset)
>  {
> @@ -775,6 +778,7 @@ static int remove_free_space_extent(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> +EXPORT_FOR_TESTS
>  int __remove_from_free_space_tree(struct btrfs_trans_handle *trans,
>  				  struct btrfs_block_group_cache *block_group,
>  				  struct btrfs_path *path, u64 start, u64 size)
> @@ -968,7 +972,7 @@ static int add_free_space_extent(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> -int __add_to_free_space_tree(struct btrfs_trans_handle *trans,
> +EXPORT_FOR_TESTS int __add_to_free_space_tree(struct btrfs_trans_handle *trans,
>  			     struct btrfs_block_group_cache *block_group,
>  			     struct btrfs_path *path, u64 start, u64 size)
>  {
> diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
> index a99dc04331b4..ab7771b2a5fb 100644
> --- a/fs/btrfs/tests/extent-io-tests.c
> +++ b/fs/btrfs/tests/extent-io-tests.c
> @@ -107,7 +107,7 @@ static int test_find_delalloc(u32 sectorsize)
>  	set_extent_delalloc(&tmp, 0, sectorsize - 1, 0, NULL);
>  	start = 0;
>  	end = 0;
> -	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> +	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
>  					 &end, max_bytes);
>  	if (!found) {
>  		test_err("should have found at least one delalloc");
> @@ -138,7 +138,7 @@ static int test_find_delalloc(u32 sectorsize)
>  	set_extent_delalloc(&tmp, sectorsize, max_bytes - 1, 0, NULL);
>  	start = test_start;
>  	end = 0;
> -	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> +	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
>  					 &end, max_bytes);
>  	if (!found) {
>  		test_err("couldn't find delalloc in our range");
> @@ -172,7 +172,7 @@ static int test_find_delalloc(u32 sectorsize)
>  	}
>  	start = test_start;
>  	end = 0;
> -	found = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> +	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
>  					 &end, max_bytes);
>  	if (found) {
>  		test_err("found range when we shouldn't have");
> @@ -193,7 +193,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 = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> +	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
>  					 &end, max_bytes);
>  	if (!found) {
>  		test_err("didn't find our range");
> @@ -234,7 +234,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 = btrfs_find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> +	found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
>  					 &end, max_bytes);
>  	if (!found) {
>  		test_err("didn't find our range");
> 

Re: [PATCH v2 2/5] btrfs: remove set but not used variable err in btrfs_add_link

[David Sterba]

On Mon, Nov 19, 2018 at 10:38:14AM +0100, Johannes Thumshirn wrote:
> err holds the return value of either btrfs_del_root_ref() or
> btrfs_del_inode_ref() but it hasn't been checked since it's introduction with
> commit fe66a05a0679 (Btrfs: improve error handling for btrfs_insert_dir_item
> callers) in 2012.
> 
> As the error value hasn't been of any interest for 6 years we can just drop it
> as well.

No, I think there should actually be proper error hanling. This is on
the error handling path already so this might be tricky and could
require restructuring btrfs_add_link.

The error handling is not done everywhere it should and such
inconsistencies are more like a hint to improve that than to count the
years of negligence.  Code removal patches should come with a brief
analysis why the code can be safely dropped. You did that, which is
great, but the conclusion was wrong.

Re: [PATCH v2 3/5] btrfs: remove unused function btrfs_sysfs_feature_update()

[David Sterba]

On Mon, Nov 19, 2018 at 10:38:15AM +0100, Johannes Thumshirn wrote:
> btrfs_sysfs_feature_update() was introduced with commit 444e75169872 (btrfs:
> sysfs: introduce helper for syncing bits with sysfs files) to provide a helper
> which was used in 14e46e04958d (btrfs: synchronize incompat feature bits with
> sysfs files).
> 
> But commit e410e34fad91 (Revert "btrfs: synchronize incompat feature bits with
> sysfs files") reverted 14e46e04958d so btrfs_sysfs_feature_update() ended up
> as an unused function.

    This ends up doing sysfs operations from deep in balance (where we
    should be GFP_NOFS) and under heavy balance load, we're making races
    against sysfs internals.

    Revert it for now while we figure things out.

With the memalloc_nofs_save/memalloc_nofs_restore we can get address the
GFP_NOFS problem, but I don't remember what was the other part about sysfs
internals.

The sysfs bits for features should be match the internal state so this
is a (low priority) bug, I'd prefer fixing it instead of removing the
code.

Re: [PATCH v2 0/5] btrfs: fix compiler warning with make W=1

[David Sterba]

On Mon, Nov 19, 2018 at 10:38:12AM +0100, Johannes Thumshirn wrote:
> This patchset fixes most of the compiler warnings encountered when building
> btrfs with make W=1.
> 
> There are two more compiler warnings left in raid56.c:
>   CC [M]  fs/btrfs/raid56.o
> fs/btrfs/raid56.c: In function ‘finish_rmw’:
> fs/btrfs/raid56.c:1185:6: warning: variable ‘p_stripe’ set but not used [-Wunused-but-set-variable]
>   int p_stripe = -1;
>       ^
> fs/btrfs/raid56.c: In function ‘finish_parity_scrub’:
> fs/btrfs/raid56.c:2343:6: warning: variable ‘p_stripe’ set but not used [-Wunused-but-set-variable]
>   int p_stripe = -1;
>       ^
> but I'm currently unsure how an appropriate fix would look like. As far as I
> can tell these variables have always been unused since they have been
> introduced.

I don't know either, it looks like they were caluclated as indexes to
the stripe array but not used. And I don't see that there would be
code missing them so removing them is probably ok. The RAID56 code
hasn't been touched for years so such gems can be found there.

> There are still warnings left emitted by kernel-doc but these are subject to
> another patchset, this one only addresses the warnings generated by gcc.

The kernel-doc warnings are an interesting topic. As there are no public
functions that would end up in some nice documentation, their use is
sparse and inconsistent and my firt idea was to drop them completely.

OTOH, if there are kdoc comments, the build checks will make sure the
argument names match the documentation, besides that we can also write
more comments to functions where it matters.

So, let's fix them and add more where the kdoc comments just lacks the
"/**" marker, and then incrementally update the rest.

> Changes to v1:
> * Added Reviews
> * Dropped already applied patches 4 + 5
> * Added EXPORT_FOR_TESTS + users
> 
> Johannes Thumshirn (5):
>   btrfs: remove unused drop_on_err in btrfs_mkdir()
>   btrfs: remove set but not used variable err in btrfs_add_link
>   btrfs: remove unused function btrfs_sysfs_feature_update()
>   btrfs: introduce EXPORT_FOR_TESTS macro
>   btrfs: use EXPORT_FOR_TESTS for conditionally shared functions

Patches 1, 4 and 5 merged, the other have comments.