* [PATCH 2/2] btrfs: replace 64-bit division with a shift
2025-02-25 19:44 [PATCH 1/2] btrfs: use min_t() for mismatched type comparison Arnd Bergmann
@ 2025-02-25 19:44 ` Arnd Bergmann
2025-02-25 21:22 ` [PATCH 1/2] btrfs: use min_t() for mismatched type comparison Qu Wenruo
2025-02-26 14:13 ` David Laight
2 siblings, 0 replies; 5+ messages in thread
From: Arnd Bergmann @ 2025-02-25 19:44 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba, Qu Wenruo
Cc: Arnd Bergmann, Johannes Thumshirn, Anand Jain, Filipe Manana,
Li Zetao, linux-btrfs, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] btrfs: use min_t() for mismatched type comparison
2025-02-25 19:44 [PATCH 1/2] btrfs: use min_t() for mismatched type comparison Arnd Bergmann
2025-02-25 19:44 ` [PATCH 2/2] btrfs: replace 64-bit division with a shift Arnd Bergmann
@ 2025-02-25 21:22 ` Qu Wenruo
2025-02-25 21:28 ` Arnd Bergmann
2025-02-26 14:13 ` David Laight
2 siblings, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2025-02-25 21:22 UTC (permalink / raw)
To: Arnd Bergmann, Chris Mason, Josef Bacik, David Sterba
Cc: Arnd Bergmann, kernel test robot, Johannes Thumshirn, Anand Jain,
Filipe Manana, Li Zetao, linux-btrfs, linux-kernel
在 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));
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] btrfs: use min_t() for mismatched type comparison
2025-02-25 21:22 ` [PATCH 1/2] btrfs: use min_t() for mismatched type comparison Qu Wenruo
@ 2025-02-25 21:28 ` Arnd Bergmann
0 siblings, 0 replies; 5+ messages in thread
From: Arnd Bergmann @ 2025-02-25 21:28 UTC (permalink / raw)
To: Qu Wenruo, Arnd Bergmann, Chris Mason, Josef Bacik, David Sterba
Cc: kernel test robot, Johannes Thumshirn, Anand Jain, Filipe Manana,
Li Zetao, linux-btrfs, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] btrfs: use min_t() for mismatched type comparison
2025-02-25 19:44 [PATCH 1/2] btrfs: use min_t() for mismatched type comparison Arnd Bergmann
2025-02-25 19:44 ` [PATCH 2/2] btrfs: replace 64-bit division with a shift Arnd Bergmann
2025-02-25 21:22 ` [PATCH 1/2] btrfs: use min_t() for mismatched type comparison Qu Wenruo
@ 2025-02-26 14:13 ` David Laight
2 siblings, 0 replies; 5+ messages in thread
From: David Laight @ 2025-02-26 14:13 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Chris Mason, Josef Bacik, David Sterba, Qu Wenruo, Arnd Bergmann,
kernel test robot, Johannes Thumshirn, Anand Jain, Filipe Manana,
Li Zetao, linux-btrfs, linux-kernel
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));
^ permalink raw reply [flat|nested] 5+ messages in thread