When compiling on a mips 64-bit machine we get these warnings:
In file included from ./arch/mips/include/asm/cacheflush.h:13,
from ./include/linux/cacheflush.h:5,
from ./include/linux/highmem.h:8,
from ./include/linux/bvec.h:10,
from ./include/linux/blk_types.h:10,
from ./include/linux/blkdev.h:9,
from fs/btrfs/disk-io.c:7:
fs/btrfs/disk-io.c: In function ‘csum_tree_block’:
fs/btrfs/disk-io.c:100:34: error: array subscript 1 is above array bounds of ‘struct page *[1]’ [-Werror=array-bounds]
100 | kaddr = page_address(buf->pages[i]);
| ~~~~~~~~~~^~~
./include/linux/mm.h:2135:48: note: in definition of macro ‘page_address’
2135 | #define page_address(page) lowmem_page_address(page)
| ^~~~
cc1: all warnings being treated as errors
We can check if i overflows to solve the problem. However, this doesn't make
much sense, since i == 1 and num_pages == 1 doesn't execute the body of the loop.
In addition, i < num_pages can also ensure that buf->pages[i] will not cross
the boundary. Unfortunately, this doesn't help with the problem observed here:
gcc still complains.
To fix this, start the loop at index 0 instead of 1. Also, a conditional was
added to skip the case where the index is 0, so that the loop iterations follow
the desired logic, and it makes all versions of gcc happy.
Signed-off-by: pengfuyuan <[email protected]>
---
fs/btrfs/disk-io.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index fbf9006c6234..8b05d556d747 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -96,9 +96,13 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
first_page_part - BTRFS_CSUM_SIZE);
- for (i = 1; i < num_pages; i++) {
- kaddr = page_address(buf->pages[i]);
- crypto_shash_update(shash, kaddr, PAGE_SIZE);
+ for (i = 0; i < num_pages; i++) {
+ struct page *p = buf->pages[i];
+
+ if (i != 0) {
+ kaddr = page_address(p);
+ crypto_shash_update(shash, kaddr, PAGE_SIZE);
+ }
}
memset(result, 0, BTRFS_CSUM_SIZE);
crypto_shash_final(shash, result);
--
2.25.1
On Tue, May 23, 2023 at 03:33:22PM +0800, Qu Wenruo wrote:
>
>
> On 2023/5/23 15:09, pengfuyuan wrote:
> >
> > When compiling on a mips 64-bit machine we get these warnings:
> >
> > In file included from ./arch/mips/include/asm/cacheflush.h:13,
> > from ./include/linux/cacheflush.h:5,
> > from ./include/linux/highmem.h:8,
> > from ./include/linux/bvec.h:10,
> > from ./include/linux/blk_types.h:10,
> > from ./include/linux/blkdev.h:9,
> > from fs/btrfs/disk-io.c:7:
> > fs/btrfs/disk-io.c: In function ‘csum_tree_block’:
> > fs/btrfs/disk-io.c:100:34: error: array subscript 1 is above array bounds of ‘struct page *[1]’ [-Werror=array-bounds]
> > 100 | kaddr = page_address(buf->pages[i]);
> > | ~~~~~~~~~~^~~
> > ./include/linux/mm.h:2135:48: note: in definition of macro ‘page_address’
> > 2135 | #define page_address(page) lowmem_page_address(page)
> > | ^~~~
> > cc1: all warnings being treated as errors
> >
> > We can check if i overflows to solve the problem. However, this doesn't make
> > much sense, since i == 1 and num_pages == 1 doesn't execute the body of the loop.
> > In addition, i < num_pages can also ensure that buf->pages[i] will not cross
> > the boundary. Unfortunately, this doesn't help with the problem observed here:
> > gcc still complains.
>
> So still false alerts, thus this bug should mostly be reported to GCC.
>
> >
> > To fix this, start the loop at index 0 instead of 1. Also, a conditional was
> > added to skip the case where the index is 0, so that the loop iterations follow
> > the desired logic, and it makes all versions of gcc happy.
> >
> > Signed-off-by: pengfuyuan <[email protected]>
> > ---
> > fs/btrfs/disk-io.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index fbf9006c6234..8b05d556d747 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -96,9 +96,13 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
> > crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
> > first_page_part - BTRFS_CSUM_SIZE);
> >
> > - for (i = 1; i < num_pages; i++) {
> > - kaddr = page_address(buf->pages[i]);
> > - crypto_shash_update(shash, kaddr, PAGE_SIZE);
> > + for (i = 0; i < num_pages; i++) {
> > + struct page *p = buf->pages[i];
> > +
> > + if (i != 0) {
> > + kaddr = page_address(p);
> > + crypto_shash_update(shash, kaddr, PAGE_SIZE);
>
> Unfortunately this damages the readability.
>
> If you really want to starts from page index 0, I don't think doing this
> is the correct way.
>
> Instead, you may take the chance to merge the first
> crypto_shahs_update() call, so the overall procedure looks like this:
>
> static void csum_tree_block()
> {
> for (int i = 0; i < num_pages; i++) {
> int page_off = whatever_to_calculate_the_offset;
> int page_len = whatever_to_calculate_the_lengh;
> char *kaddr = page_address(buf->pages[i]) + page_off;
>
> crypto_shash_update(shash, kaddr, page_len);
> }
> memset();
> crypto_shash_final();
> }
>
> Although even with such change, I'm still not sure if it's any better or
> worse, as most of the calculation can still be bulky.
Yeah I think the calculations would have to be conditional or keeping
some state. I'd like to keep the structure of the first page and the
rest.
Possible ways is to add extra condition
for (i = 1; i < num_pages && i < INLINE_EXTENT_BUFFER_PAGES; i++)
which leads to dead code if page size is 64k. It still has to check two
conditions which is not the best, so could do
int num_pages = max(num_extent_pages(eb0, INLINE_EXTENT_BUFFER_PAGES);
On Wed, May 24, 2023 at 07:46:42AM +0800, Qu Wenruo wrote:
>
>
> On 2023/5/24 03:32, David Sterba wrote:
> > On Tue, May 23, 2023 at 03:33:22PM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2023/5/23 15:09, pengfuyuan wrote:
> >>>
> >>> When compiling on a mips 64-bit machine we get these warnings:
> >>>
> >>> In file included from ./arch/mips/include/asm/cacheflush.h:13,
> >>> from ./include/linux/cacheflush.h:5,
> >>> from ./include/linux/highmem.h:8,
> >>> from ./include/linux/bvec.h:10,
> >>> from ./include/linux/blk_types.h:10,
> >>> from ./include/linux/blkdev.h:9,
> >>> from fs/btrfs/disk-io.c:7:
> >>> fs/btrfs/disk-io.c: In function ‘csum_tree_block’:
> >>> fs/btrfs/disk-io.c:100:34: error: array subscript 1 is above array bounds of ‘struct page *[1]’ [-Werror=array-bounds]
> >>> 100 | kaddr = page_address(buf->pages[i]);
> >>> | ~~~~~~~~~~^~~
> >>> ./include/linux/mm.h:2135:48: note: in definition of macro ‘page_address’
> >>> 2135 | #define page_address(page) lowmem_page_address(page)
> >>> | ^~~~
> >>> cc1: all warnings being treated as errors
> >>>
> >>> We can check if i overflows to solve the problem. However, this doesn't make
> >>> much sense, since i == 1 and num_pages == 1 doesn't execute the body of the loop.
> >>> In addition, i < num_pages can also ensure that buf->pages[i] will not cross
> >>> the boundary. Unfortunately, this doesn't help with the problem observed here:
> >>> gcc still complains.
> >>
> >> So still false alerts, thus this bug should mostly be reported to GCC.
> >>
> >>>
> >>> To fix this, start the loop at index 0 instead of 1. Also, a conditional was
> >>> added to skip the case where the index is 0, so that the loop iterations follow
> >>> the desired logic, and it makes all versions of gcc happy.
> >>>
> >>> Signed-off-by: pengfuyuan <[email protected]>
> >>> ---
> >>> fs/btrfs/disk-io.c | 10 +++++++---
> >>> 1 file changed, 7 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >>> index fbf9006c6234..8b05d556d747 100644
> >>> --- a/fs/btrfs/disk-io.c
> >>> +++ b/fs/btrfs/disk-io.c
> >>> @@ -96,9 +96,13 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
> >>> crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
> >>> first_page_part - BTRFS_CSUM_SIZE);
> >>>
> >>> - for (i = 1; i < num_pages; i++) {
> >>> - kaddr = page_address(buf->pages[i]);
> >>> - crypto_shash_update(shash, kaddr, PAGE_SIZE);
> >>> + for (i = 0; i < num_pages; i++) {
> >>> + struct page *p = buf->pages[i];
> >>> +
> >>> + if (i != 0) {
> >>> + kaddr = page_address(p);
> >>> + crypto_shash_update(shash, kaddr, PAGE_SIZE);
> >>
> >> Unfortunately this damages the readability.
> >>
> >> If you really want to starts from page index 0, I don't think doing this
> >> is the correct way.
> >>
> >> Instead, you may take the chance to merge the first
> >> crypto_shahs_update() call, so the overall procedure looks like this:
> >>
> >> static void csum_tree_block()
> >> {
> >> for (int i = 0; i < num_pages; i++) {
> >> int page_off = whatever_to_calculate_the_offset;
> >> int page_len = whatever_to_calculate_the_lengh;
> >> char *kaddr = page_address(buf->pages[i]) + page_off;
> >>
> >> crypto_shash_update(shash, kaddr, page_len);
> >> }
> >> memset();
> >> crypto_shash_final();
> >> }
> >>
> >> Although even with such change, I'm still not sure if it's any better or
> >> worse, as most of the calculation can still be bulky.
> >
> > Yeah I think the calculations would have to be conditional or keeping
> > some state. I'd like to keep the structure of the first page and the
> > rest.
>
> Yeah, there would be conditional checks, but it turns out to be simpler
> like the following:
>
> int cur = BTRFS_CSUM_SIZE;
>
> for (int i = 0; i < num_pages; i++) {
> int range_end = min(eb->len, (i + 1) << PAGE_SHIFT);
> int page_len = range_end - cur;
> int page_off = offset_in_page(cur);
>
> cypto_shash_update();
> cur = range_end;
> }
>
> The only conditional thing is the min() call, but I'm not sure if this
> is any more readable though...
And then comes some joker and says "why don't you just handle the
first page separately and then loop over full pages" :)
We could also put the whole loop under #if INLINE_EB_PAGES > 1. I've
checked that this type of iteration over the pages is only present in
this function so this should not become a pattern that would spread
elsewhere.
On Tue, May 23, 2023 at 09:32:12PM +0200, David Sterba wrote:
> On Tue, May 23, 2023 at 03:33:22PM +0800, Qu Wenruo wrote:
> > On 2023/5/23 15:09, pengfuyuan wrote:
> > Although even with such change, I'm still not sure if it's any better or
> > worse, as most of the calculation can still be bulky.
>
> Yeah I think the calculations would have to be conditional or keeping
> some state. I'd like to keep the structure of the first page and the
> rest.
>
> Possible ways is to add extra condition
>
> for (i = 1; i < num_pages && i < INLINE_EXTENT_BUFFER_PAGES; i++)
The final version is
for (i = 1; i < num_pages && INLINE_EXTENT_BUFFER_PAGES > 1; i++)
ie. 'INLINE_EXTENT_BUFFER_PAGES > 1' can be evaluated at compile time
and result in removing the for loop completely.
Pengfuyuan, can you please do a build test that it does not report the
warning anymore? The diff is:
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -88,7 +88,6 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
const int first_page_part = min_t(u32, PAGE_SIZE, fs_info->nodesize);
SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
char *kaddr;
- int i;
shash->tfm = fs_info->csum_shash;
crypto_shash_init(shash);
@@ -96,7 +95,7 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
first_page_part - BTRFS_CSUM_SIZE);
- for (i = 1; i < num_pages; i++) {
+ for (int i = 1; i < num_pages && INLINE_EXTENT_BUFFER_PAGES > 1; i++) {
kaddr = page_address(buf->pages[i]);
crypto_shash_update(shash, kaddr, PAGE_SIZE);
}
---
Thanks.
On 2023/5/26 22:35, David Sterba wrote:
> On Tue, May 23, 2023 at 09:32:12PM +0200, David Sterba wrote:
>> On Tue, May 23, 2023 at 03:33:22PM +0800, Qu Wenruo wrote:
>>> On 2023/5/23 15:09, pengfuyuan wrote:
>>> Although even with such change, I'm still not sure if it's any better or
>>> worse, as most of the calculation can still be bulky.
>> Yeah I think the calculations would have to be conditional or keeping
>> some state. I'd like to keep the structure of the first page and the
>> rest.
>>
>> Possible ways is to add extra condition
>>
>> for (i = 1; i < num_pages && i < INLINE_EXTENT_BUFFER_PAGES; i++)
> The final version is
>
> for (i = 1; i < num_pages && INLINE_EXTENT_BUFFER_PAGES > 1; i++)
>
> ie. 'INLINE_EXTENT_BUFFER_PAGES > 1' can be evaluated at compile time
> and result in removing the for loop completely.
>
> Pengfuyuan, can you please do a build test that it does not report the
> warning anymore? The diff is:
>
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -88,7 +88,6 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
> const int first_page_part = min_t(u32, PAGE_SIZE, fs_info->nodesize);
> SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
> char *kaddr;
> - int i;
>
> shash->tfm = fs_info->csum_shash;
> crypto_shash_init(shash);
> @@ -96,7 +95,7 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
> crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
> first_page_part - BTRFS_CSUM_SIZE);
>
> - for (i = 1; i < num_pages; i++) {
> + for (int i = 1; i < num_pages && INLINE_EXTENT_BUFFER_PAGES > 1; i++) {
> kaddr = page_address(buf->pages[i]);
> crypto_shash_update(shash, kaddr, PAGE_SIZE);
> }
> ---
I did a build test on the mips64 architecture, the compilation passed,
and it no longer reported warnings.
Thank you very much and wish you a happy life.
Thanks.
On Mon, May 29, 2023 at 11:28:59AM +0800, pengfuyuan wrote:
>
> On 2023/5/26 22:35, David Sterba wrote:
> > On Tue, May 23, 2023 at 09:32:12PM +0200, David Sterba wrote:
> >> On Tue, May 23, 2023 at 03:33:22PM +0800, Qu Wenruo wrote:
> >>> On 2023/5/23 15:09, pengfuyuan wrote:
> >>> Although even with such change, I'm still not sure if it's any better or
> >>> worse, as most of the calculation can still be bulky.
> > The final version is
> >
> > for (i = 1; i < num_pages && INLINE_EXTENT_BUFFER_PAGES > 1; i++)
> >
> > ie. 'INLINE_EXTENT_BUFFER_PAGES > 1' can be evaluated at compile time
> > and result in removing the for loop completely.
> >
> > Pengfuyuan, can you please do a build test that it does not report the
> > warning anymore? The diff is:
> >
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -88,7 +88,6 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
> > const int first_page_part = min_t(u32, PAGE_SIZE, fs_info->nodesize);
> > SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
> > char *kaddr;
> > - int i;
> >
> > shash->tfm = fs_info->csum_shash;
> > crypto_shash_init(shash);
> > @@ -96,7 +95,7 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
> > crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
> > first_page_part - BTRFS_CSUM_SIZE);
> >
> > - for (i = 1; i < num_pages; i++) {
> > + for (int i = 1; i < num_pages && INLINE_EXTENT_BUFFER_PAGES > 1; i++) {
> > kaddr = page_address(buf->pages[i]);
> > crypto_shash_update(shash, kaddr, PAGE_SIZE);
> > }
> > ---
>
> I did a build test on the mips64 architecture, the compilation passed,
> and it no longer reported warnings.
Thanks, patch added to misc-next.