[PATCH 1/2] btrfs: use min_t() for mismatched type comparison

[PATCH 1/2] btrfs: use min_t() for mismatched type comparison

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

loff_t is a signed type, so using min() to compare it against a u64
causes a compiler warning:

fs/btrfs/extent_io.c:2497:13: error: call to '__compiletime_assert_728' declared with 'error' attribute: min(folio_pos(folio) + folio_size(folio) - 1, end) signedness error
 2497 |                 cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end);
      |                           ^

Use min_t() instead.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202502211908.aCcQQyEY-lkp@intel.com/
Fixes: aba063bf9336 ("btrfs: prepare extent_io.c for future larger folio support")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/btrfs/extent_io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index f0a1da40d641..7dc996e7e249 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2485,7 +2485,7 @@ void extent_write_locked_range(struct inode *inode, const struct folio *locked_f
 		 * code is just in case, but shouldn't actually be run.
 		 */
 		if (IS_ERR(folio)) {
-			cur_end = min(round_down(cur, PAGE_SIZE) + PAGE_SIZE - 1, end);
+			cur_end = min_t(u64, round_down(cur, PAGE_SIZE) + PAGE_SIZE - 1, end);
 			cur_len = cur_end + 1 - cur;
 			btrfs_mark_ordered_io_finished(BTRFS_I(inode), NULL,
 						       cur, cur_len, false);
@@ -2494,7 +2494,7 @@ void extent_write_locked_range(struct inode *inode, const struct folio *locked_f
 			continue;
 		}
 
-		cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end);
+		cur_end = min_t(u64, folio_pos(folio) + folio_size(folio) - 1, end);
 		cur_len = cur_end + 1 - cur;
 
 		ASSERT(folio_test_locked(folio));
-- 
2.39.5

[PATCH 2/2] btrfs: replace 64-bit division with a shift

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

folio_size() is not a compile-time constant, so in some configurations,
the DIV_ROUND_UP() turns into a 64-bit division that is not allowed
on 32-bit architectures.

x86_64-linux-ld: fs/btrfs/extent_io.o: in function `writepage_delalloc':
extent_io.c:(.text+0x2b6e): undefined reference to `__udivdi3'

This is probably not the correct solution, but it should illustrate
the issue. A trivial fix would be DIV64_U64_ROUND_UP(), which of course
is very expensive.  Maybe there should be a DIV_ROUND_UP_FOLIO macro?

Fixes: aba063bf9336 ("btrfs: prepare extent_io.c for future larger folio support")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/btrfs/extent_io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 7dc996e7e249..e4ba4fa3f48c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1505,8 +1505,7 @@ static noinline_for_stack int writepage_delalloc(struct btrfs_inode *inode,
 	 * delalloc_end is already one less than the total length, so
 	 * we don't subtract one from folio_size().
 	 */
-	delalloc_to_write +=
-		DIV_ROUND_UP(delalloc_end + 1 - page_start, folio_size(folio));
+	delalloc_to_write += (delalloc_end + 1 - page_start + folio_size(folio) - 1) >> folio_order(folio);
 
 	/*
 	 * If all ranges are submitted asynchronously, we just need to account
-- 
2.39.5

Re: [PATCH 1/2] btrfs: use min_t() for mismatched type comparison

[Qu Wenruo]

在 2025/2/26 06:14, Arnd Bergmann 写道:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> loff_t is a signed type, so using min() to compare it against a u64
> causes a compiler warning:
> 
> fs/btrfs/extent_io.c:2497:13: error: call to '__compiletime_assert_728' declared with 'error' attribute: min(folio_pos(folio) + folio_size(folio) - 1, end) signedness error
>   2497 |                 cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end);
>        |                           ^
> 
> Use min_t() instead.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202502211908.aCcQQyEY-lkp@intel.com/
> Fixes: aba063bf9336 ("btrfs: prepare extent_io.c for future larger folio support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks a lot, those fixes will be merged into the next version.

For now the series is only for test purposes as there is a backlog of 
subpage block size related patches pending.

Thanks,
Qu

> ---
>   fs/btrfs/extent_io.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index f0a1da40d641..7dc996e7e249 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2485,7 +2485,7 @@ void extent_write_locked_range(struct inode *inode, const struct folio *locked_f
>   		 * code is just in case, but shouldn't actually be run.
>   		 */
>   		if (IS_ERR(folio)) {
> -			cur_end = min(round_down(cur, PAGE_SIZE) + PAGE_SIZE - 1, end);
> +			cur_end = min_t(u64, round_down(cur, PAGE_SIZE) + PAGE_SIZE - 1, end);
>   			cur_len = cur_end + 1 - cur;
>   			btrfs_mark_ordered_io_finished(BTRFS_I(inode), NULL,
>   						       cur, cur_len, false);
> @@ -2494,7 +2494,7 @@ void extent_write_locked_range(struct inode *inode, const struct folio *locked_f
>   			continue;
>   		}
>   
> -		cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end);
> +		cur_end = min_t(u64, folio_pos(folio) + folio_size(folio) - 1, end);
>   		cur_len = cur_end + 1 - cur;
>   
>   		ASSERT(folio_test_locked(folio));

Re: [PATCH 1/2] btrfs: use min_t() for mismatched type comparison

[Arnd Bergmann]

On Tue, Feb 25, 2025, at 22:22, Qu Wenruo wrote:
> 在 2025/2/26 06:14, Arnd Bergmann 写道:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> loff_t is a signed type, so using min() to compare it against a u64
>> causes a compiler warning:
>> 
>> fs/btrfs/extent_io.c:2497:13: error: call to '__compiletime_assert_728' declared with 'error' attribute: min(folio_pos(folio) + folio_size(folio) - 1, end) signedness error
>>   2497 |                 cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end);
>>        |                           ^
>> 
>> Use min_t() instead.
>> 
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202502211908.aCcQQyEY-lkp@intel.com/
>> Fixes: aba063bf9336 ("btrfs: prepare extent_io.c for future larger folio support")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Thanks a lot, those fixes will be merged into the next version.
>
> For now the series is only for test purposes as there is a backlog of 
> subpage block size related patches pending.

Ok, thanks! Please double-check that the calculation in
patch 2/2 is actually correct though, as I wasn't entirely
sure about that part. 

     Arnd

Re: [PATCH 1/2] btrfs: use min_t() for mismatched type comparison

[David Laight]

On Tue, 25 Feb 2025 20:44:10 +0100
Arnd Bergmann <arnd@kernel.org> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> loff_t is a signed type, so using min() to compare it against a u64
> causes a compiler warning:
> 
> fs/btrfs/extent_io.c:2497:13: error: call to '__compiletime_assert_728' declared with 'error' attribute: min(folio_pos(folio) + folio_size(folio) - 1, end) signedness error
>  2497 |                 cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end);

Isn't the actual problem that folio_pos() has the wrong return type.
I can't remember what loff_t is supposed to be for, but here you want
something that reduces to 'unsigned long'.

> Use min_t() instead.

If a signed variable is known to contain a non-negative value then
min_unsigned() is better.
In particular it will never discard upper bits.

Enough min_t() cause bugs (usually due to high bits being discarded when the
type of the destination (eg u8) is used) that is is tempting to start a 'duck shoot'
season against them.

> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202502211908.aCcQQyEY-lkp@intel.com/
> Fixes: aba063bf9336 ("btrfs: prepare extent_io.c for future larger folio support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  fs/btrfs/extent_io.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index f0a1da40d641..7dc996e7e249 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2485,7 +2485,7 @@ void extent_write_locked_range(struct inode *inode, const struct folio *locked_f
>  		 * code is just in case, but shouldn't actually be run.
>  		 */
>  		if (IS_ERR(folio)) {
> -			cur_end = min(round_down(cur, PAGE_SIZE) + PAGE_SIZE - 1, end);
> +			cur_end = min_t(u64, round_down(cur, PAGE_SIZE) + PAGE_SIZE - 1, end);

That one is fine and doesn't need changing.

>  			cur_len = cur_end + 1 - cur;
>  			btrfs_mark_ordered_io_finished(BTRFS_I(inode), NULL,
>  						       cur, cur_len, false);
> @@ -2494,7 +2494,7 @@ void extent_write_locked_range(struct inode *inode, const struct folio *locked_f
>  			continue;
>  		}
>  
> -		cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end);
> +		cur_end = min_t(u64, folio_pos(folio) + folio_size(folio) - 1, end);

A subtle alternative to min_unsigned() is to change the 1 to 1ull.

	David

>  		cur_len = cur_end + 1 - cur;
>  
>  		ASSERT(folio_test_locked(folio));