diff mbox series

[2/2] Fix several style errors in fs/btrfs/inode.c

Message ID PAXP193MB20897179089B101CFFFB4B7CA7F19@PAXP193MB2089.EURP193.PROD.OUTLOOK.COM (mailing list archive)
State New, archived
Headers show
Series Check the return value of unpin_exten_cache. Cleanup style. | expand

Commit Message

Siddhartha Menon Dec. 31, 2022, 6:47 p.m. UTC 
Signed-off-by: Siddhartha Menon <siddharthamenon@outlook.com>
---
 fs/btrfs/inode.c | 49 +++++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

Comments

Qu Wenruo Jan. 1, 2023, 12:11 a.m. UTC | #1 
On 2023/1/1 02:47, Siddhartha Menon wrote:
> Signed-off-by: Siddhartha Menon <siddharthamenon@outlook.com>

Even for pure style cleanup, you need a commit message.

Not to mention this is not a simple style cleanup.

There are several big problems affecting some core components, did you 
ever test the patches?

> ---
>   fs/btrfs/inode.c | 49 +++++++++++++++++++++++++-----------------------
>   1 file changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index cb95d47e4d02..ee7ca0e69aa1 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -366,6 +366,7 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans,
>   	if (compress_type != BTRFS_COMPRESS_NONE) {
>   		struct page *cpage;
>   		int i = 0;
> +

You can submit a patch to address all such missing newline between 
declaration and code.

IIRC there used to be a warning for this.

>   		while (compressed_size > 0) {
>   			cpage = compressed_pages[i];
>   			cur_size = min_t(unsigned long, compressed_size,
> @@ -1221,7 +1222,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   	u64 blocksize = fs_info->sectorsize;
>   	struct btrfs_key ins;
>   	struct extent_map *em;
> -	unsigned clear_bits;
> +	unsigned int clear_bits;
>   	unsigned long page_ops;
>   	bool extent_reserved = false;
>   	int ret = 0;
> @@ -1557,7 +1558,7 @@ static int cow_file_range_async(struct btrfs_inode *inode,
>   	u64 num_chunks = DIV_ROUND_UP(end - start, SZ_512K);
>   	int i;
>   	bool should_compress;
> -	unsigned nofs_flag;
> +	unsigned int nofs_flag;

For unsigned -> unsigned int, it's also fine to do it in a separate 
patch, not mixing with other different changes.

>   	const blk_opf_t write_flags = wbc_to_write_flags(wbc);
>   
>   	unlock_extent(&inode->io_tree, start, end, NULL);
> @@ -1575,7 +1576,7 @@ static int cow_file_range_async(struct btrfs_inode *inode,
>   	memalloc_nofs_restore(nofs_flag);
>   
>   	if (!ctx) {
> -		unsigned clear_bits = EXTENT_LOCKED | EXTENT_DELALLOC |
> +		unsigned int clear_bits = EXTENT_LOCKED | EXTENT_DELALLOC |
>   			EXTENT_DELALLOC_NEW | EXTENT_DEFRAG |
>   			EXTENT_DO_ACCOUNTING;
>   		unsigned long page_ops = PAGE_UNLOCK | PAGE_START_WRITEBACK |
> @@ -3846,7 +3847,7 @@ int btrfs_orphan_cleanup(struct btrfs_root *root)
>   				ret = PTR_ERR(trans);
>   				goto out;
>   			}
> -			btrfs_debug(fs_info, "auto deleting %Lu",
> +			btrfs_debug(fs_info, "auto deleting %llu",
>   				    found_key.objectid);
>   			ret = btrfs_del_orphan_item(trans, root,
>   						    found_key.objectid);
> @@ -3892,8 +3893,8 @@ static noinline int acls_after_inode_item(struct extent_buffer *leaf,
>   {
>   	u32 nritems = btrfs_header_nritems(leaf);
>   	struct btrfs_key found_key;
> -	static u64 xattr_access = 0;
> -	static u64 xattr_default = 0;
> +	static u64 xattr_access;
> +	static u64 xattr_default;

Nope, xattr_access is immediately read.
If you removed the initialization, we got garbage.


In fact, you should initialize them using the same lines at the if 
(!xattr_access) branch.


>   	int scanned = 0;
>   
>   	if (!xattr_access) {
> @@ -4920,7 +4921,7 @@ int btrfs_truncate_block(struct btrfs_inode *inode, loff_t from, loff_t len,
>   	bool only_release_metadata = false;
>   	u32 blocksize = fs_info->sectorsize;
>   	pgoff_t index = from >> PAGE_SHIFT;
> -	unsigned offset = from & (blocksize - 1);
> +	unsigned int offset = from & (blocksize - 1);
>   	struct page *page;
>   	gfp_t mask = btrfs_alloc_write_mask(mapping);
>   	size_t write_bytes = blocksize;
> @@ -5358,7 +5359,7 @@ static void evict_inode_truncate_pages(struct inode *inode)
>   		struct extent_state *cached_state = NULL;
>   		u64 start;
>   		u64 end;
> -		unsigned state_flags;
> +		unsigned int state_flags;
>   
>   		node = rb_first(&io_tree->state);
>   		state = rb_entry(node, struct extent_state, rb_node);
> @@ -5842,7 +5843,7 @@ static struct inode *new_simple_dir(struct super_block *s,
>   	inode->i_op = &simple_dir_inode_operations;
>   	inode->i_opflags &= ~IOP_XATTR;
>   	inode->i_fop = &simple_dir_operations;
> -	inode->i_mode = S_IFDIR | S_IRUGO | S_IWUSR | S_IXUGO;
> +	inode->i_mode = 0755;

Big NO. You completely ignored the file type.

>   	inode->i_mtime = current_time(inode);
>   	inode->i_atime = inode->i_mtime;
>   	inode->i_ctime = inode->i_mtime;
> @@ -5983,7 +5984,7 @@ static int btrfs_opendir(struct inode *inode, struct file *file)
>   struct dir_entry {
>   	u64 ino;
>   	u64 offset;
> -	unsigned type;
> +	unsigned int type;
>   	int name_len;
>   };
>   
> @@ -6667,9 +6668,11 @@ int btrfs_add_link(struct btrfs_trans_handle *trans,
>   	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);
> +

I don't think the newline is ever needed.

>   		if (err)
>   			btrfs_abort_transaction(trans, err);
>   	} else if (add_backref) {
> @@ -8930,20 +8933,20 @@ void btrfs_destroy_inode(struct inode *vfs_inode)
>   
>   	while (1) {
>   		ordered = btrfs_lookup_first_ordered_extent(inode, (u64)-1);
> +
>   		if (!ordered)
>   			break;
> -		else {
> -			btrfs_err(root->fs_info,
> -				  "found ordered extent %llu %llu on inode cleanup",
> -				  ordered->file_offset, ordered->num_bytes);
>   
> -			if (!freespace_inode)
> -				btrfs_lockdep_acquire(root->fs_info, btrfs_ordered_extent);
> +		btrfs_err(root->fs_info,

The error message is only supposed to be triggered at if (!ordered) branch.

But now it's unconditionally.

> +			  "found ordered extent %llu %llu on inode cleanup",
> +			  ordered->file_offset, ordered->num_bytes);
>   
> -			btrfs_remove_ordered_extent(inode, ordered);
> -			btrfs_put_ordered_extent(ordered);
> -			btrfs_put_ordered_extent(ordered);
> -		}
> +		if (!freespace_inode)
> +			btrfs_lockdep_acquire(root->fs_info, btrfs_ordered_extent);
> +
> +		btrfs_remove_ordered_extent(inode, ordered);
> +		btrfs_put_ordered_extent(ordered);
> +		btrfs_put_ordered_extent(ordered);
>   	}
>   	btrfs_qgroup_check_reserved_leak(inode);
>   	inode_tree_del(inode);
> @@ -9357,10 +9360,10 @@ static int btrfs_rename(struct user_namespace *mnt_userns,
>   	if (ret) {
>   		if (ret == -EEXIST) {
>   			/* we shouldn't get

If you're changing the style, please also change the comment to follow 
the common standard.

Thanks,
Qu
> -			 * eexist without a new_inode */
> -			if (WARN_ON(!new_inode)) {
> +			 * eexist without a new_inode
> +			 */
> +			if (WARN_ON(!new_inode))
>   				goto out_fscrypt_names;
> -			}
>   		} else {
>   			/* maybe -EOVERFLOW */
>   			goto out_fscrypt_names;
kernel test robot Jan. 15, 2023, 1:56 p.m. UTC | #2 
Greeting,

FYI, we noticed xfstests.btrfs.047.fail due to commit (built with gcc-11):

commit: 180afc95bcaa00d7ad52f61665a71cb059074fcc ("[PATCH 2/2] Fix several style errors in fs/btrfs/inode.c")
url: https://github.com/intel-lab-lkp/linux/commits/Siddhartha-Menon/Fix-several-style-errors-in-fs-btrfs-inode-c/20230101-024842
base: https://git.kernel.org/cgit/linux/kernel/git/kdave/linux.git for-next
patch link: https://lore.kernel.org/all/PAXP193MB20897179089B101CFFFB4B7CA7F19@PAXP193MB2089.EURP193.PROD.OUTLOOK.COM/
patch subject: [PATCH 2/2] Fix several style errors in fs/btrfs/inode.c

in testcase: xfstests
version: xfstests-x86_64-fb6575e-1_20230102
with following parameters:

	disk: 6HDD
	fs: btrfs
	test: btrfs-group-04

test-description: xfstests is a regression test suite for xfs and other files ystems.
test-url: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git


on test machine: 8 threads 1 sockets Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz (Haswell) with 8G memory

caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):




If you fix the issue, kindly add following tag
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Link: https://lore.kernel.org/oe-lkp/202301152153.51148136-oliver.sang@intel.com

2023-01-15 00:20:27 export TEST_DIR=/fs/sda1
2023-01-15 00:20:27 export TEST_DEV=/dev/sda1
2023-01-15 00:20:27 export FSTYP=btrfs
2023-01-15 00:20:27 export SCRATCH_MNT=/fs/scratch
2023-01-15 00:20:27 mkdir /fs/scratch -p
2023-01-15 00:20:27 export SCRATCH_DEV_POOL="/dev/sda2 /dev/sda3 /dev/sda4 /dev/sda5 /dev/sda6"
2023-01-15 00:20:27 sed "s:^:btrfs/:" //lkp/benchmarks/xfstests/tests/btrfs-group-04
2023-01-15 00:20:27 ./check btrfs/040 btrfs/041 btrfs/042 btrfs/043 btrfs/044 btrfs/045 btrfs/046 btrfs/047 btrfs/048 btrfs/049
FSTYP         -- btrfs
PLATFORM      -- Linux/x86_64 lkp-hsw-d01 6.1.0-rc8-00284-g180afc95bcaa #1 SMP Sun Jan 15 00:39:58 CST 2023
MKFS_OPTIONS  -- /dev/sda2
MOUNT_OPTIONS -- /dev/sda2 /fs/scratch

btrfs/040        2s
btrfs/041        3s
btrfs/042        4s
btrfs/043        2s
btrfs/044        2s
btrfs/045        3s
btrfs/046        8s
btrfs/047       - output mismatch (see /lkp/benchmarks/xfstests/results//btrfs/047.out.bad)
    --- tests/btrfs/047.out	2023-01-02 16:54:41.000000000 +0000
    +++ /lkp/benchmarks/xfstests/results//btrfs/047.out.bad	2023-01-15 00:20:53.456443398 +0000
    @@ -1,2 +1,2 @@
     QA output created by 047
    -setfattr: SCRATCH_MNT/snapshot/child: Operation not supported
    +setfattr: SCRATCH_MNT/snapshot/child: Operation not permitted
    ...
    (Run 'diff -u /lkp/benchmarks/xfstests/tests/btrfs/047.out /lkp/benchmarks/xfstests/results//btrfs/047.out.bad'  to see the entire diff)
btrfs/048        4s
btrfs/049       [failed, exit status 1]- output mismatch (see /lkp/benchmarks/xfstests/results//btrfs/049.out.bad)
    --- tests/btrfs/049.out	2023-01-02 16:54:41.000000000 +0000
    +++ /lkp/benchmarks/xfstests/results//btrfs/049.out.bad	2023-01-15 00:21:00.341443701 +0000
    @@ -1,2 +1,3 @@
     QA output created by 049
    -Silence is golden
    +Successfully replaced device
    +(see /lkp/benchmarks/xfstests/results//btrfs/049.full for details)
    ...
    (Run 'diff -u /lkp/benchmarks/xfstests/tests/btrfs/049.out /lkp/benchmarks/xfstests/results//btrfs/049.out.bad'  to see the entire diff)
Ran: btrfs/040 btrfs/041 btrfs/042 btrfs/043 btrfs/044 btrfs/045 btrfs/046 btrfs/047 btrfs/048 btrfs/049
Failures: btrfs/047 btrfs/049
Failed 2 of 10 tests


(please be noted the btrfs/049 also failed on parent)


To reproduce:

        git clone https://github.com/intel/lkp-tests.git
        cd lkp-tests
        sudo bin/lkp install job.yaml           # job file is attached in this email
        bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
        sudo bin/lkp run generated-yaml-file

        # if come across any failure that blocks the test,
        # please remove ~/.lkp and /lkp dir to run from a clean state.
diff mbox series

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index cb95d47e4d02..ee7ca0e69aa1 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -366,6 +366,7 @@  static int insert_inline_extent(struct btrfs_trans_handle *trans,
 	if (compress_type != BTRFS_COMPRESS_NONE) {
 		struct page *cpage;
 		int i = 0;
+
 		while (compressed_size > 0) {
 			cpage = compressed_pages[i];
 			cur_size = min_t(unsigned long, compressed_size,
@@ -1221,7 +1222,7 @@  static noinline int cow_file_range(struct btrfs_inode *inode,
 	u64 blocksize = fs_info->sectorsize;
 	struct btrfs_key ins;
 	struct extent_map *em;
-	unsigned clear_bits;
+	unsigned int clear_bits;
 	unsigned long page_ops;
 	bool extent_reserved = false;
 	int ret = 0;
@@ -1557,7 +1558,7 @@  static int cow_file_range_async(struct btrfs_inode *inode,
 	u64 num_chunks = DIV_ROUND_UP(end - start, SZ_512K);
 	int i;
 	bool should_compress;
-	unsigned nofs_flag;
+	unsigned int nofs_flag;
 	const blk_opf_t write_flags = wbc_to_write_flags(wbc);
 
 	unlock_extent(&inode->io_tree, start, end, NULL);
@@ -1575,7 +1576,7 @@  static int cow_file_range_async(struct btrfs_inode *inode,
 	memalloc_nofs_restore(nofs_flag);
 
 	if (!ctx) {
-		unsigned clear_bits = EXTENT_LOCKED | EXTENT_DELALLOC |
+		unsigned int clear_bits = EXTENT_LOCKED | EXTENT_DELALLOC |
 			EXTENT_DELALLOC_NEW | EXTENT_DEFRAG |
 			EXTENT_DO_ACCOUNTING;
 		unsigned long page_ops = PAGE_UNLOCK | PAGE_START_WRITEBACK |
@@ -3846,7 +3847,7 @@  int btrfs_orphan_cleanup(struct btrfs_root *root)
 				ret = PTR_ERR(trans);
 				goto out;
 			}
-			btrfs_debug(fs_info, "auto deleting %Lu",
+			btrfs_debug(fs_info, "auto deleting %llu",
 				    found_key.objectid);
 			ret = btrfs_del_orphan_item(trans, root,
 						    found_key.objectid);
@@ -3892,8 +3893,8 @@  static noinline int acls_after_inode_item(struct extent_buffer *leaf,
 {
 	u32 nritems = btrfs_header_nritems(leaf);
 	struct btrfs_key found_key;
-	static u64 xattr_access = 0;
-	static u64 xattr_default = 0;
+	static u64 xattr_access;
+	static u64 xattr_default;
 	int scanned = 0;
 
 	if (!xattr_access) {
@@ -4920,7 +4921,7 @@  int btrfs_truncate_block(struct btrfs_inode *inode, loff_t from, loff_t len,
 	bool only_release_metadata = false;
 	u32 blocksize = fs_info->sectorsize;
 	pgoff_t index = from >> PAGE_SHIFT;
-	unsigned offset = from & (blocksize - 1);
+	unsigned int offset = from & (blocksize - 1);
 	struct page *page;
 	gfp_t mask = btrfs_alloc_write_mask(mapping);
 	size_t write_bytes = blocksize;
@@ -5358,7 +5359,7 @@  static void evict_inode_truncate_pages(struct inode *inode)
 		struct extent_state *cached_state = NULL;
 		u64 start;
 		u64 end;
-		unsigned state_flags;
+		unsigned int state_flags;
 
 		node = rb_first(&io_tree->state);
 		state = rb_entry(node, struct extent_state, rb_node);
@@ -5842,7 +5843,7 @@  static struct inode *new_simple_dir(struct super_block *s,
 	inode->i_op = &simple_dir_inode_operations;
 	inode->i_opflags &= ~IOP_XATTR;
 	inode->i_fop = &simple_dir_operations;
-	inode->i_mode = S_IFDIR | S_IRUGO | S_IWUSR | S_IXUGO;
+	inode->i_mode = 0755;
 	inode->i_mtime = current_time(inode);
 	inode->i_atime = inode->i_mtime;
 	inode->i_ctime = inode->i_mtime;
@@ -5983,7 +5984,7 @@  static int btrfs_opendir(struct inode *inode, struct file *file)
 struct dir_entry {
 	u64 ino;
 	u64 offset;
-	unsigned type;
+	unsigned int type;
 	int name_len;
 };
 
@@ -6667,9 +6668,11 @@  int btrfs_add_link(struct btrfs_trans_handle *trans,
 	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);
+
 		if (err)
 			btrfs_abort_transaction(trans, err);
 	} else if (add_backref) {
@@ -8930,20 +8933,20 @@  void btrfs_destroy_inode(struct inode *vfs_inode)
 
 	while (1) {
 		ordered = btrfs_lookup_first_ordered_extent(inode, (u64)-1);
+
 		if (!ordered)
 			break;
-		else {
-			btrfs_err(root->fs_info,
-				  "found ordered extent %llu %llu on inode cleanup",
-				  ordered->file_offset, ordered->num_bytes);
 
-			if (!freespace_inode)
-				btrfs_lockdep_acquire(root->fs_info, btrfs_ordered_extent);
+		btrfs_err(root->fs_info,
+			  "found ordered extent %llu %llu on inode cleanup",
+			  ordered->file_offset, ordered->num_bytes);
 
-			btrfs_remove_ordered_extent(inode, ordered);
-			btrfs_put_ordered_extent(ordered);
-			btrfs_put_ordered_extent(ordered);
-		}
+		if (!freespace_inode)
+			btrfs_lockdep_acquire(root->fs_info, btrfs_ordered_extent);
+
+		btrfs_remove_ordered_extent(inode, ordered);
+		btrfs_put_ordered_extent(ordered);
+		btrfs_put_ordered_extent(ordered);
 	}
 	btrfs_qgroup_check_reserved_leak(inode);
 	inode_tree_del(inode);
@@ -9357,10 +9360,10 @@  static int btrfs_rename(struct user_namespace *mnt_userns,
 	if (ret) {
 		if (ret == -EEXIST) {
 			/* we shouldn't get
-			 * eexist without a new_inode */
-			if (WARN_ON(!new_inode)) {
+			 * eexist without a new_inode
+			 */
+			if (WARN_ON(!new_inode))
 				goto out_fscrypt_names;
-			}
 		} else {
 			/* maybe -EOVERFLOW */
 			goto out_fscrypt_names;