2021-10-15 19:36:26

by Jiabing Wan

Subject: [PATCH] btrfs: Simplify conditional in assert

Fix following coccicheck warning:
./fs/btrfs/inode.c:2015:16-18: WARNING !A || A && B is equivalent to !A || B

Signed-off-by: Wan Jiabing <[email protected]>
---
fs/btrfs/inode.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e9154b436c47..da4aeef73b0d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2011,8 +2011,7 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
* to use run_delalloc_nocow() here, like for regular
* preallocated inodes.
*/
- ASSERT(!zoned ||
- (zoned && btrfs_is_data_reloc_root(inode->root)));
+ ASSERT(!zoned || btrfs_is_data_reloc_root(inode->root));
ret = run_delalloc_nocow(inode, locked_page, start, end,
page_started, nr_written);
} else if (!inode_can_compress(inode) ||
--
2.20.1

2021-10-15 19:40:46

by David Sterba

Subject: Re: [PATCH] btrfs: Simplify conditional in assert

Adding Johannes to CC,

On Fri, Oct 15, 2021 at 06:36:39AM -0400, Wan Jiabing wrote:
> Fix following coccicheck warning:
> ./fs/btrfs/inode.c:2015:16-18: WARNING !A || A && B is equivalent to !A || B
>
> Signed-off-by: Wan Jiabing <[email protected]>
> ---
> fs/btrfs/inode.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index e9154b436c47..da4aeef73b0d 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2011,8 +2011,7 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
> * to use run_delalloc_nocow() here, like for regular
> * preallocated inodes.
> */
> - ASSERT(!zoned ||
> - (zoned && btrfs_is_data_reloc_root(inode->root)));
> + ASSERT(!zoned || btrfs_is_data_reloc_root(inode->root));

The short form is equivalent, but I'm not sure it's also on the same
level of readability. Repeating the 'zoned' condition check makes it
obvious on first sight, which is what I'd prefer.

Johannes if you'd like the new version I'll change it but otherwise I'm
fine with what we have now.

> ret = run_delalloc_nocow(inode, locked_page, start, end,
> page_started, nr_written);
> } else if (!inode_can_compress(inode) ||
> --
> 2.20.1

2021-10-15 21:30:12

by Johannes Thumshirn

Subject: Re: [PATCH] btrfs: Simplify conditional in assert

On 15/10/2021 12:52, David Sterba wrote:
> Adding Johannes to CC,
>
> On Fri, Oct 15, 2021 at 06:36:39AM -0400, Wan Jiabing wrote:
>> Fix following coccicheck warning:
>> ./fs/btrfs/inode.c:2015:16-18: WARNING !A || A && B is equivalent to !A || B
>>
>> Signed-off-by: Wan Jiabing <[email protected]>
>> ---
>> fs/btrfs/inode.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index e9154b436c47..da4aeef73b0d 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -2011,8 +2011,7 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
>> * to use run_delalloc_nocow() here, like for regular
>> * preallocated inodes.
>> */
>> - ASSERT(!zoned ||
>> - (zoned && btrfs_is_data_reloc_root(inode->root)));
>> + ASSERT(!zoned || btrfs_is_data_reloc_root(inode->root));
>
> The short form is equivalent, but I'm not sure it's also on the same
> level of readability. Repeating the 'zoned' condition check makes it
> obvious on first sight, which is what I'd prefer.
>
> Johannes if you'd like the new version I'll change it but otherwise I'm
> fine with what we have now.

I'm fine either way, no strong preferences from my side.

2021-10-16 11:22:49

by Nikolay Borisov

Subject: Re: [PATCH] btrfs: Simplify conditional in assert

On 15.10.21 г. 13:51, David Sterba wrote:
> Adding Johannes to CC,
>
> On Fri, Oct 15, 2021 at 06:36:39AM -0400, Wan Jiabing wrote:
>> Fix following coccicheck warning:
>> ./fs/btrfs/inode.c:2015:16-18: WARNING !A || A && B is equivalent to !A || B
>>
>> Signed-off-by: Wan Jiabing <[email protected]>
>> ---
>> fs/btrfs/inode.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index e9154b436c47..da4aeef73b0d 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -2011,8 +2011,7 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
>> * to use run_delalloc_nocow() here, like for regular
>> * preallocated inodes.
>> */
>> - ASSERT(!zoned ||
>> - (zoned && btrfs_is_data_reloc_root(inode->root)));
>> + ASSERT(!zoned || btrfs_is_data_reloc_root(inode->root));
>
> The short form is equivalent, but I'm not sure it's also on the same
> level of readability. Repeating the 'zoned' condition check makes it
> obvious on first sight, which is what I'd prefer.
>
> Johannes if you'd like the new version I'll change it but otherwise I'm
> fine with what we have now.

Just my 2 cents:

The less code we have the better, i.e !zoned is obvious when it's true
i.e when zoned is false. So the way I read teh assert with the short
form is "we are not zoned OR we are (this is implicit) and this is the
data reloc root". Obviously this is personal preference as you deem it's
better better to have the !zoned || zoned.


>
>> ret = run_delalloc_nocow(inode, locked_page, start, end,
>> page_started, nr_written);
>> } else if (!inode_can_compress(inode) ||
>> --
>> 2.20.1
>