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

[PATCH 0/6] 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.

Johannes Thumshirn (6):
  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: remove unused variable tree in bio_readpage_error()
  btrfs: remove unused variable tree in end_compressed_bio_write()
  btrfs: unconditionally provide function prototypes from
    free-space-tree.h

 fs/btrfs/compression.c     |  2 --
 fs/btrfs/extent_io.c       |  3 ---
 fs/btrfs/free-space-tree.h |  2 --
 fs/btrfs/inode.c           | 16 ++++++----------
 fs/btrfs/sysfs.c           | 33 ---------------------------------
 fs/btrfs/sysfs.h           |  2 --
 6 files changed, 6 insertions(+), 52 deletions(-)

-- 
2.16.4

[PATCH 1/6] 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>
---
 fs/btrfs/inode.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 4a2f9f7fd96e..7d17b0a654e6 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6677,7 +6677,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;
 
@@ -6703,7 +6702,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;
@@ -6724,7 +6722,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 2/6] 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>
---
 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 7d17b0a654e6..57af243b3f59 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6427,17 +6427,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 3/6] 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>
---
 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 4/6] btrfs: remove unused variable tree in bio_readpage_error()

[Johannes Thumshirn]

Commit 2922040236f9 (btrfs: Remove extent_io_ops::writepage_end_io_hook)
removed the indirection to extent_io_ops::writepage_end_io_hook but didn't
remove the tree variable which then became unused.

Remove 'tree' as well to silence the warning when -Wunused-but-set-variable is
used to compile btrfs.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/extent_io.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 0f8f9c035812..17a15cc6b542 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2403,11 +2403,8 @@ static int bio_readpage_error(struct bio *failed_bio, u64 phy_offset,
 void end_extent_writepage(struct page *page, int err, u64 start, u64 end)
 {
 	int uptodate = (err == 0);
-	struct extent_io_tree *tree;
 	int ret = 0;
 
-	tree = &BTRFS_I(page->mapping->host)->io_tree;
-
 	btrfs_writepage_endio_finish_ordered(page, start, end, NULL, uptodate);
 
 	if (!uptodate) {
-- 
2.16.4

[PATCH 5/6] btrfs: remove unused variable tree in end_compressed_bio_write()

[Johannes Thumshirn]

Commit 2922040236f9 (btrfs: Remove extent_io_ops::writepage_end_io_hook)
removed the indirection to extent_io_ops::writepage_end_io_hook but didn't
remove the tree variable which then became unused.

Remove 'tree' as well to silence the warning when -Wunused-but-set-variable
is used to compile btrfs.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/compression.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index bde8d0487bbb..088570c5dfb8 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -229,7 +229,6 @@ static noinline void end_compressed_writeback(struct inode *inode,
  */
 static void end_compressed_bio_write(struct bio *bio)
 {
-	struct extent_io_tree *tree;
 	struct compressed_bio *cb = bio->bi_private;
 	struct inode *inode;
 	struct page *page;
@@ -248,7 +247,6 @@ static void end_compressed_bio_write(struct bio *bio)
 	 * call back into the FS and do all the end_io operations
 	 */
 	inode = cb->inode;
-	tree = &BTRFS_I(inode)->io_tree;
 	cb->compressed_pages[0]->mapping = cb->inode->i_mapping;
 	btrfs_writepage_endio_finish_ordered(cb->compressed_pages[0],
 			cb->start, cb->start + cb->len - 1, NULL,
-- 
2.16.4

[PATCH 6/6] btrfs: unconditionally provide function prototypes from free-space-tree.h

[Johannes Thumshirn]

Currently the function prototypes of:
* search_free_space_info()
* convert_free_space_to_bitmaps()
* convert_free_space_to_extents()
* free_space_test_bit()
* __remove_from_free_space_tree()
* __add_to_free_space_tree()
are hidden behind CONFIG_BTRFS_FS_RUN_SANITY_TESTS.

If CONFIG_BTRFS_FS_RUN_SANITY_TESTS is not set, these functions are only
used within free-space-tree.c so technically there would be no need to provide
the function prototypes. But when CONFIG_BTRFS_FS_RUN_SANITY_TESTS they are
also used from tests/free-space-tree-tests.c which explains the need for
the function prototypes and the non-static declaration of the functions.

When compiling with -Wmissing-prototypes and
CONFIG_BTRFS_FS_RUN_SANITY_TESTS=n gcc emits a warning for each of these
functions, so remove the ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS guard to
make the compiler happy when running 'make W=1'.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/free-space-tree.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/free-space-tree.h b/fs/btrfs/free-space-tree.h
index 3133651d7d70..0f4db2ad68c8 100644
--- a/fs/btrfs/free-space-tree.h
+++ b/fs/btrfs/free-space-tree.h
@@ -27,7 +27,6 @@ int add_to_free_space_tree(struct btrfs_trans_handle *trans,
 int remove_from_free_space_tree(struct btrfs_trans_handle *trans,
 				u64 start, u64 size);
 
-#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 struct btrfs_free_space_info *
 search_free_space_info(struct btrfs_trans_handle *trans,
 		       struct btrfs_fs_info *fs_info,
@@ -47,6 +46,5 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 				  struct btrfs_path *path);
 int free_space_test_bit(struct btrfs_block_group_cache *block_group,
 			struct btrfs_path *path, u64 offset);
-#endif
 
 #endif
-- 
2.16.4

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

[Nikolay Borisov]

On 14.11.18 г. 15:35 ч., 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.
> 
> 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.
> 
> Johannes Thumshirn (6):
>   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: remove unused variable tree in bio_readpage_error()
>   btrfs: remove unused variable tree in end_compressed_bio_write()
>   btrfs: unconditionally provide function prototypes from
>     free-space-tree.h
> 
>  fs/btrfs/compression.c     |  2 --
>  fs/btrfs/extent_io.c       |  3 ---
>  fs/btrfs/free-space-tree.h |  2 --
>  fs/btrfs/inode.c           | 16 ++++++----------
>  fs/btrfs/sysfs.c           | 33 ---------------------------------
>  fs/btrfs/sysfs.h           |  2 --
>  6 files changed, 6 insertions(+), 52 deletions(-)

For the whole series:

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

However, I have some comments for some of the patches.

> 

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

[Nikolay Borisov]

On 14.11.18 г. 15:35 ч., 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.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>

Technically you are correct, given this is in error path I guess we can
live by ignoring error from those functions in this context. Ideally we
should strive to actually handle error wherever we can.

> ---
>  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 7d17b0a654e6..57af243b3f59 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6427,17 +6427,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;
>  }
> 

Re: [PATCH 6/6] btrfs: unconditionally provide function prototypes from free-space-tree.h

[Nikolay Borisov]

On 14.11.18 г. 15:35 ч., Johannes Thumshirn wrote:
> Currently the function prototypes of:
> * search_free_space_info()
> * convert_free_space_to_bitmaps()
> * convert_free_space_to_extents()
> * free_space_test_bit()
> * __remove_from_free_space_tree()
> * __add_to_free_space_tree()
> are hidden behind CONFIG_BTRFS_FS_RUN_SANITY_TESTS.
> 
> If CONFIG_BTRFS_FS_RUN_SANITY_TESTS is not set, these functions are only
> used within free-space-tree.c so technically there would be no need to provide
> the function prototypes. But when CONFIG_BTRFS_FS_RUN_SANITY_TESTS they are
> also used from tests/free-space-tree-tests.c which explains the need for
> the function prototypes and the non-static declaration of the functions.
> 
> When compiling with -Wmissing-prototypes and
> CONFIG_BTRFS_FS_RUN_SANITY_TESTS=n gcc emits a warning for each of these
> functions, so remove the ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS guard to
> make the compiler happy when running 'make W=1'.

I agree with this patch, however you go into the gray area of
"everything which is exported should have btrfs_ prefix". It's up to
David to see if he is content with this change.

> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  fs/btrfs/free-space-tree.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/btrfs/free-space-tree.h b/fs/btrfs/free-space-tree.h
> index 3133651d7d70..0f4db2ad68c8 100644
> --- a/fs/btrfs/free-space-tree.h
> +++ b/fs/btrfs/free-space-tree.h
> @@ -27,7 +27,6 @@ int add_to_free_space_tree(struct btrfs_trans_handle *trans,
>  int remove_from_free_space_tree(struct btrfs_trans_handle *trans,
>  				u64 start, u64 size);
>  
> -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>  struct btrfs_free_space_info *
>  search_free_space_info(struct btrfs_trans_handle *trans,
>  		       struct btrfs_fs_info *fs_info,
> @@ -47,6 +46,5 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
>  				  struct btrfs_path *path);
>  int free_space_test_bit(struct btrfs_block_group_cache *block_group,
>  			struct btrfs_path *path, u64 offset);
> -#endif
>  
>  #endif
> 

Re: [PATCH 6/6] btrfs: unconditionally provide function prototypes from free-space-tree.h

[Johannes Thumshirn]

On 14/11/2018 14:52, Nikolay Borisov wrote:
> 
> I agree with this patch, however you go into the gray area of
> "everything which is exported should have btrfs_ prefix". It's up to
> David to see if he is content with this change.

Yep you're right. I think I should change it, but I'll wait for David's
response first.

Byte,
	Johannes
-- 
Johannes Thumshirn                                        SUSE Labs
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 1/6] btrfs: remove unused drop_on_err in btrfs_mkdir()

[Omar Sandoval]

On Wed, Nov 14, 2018 at 02:35:15PM +0100, Johannes Thumshirn wrote:
> 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.

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

> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  fs/btrfs/inode.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 4a2f9f7fd96e..7d17b0a654e6 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6677,7 +6677,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;
>  
> @@ -6703,7 +6702,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;
> @@ -6724,7 +6722,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
> 

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

[Omar Sandoval]

On Wed, Nov 14, 2018 at 02:35:17PM +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.

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

> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  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
> 

Re: [PATCH 4/6] btrfs: remove unused variable tree in bio_readpage_error()

[Omar Sandoval]

On Wed, Nov 14, 2018 at 02:35:18PM +0100, Johannes Thumshirn wrote:
> Commit 2922040236f9 (btrfs: Remove extent_io_ops::writepage_end_io_hook)
> removed the indirection to extent_io_ops::writepage_end_io_hook but didn't
> remove the tree variable which then became unused.
> 
> Remove 'tree' as well to silence the warning when -Wunused-but-set-variable is
> used to compile btrfs.

The subject says bio_readpage_error() but this is
end_extent_writepage(). Otherwise,

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

> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  fs/btrfs/extent_io.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 0f8f9c035812..17a15cc6b542 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2403,11 +2403,8 @@ static int bio_readpage_error(struct bio *failed_bio, u64 phy_offset,
>  void end_extent_writepage(struct page *page, int err, u64 start, u64 end)
>  {
>  	int uptodate = (err == 0);
> -	struct extent_io_tree *tree;
>  	int ret = 0;
>  
> -	tree = &BTRFS_I(page->mapping->host)->io_tree;
> -
>  	btrfs_writepage_endio_finish_ordered(page, start, end, NULL, uptodate);
>  
>  	if (!uptodate) {
> -- 
> 2.16.4
> 

Re: [PATCH 5/6] btrfs: remove unused variable tree in end_compressed_bio_write()

[Omar Sandoval]

On Wed, Nov 14, 2018 at 02:35:19PM +0100, Johannes Thumshirn wrote:
> Commit 2922040236f9 (btrfs: Remove extent_io_ops::writepage_end_io_hook)
> removed the indirection to extent_io_ops::writepage_end_io_hook but didn't
> remove the tree variable which then became unused.
> 
> Remove 'tree' as well to silence the warning when -Wunused-but-set-variable
> is used to compile btrfs.

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

> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  fs/btrfs/compression.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index bde8d0487bbb..088570c5dfb8 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -229,7 +229,6 @@ static noinline void end_compressed_writeback(struct inode *inode,
>   */
>  static void end_compressed_bio_write(struct bio *bio)
>  {
> -	struct extent_io_tree *tree;
>  	struct compressed_bio *cb = bio->bi_private;
>  	struct inode *inode;
>  	struct page *page;
> @@ -248,7 +247,6 @@ static void end_compressed_bio_write(struct bio *bio)
>  	 * call back into the FS and do all the end_io operations
>  	 */
>  	inode = cb->inode;
> -	tree = &BTRFS_I(inode)->io_tree;
>  	cb->compressed_pages[0]->mapping = cb->inode->i_mapping;
>  	btrfs_writepage_endio_finish_ordered(cb->compressed_pages[0],
>  			cb->start, cb->start + cb->len - 1, NULL,
> -- 
> 2.16.4
> 

Re: [PATCH 6/6] btrfs: unconditionally provide function prototypes from free-space-tree.h

[David Sterba]

On Wed, Nov 14, 2018 at 02:54:37PM +0100, Johannes Thumshirn wrote:
> On 14/11/2018 14:52, Nikolay Borisov wrote:
> > I agree with this patch, however you go into the gray area of
> > "everything which is exported should have btrfs_ prefix". It's up to
> > David to see if he is content with this change.
> 
> Yep you're right. I think I should change it, but I'll wait for David's
> response first.

There's one prior example of the conditionally exported functions:
btrfs_find_lock_delalloc_range . This function declaration and
definition are under the ifdef and it's a simple wrapper around a static
function find_lock_delalloc_range.

I'd do the same for the functions in your list, where applies. A static
function can be better optimized and I don't want to make it harder for
the compiler just to have it exported for the tests. They're not enabled
by default and never in production builds.

So:

free-space-tree.h:

#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS

struct btrfs_free_space_info *btrfs_search_free_space_info(
	struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info,
	struct btrfs_block_group_cache *block_group, struct btrfs_path *path,
	int cow);
...

#endif

free-space-tree.c:

#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS

struct btrfs_free_space_info *btrfs_search_free_space_info(
	struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info,
	struct btrfs_block_group_cache *block_group, struct btrfs_path *path,
	int cow)
{
	return search_free_space_info(trans, fs_info, block_group, path, cow);
}

...

#endif

I hope it's a reasonable compromise among the options.

Re: [PATCH 5/6] btrfs: remove unused variable tree in end_compressed_bio_write()

[David Sterba]

On Wed, Nov 14, 2018 at 02:35:19PM +0100, Johannes Thumshirn wrote:
> Commit 2922040236f9 (btrfs: Remove extent_io_ops::writepage_end_io_hook)
> removed the indirection to extent_io_ops::writepage_end_io_hook but didn't
> remove the tree variable which then became unused.
> 
> Remove 'tree' as well to silence the warning when -Wunused-but-set-variable
> is used to compile btrfs.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  fs/btrfs/compression.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index bde8d0487bbb..088570c5dfb8 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -229,7 +229,6 @@ static noinline void end_compressed_writeback(struct inode *inode,
>   */
>  static void end_compressed_bio_write(struct bio *bio)
>  {
> -	struct extent_io_tree *tree;
>  	struct compressed_bio *cb = bio->bi_private;
>  	struct inode *inode;
>  	struct page *page;
> @@ -248,7 +247,6 @@ static void end_compressed_bio_write(struct bio *bio)
>  	 * call back into the FS and do all the end_io operations
>  	 */
>  	inode = cb->inode;
> -	tree = &BTRFS_I(inode)->io_tree;

The fix has been folded to the original patch already, same the other
one removing 'tree'. If you used linux-next to run the build, then it's
probably lagging behind the development branches that are synced to
kernel.org after some testing. The devel repos are either
git://github.com/kdave/btrfs-devel or
git://gitlab.com/kdave/btrfs-devel, branch misc-next.

Re: [PATCH 6/6] btrfs: unconditionally provide function prototypes from free-space-tree.h

[Nikolay Borisov]

On 14.11.18 г. 21:53 ч., David Sterba wrote:
> On Wed, Nov 14, 2018 at 02:54:37PM +0100, Johannes Thumshirn wrote:
>> On 14/11/2018 14:52, Nikolay Borisov wrote:
>>> I agree with this patch, however you go into the gray area of
>>> "everything which is exported should have btrfs_ prefix". It's up to
>>> David to see if he is content with this change.
>>
>> Yep you're right. I think I should change it, but I'll wait for David's
>> response first.
> 
> There's one prior example of the conditionally exported functions:
> btrfs_find_lock_delalloc_range . This function declaration and
> definition are under the ifdef and it's a simple wrapper around a static
> function find_lock_delalloc_range.

I have a patch which actually simplifies this somewhat because, frankly I find it stupid and redundant to do this frickin' dance for functions which are used only in debugging. Here is the diff (still not submitted since it's waiting for some other cleanups to be merged): 

--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1556,10 +1556,13 @@ 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,
-                                   struct extent_io_tree *tree,
-                                   struct page *locked_page, u64 *start,
-                                   u64 *end)
+#ifndef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
+static
+#endif
+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 = BTRFS_MAX_EXTENT_SIZE;
        u64 delalloc_start;
@@ -1637,16 +1640,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)
-{
-       return find_lock_delalloc_range(inode, tree, locked_page, start, end);
-}
-#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 30bfeefa2d89..12d300fae7a0 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -522,10 +522,8 @@ 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 find_lock_delalloc_range(struct inode *inode, struct extent_io_tree *tree,
+                            struct page *locked_page, u64 *start, u64 *end);
 #endif
 struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,


> 
> I'd do the same for the functions in your list, where applies. A static
> function can be better optimized and I don't want to make it harder for
> the compiler just to have it exported for the tests. They're not enabled
> by default and never in production builds.
> 
> So:
> 
> free-space-tree.h:
> 
> #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> 
> struct btrfs_free_space_info *btrfs_search_free_space_info(
> 	struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info,
> 	struct btrfs_block_group_cache *block_group, struct btrfs_path *path,
> 	int cow);
> ...
> 
> #endif
> 
> free-space-tree.c:
> 
> #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> 
> struct btrfs_free_space_info *btrfs_search_free_space_info(
> 	struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info,
> 	struct btrfs_block_group_cache *block_group, struct btrfs_path *path,
> 	int cow)
> {
> 	return search_free_space_info(trans, fs_info, block_group, path, cow);
> }
> 
> ...
> 
> #endif
> 
> I hope it's a reasonable compromise among the options.
> 

Re: [PATCH 6/6] btrfs: unconditionally provide function prototypes from free-space-tree.h

[David Sterba]

On Wed, Nov 14, 2018 at 11:05:16PM +0200, Nikolay Borisov wrote:
> 
> 
> On 14.11.18 г. 21:53 ч., David Sterba wrote:
> > On Wed, Nov 14, 2018 at 02:54:37PM +0100, Johannes Thumshirn wrote:
> >> On 14/11/2018 14:52, Nikolay Borisov wrote:
> >>> I agree with this patch, however you go into the gray area of
> >>> "everything which is exported should have btrfs_ prefix". It's up to
> >>> David to see if he is content with this change.
> >>
> >> Yep you're right. I think I should change it, but I'll wait for David's
> >> response first.
> > 
> > There's one prior example of the conditionally exported functions:
> > btrfs_find_lock_delalloc_range . This function declaration and
> > definition are under the ifdef and it's a simple wrapper around a static
> > function find_lock_delalloc_range.
> 
> I have a patch which actually simplifies this somewhat because,
> frankly I find it stupid and redundant to do this frickin' dance for
> functions which are used only in debugging. Here is the diff (still
> not submitted since it's waiting for some other cleanups to be
> merged): 
> 
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1556,10 +1556,13 @@ 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,
> -                                   struct extent_io_tree *tree,
> -                                   struct page *locked_page, u64 *start,
> -                                   u64 *end)
> +#ifndef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> +static
> +#endif
> +noinline_for_stack u64 find_lock_delalloc_range(struct inode *inode,
> +                                               struct extent_io_tree *tree,
> +                                               struct page *locked_page,
> +                                               u64 *start, u64 *end)

If there's nostatic_for_tests or export_for_tests (possibly all caps),
then ok.

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1962 bytes --]



On 2018/11/14 下午9:35, 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.
> 
> 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.

Looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

BTW, could we just make "-Wunused-but-set-variable" default for btrfs
module instead of following the global setting?

Thanks,
Qu


> 
> Johannes Thumshirn (6):
>   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: remove unused variable tree in bio_readpage_error()
>   btrfs: remove unused variable tree in end_compressed_bio_write()
>   btrfs: unconditionally provide function prototypes from
>     free-space-tree.h
> 
>  fs/btrfs/compression.c     |  2 --
>  fs/btrfs/extent_io.c       |  3 ---
>  fs/btrfs/free-space-tree.h |  2 --
>  fs/btrfs/inode.c           | 16 ++++++----------
>  fs/btrfs/sysfs.c           | 33 ---------------------------------
>  fs/btrfs/sysfs.h           |  2 --
>  6 files changed, 6 insertions(+), 52 deletions(-)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/6] btrfs: remove unused variable tree in end_compressed_bio_write()

[Johannes Thumshirn]

On 14/11/2018 22:04, David Sterba wrote:
> The fix has been folded to the original patch already, same the other
> one removing 'tree'. If you used linux-next to run the build, then it's
> probably lagging behind the development branches that are synced to
> kernel.org after some testing. The devel repos are either
> git://github.com/kdave/btrfs-devel or
> git://gitlab.com/kdave/btrfs-devel, branch misc-next.

Ah OK, I've used your kernel.org btrfs/for-next, I'll rebase for v2.

-- 
Johannes Thumshirn                                        SUSE Labs
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

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

[David Sterba]

On Thu, Nov 15, 2018 at 09:30:42AM +0800, Qu Wenruo wrote:
> BTW, could we just make "-Wunused-but-set-variable" default for btrfs
> module instead of following the global setting?

We'd need to fix all the warnings first, there are more left. We could
make it on by default for the debugging build though. I've checked that
the the macro from scripts/Makefile.extrawarn to detect the options is
available when building the module so, something like

ifeq(CONFIG_BTRFS_DEBUG,y)

warnings+=$(call cc-option,-Wunused-but-set-variable)
...

ccflags-y+=$(warnings)

endif

may be a good start.