From: "Gustavo A. R. Silva" <gustavoars@kernel.org>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
David Sterba <dsterba@suse.com>,
linux-btrfs@vger.kernel.org
Subject: Re: [PATCH] btrfs: add special case to setget helpers for 64k pages
Date: Thu, 1 Jul 2021 19:39:36 -0500 [thread overview]
Message-ID: <20210702003936.GA13456@embeddedor> (raw)
In-Reply-To: <62ec2948-77a3-6e50-7940-8de259b8671f@gmx.com>
On Fri, Jul 02, 2021 at 08:21:31AM +0800, Qu Wenruo wrote:
>
>
> On 2021/7/2 上午8:09, Gustavo A. R. Silva wrote:
> >
> >
> > On 7/1/21 18:59, Qu Wenruo wrote:
> > >
> > >
> > > On 2021/7/2 上午5:57, Gustavo A. R. Silva wrote:
> > > > On Thu, Jul 01, 2021 at 06:00:39PM +0200, David Sterba wrote:
> > > > > On 64K pages the size of the extent_buffer::pages array is 1 and
> > > > > compilation with -Warray-bounds warns due to
> > > > >
> > > > > kaddr = page_address(eb->pages[idx + 1]);
> > > > >
> > > > > when reading byte range crossing page boundary.
> > > > >
> > > > > This does never actually overflow the array because on 64K because all
> > > > > the data fit in one page and bounds are checked by check_setget_bounds.
> > > > >
> > > > > To fix the reported overflow and warning add a copy of the non-crossing
> > > > > read/write code and put it behind a condition that's evaluated at
> > > > > compile time. That way only one implementation remains due to dead code
> > > > > elimination.
> > > >
> > > > Any chance we can use a flexible-array in struct extent_buffer instead,
> > > > so all the warnings are removed?
> > > >
> > > > Something like this:
> > > >
> > > > diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> > > > index 62027f551b44..b82e8b694a3b 100644
> > > > --- a/fs/btrfs/extent_io.h
> > > > +++ b/fs/btrfs/extent_io.h
> > > > @@ -94,11 +94,11 @@ struct extent_buffer {
> > > >
> > > > struct rw_semaphore lock;
> > > >
> > > > - struct page *pages[INLINE_EXTENT_BUFFER_PAGES];
> > > > struct list_head release_list;
> > > > #ifdef CONFIG_BTRFS_DEBUG
> > > > struct list_head leak_list;
> > > > #endif
> > > > + struct page *pages[];
> > > > };
> > >
> > > But wouldn't that make the the size of extent_buffer structure change
> > > and affect the kmem cache for it?
> >
> > Could you please point out the places in the code that would be
> > affected?
>
> Sure, the direct code get affected is here:
>
> extent_io.c:
> int __init extent_io_init(void)
> {
> extent_buffer_cache = kmem_cache_create("btrfs_extent_buffer",
> sizeof(struct extent_buffer), 0,
> SLAB_MEM_SPREAD, NULL);
>
> So here we can no longer use sizeof(struct extent_buffer);
>
> Furthermore, this function is called at btrfs module load time,
> at that time we have no idea how large the extent buffer could be, thus we
> must allocate a large enough cache for extent buffer.
>
> Thus the size will be fixed to the current size, no matter if we use flex
> array or not.
>
> Though I'm not sure if using such flex array with fixed real size can silent
> the warning though.
Yeah; I think this might be the right solution:
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 9e81d25dea70..4cf0b72fdd9f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -232,8 +232,9 @@ int __init extent_state_cache_init(void)
int __init extent_io_init(void)
{
extent_buffer_cache = kmem_cache_create("btrfs_extent_buffer",
- sizeof(struct extent_buffer), 0,
- SLAB_MEM_SPREAD, NULL);
+ struct_size((struct extent_buffer *)0, pages,
+ INLINE_EXTENT_BUFFER_PAGES),
+ 0, SLAB_MEM_SPREAD, NULL);
if (!extent_buffer_cache)
return -ENOMEM;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 62027f551b44..b82e8b694a3b 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -94,11 +94,11 @@ struct extent_buffer {
struct rw_semaphore lock;
- struct page *pages[INLINE_EXTENT_BUFFER_PAGES];
struct list_head release_list;
#ifdef CONFIG_BTRFS_DEBUG
struct list_head leak_list;
#endif
+ struct page *pages[];
};
/*
I see zero warnings by building with
make ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu- ppc64_defconfig
and -Warray-bounds enabled by default:
diff --git a/Makefile b/Makefile
index c2cc2fa28525..310452119ab5 100644
--- a/Makefile
+++ b/Makefile
@@ -1068,7 +1068,6 @@ KBUILD_CFLAGS += $(call cc-disable-warning, stringop-truncation)
# We'll want to enable this eventually, but it's not going away for 5.7 at least
KBUILD_CFLAGS += $(call cc-disable-warning, zero-length-bounds)
-KBUILD_CFLAGS += $(call cc-disable-warning, array-bounds)
KBUILD_CFLAGS += $(call cc-disable-warning, stringop-overflow)
# Another good warning that we'll want to enable eventually
Do you think there is any other place where we should update
the total size for extent_buffer?
--
Gustavo
>
> Thanks,
> Qu
> >
> > I'm trying to understand this code and see the possibility of
> > using a flex-array together with proper memory allocation, so
> > we can avoid having one-element array in extent_buffer.
> >
> > Not sure at what extent this would be possible. So, any pointer
> > is greatly appreciate it. :)
> >
> > Thanks
> > --
> > Gustavo
> >
> > >
> > > Thanks,
> > > Qu
> > > >
> > > > /*
> > > >
> > > > which is actually what is needed in this case to silence the
> > > > array-bounds warnings: the replacement of the one-element array
> > > > with a flexible-array member[1] in struct extent_buffer.
> > > >
> > > > --
> > > > Gustavo
> > > >
> > > > [1] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
> > > >
> > > > >
> > > > > Link: https://lore.kernel.org/lkml/20210623083901.1d49d19d@canb.auug.org.au/
> > > > > CC: Gustavo A. R. Silva <gustavoars@kernel.org>
> > > > > Signed-off-by: David Sterba <dsterba@suse.com>
> > > > > ---
> > > > > fs/btrfs/struct-funcs.c | 66 +++++++++++++++++++++++++----------------
> > > > > 1 file changed, 41 insertions(+), 25 deletions(-)
> > > > >
> > > > > diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c
> > > > > index 8260f8bb3ff0..51204b280da8 100644
> > > > > --- a/fs/btrfs/struct-funcs.c
> > > > > +++ b/fs/btrfs/struct-funcs.c
> > > > > @@ -73,14 +73,18 @@ u##bits btrfs_get_token_##bits(struct btrfs_map_token *token, \
> > > > > } \
> > > > > token->kaddr = page_address(token->eb->pages[idx]); \
> > > > > token->offset = idx << PAGE_SHIFT; \
> > > > > - if (oip + size <= PAGE_SIZE) \
> > > > > + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \
> > > > > return get_unaligned_le##bits(token->kaddr + oip); \
> > > > > + } else { \
> > > > > + if (oip + size <= PAGE_SIZE) \
> > > > > + return get_unaligned_le##bits(token->kaddr + oip); \
> > > > > \
> > > > > - memcpy(lebytes, token->kaddr + oip, part); \
> > > > > - token->kaddr = page_address(token->eb->pages[idx + 1]); \
> > > > > - token->offset = (idx + 1) << PAGE_SHIFT; \
> > > > > - memcpy(lebytes + part, token->kaddr, size - part); \
> > > > > - return get_unaligned_le##bits(lebytes); \
> > > > > + memcpy(lebytes, token->kaddr + oip, part); \
> > > > > + token->kaddr = page_address(token->eb->pages[idx + 1]); \
> > > > > + token->offset = (idx + 1) << PAGE_SHIFT; \
> > > > > + memcpy(lebytes + part, token->kaddr, size - part); \
> > > > > + return get_unaligned_le##bits(lebytes); \
> > > > > + } \
> > > > > } \
> > > > > u##bits btrfs_get_##bits(const struct extent_buffer *eb, \
> > > > > const void *ptr, unsigned long off) \
> > > > > @@ -94,13 +98,17 @@ u##bits btrfs_get_##bits(const struct extent_buffer *eb, \
> > > > > u8 lebytes[sizeof(u##bits)]; \
> > > > > \
> > > > > ASSERT(check_setget_bounds(eb, ptr, off, size)); \
> > > > > - if (oip + size <= PAGE_SIZE) \
> > > > > + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \
> > > > > return get_unaligned_le##bits(kaddr + oip); \
> > > > > + } else { \
> > > > > + if (oip + size <= PAGE_SIZE) \
> > > > > + return get_unaligned_le##bits(kaddr + oip); \
> > > > > \
> > > > > - memcpy(lebytes, kaddr + oip, part); \
> > > > > - kaddr = page_address(eb->pages[idx + 1]); \
> > > > > - memcpy(lebytes + part, kaddr, size - part); \
> > > > > - return get_unaligned_le##bits(lebytes); \
> > > > > + memcpy(lebytes, kaddr + oip, part); \
> > > > > + kaddr = page_address(eb->pages[idx + 1]); \
> > > > > + memcpy(lebytes + part, kaddr, size - part); \
> > > > > + return get_unaligned_le##bits(lebytes); \
> > > > > + } \
> > > > > } \
> > > > > void btrfs_set_token_##bits(struct btrfs_map_token *token, \
> > > > > const void *ptr, unsigned long off, \
> > > > > @@ -124,15 +132,19 @@ void btrfs_set_token_##bits(struct btrfs_map_token *token, \
> > > > > } \
> > > > > token->kaddr = page_address(token->eb->pages[idx]); \
> > > > > token->offset = idx << PAGE_SHIFT; \
> > > > > - if (oip + size <= PAGE_SIZE) { \
> > > > > + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \
> > > > > put_unaligned_le##bits(val, token->kaddr + oip); \
> > > > > - return; \
> > > > > + } else { \
> > > > > + if (oip + size <= PAGE_SIZE) { \
> > > > > + put_unaligned_le##bits(val, token->kaddr + oip); \
> > > > > + return; \
> > > > > + } \
> > > > > + put_unaligned_le##bits(val, lebytes); \
> > > > > + memcpy(token->kaddr + oip, lebytes, part); \
> > > > > + token->kaddr = page_address(token->eb->pages[idx + 1]); \
> > > > > + token->offset = (idx + 1) << PAGE_SHIFT; \
> > > > > + memcpy(token->kaddr, lebytes + part, size - part); \
> > > > > } \
> > > > > - put_unaligned_le##bits(val, lebytes); \
> > > > > - memcpy(token->kaddr + oip, lebytes, part); \
> > > > > - token->kaddr = page_address(token->eb->pages[idx + 1]); \
> > > > > - token->offset = (idx + 1) << PAGE_SHIFT; \
> > > > > - memcpy(token->kaddr, lebytes + part, size - part); \
> > > > > } \
> > > > > void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr, \
> > > > > unsigned long off, u##bits val) \
> > > > > @@ -146,15 +158,19 @@ void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr, \
> > > > > u8 lebytes[sizeof(u##bits)]; \
> > > > > \
> > > > > ASSERT(check_setget_bounds(eb, ptr, off, size)); \
> > > > > - if (oip + size <= PAGE_SIZE) { \
> > > > > + if (INLINE_EXTENT_BUFFER_PAGES == 1) { \
> > > > > put_unaligned_le##bits(val, kaddr + oip); \
> > > > > - return; \
> > > > > - } \
> > > > > + } else { \
> > > > > + if (oip + size <= PAGE_SIZE) { \
> > > > > + put_unaligned_le##bits(val, kaddr + oip); \
> > > > > + return; \
> > > > > + } \
> > > > > \
> > > > > - put_unaligned_le##bits(val, lebytes); \
> > > > > - memcpy(kaddr + oip, lebytes, part); \
> > > > > - kaddr = page_address(eb->pages[idx + 1]); \
> > > > > - memcpy(kaddr, lebytes + part, size - part); \
> > > > > + put_unaligned_le##bits(val, lebytes); \
> > > > > + memcpy(kaddr + oip, lebytes, part); \
> > > > > + kaddr = page_address(eb->pages[idx + 1]); \
> > > > > + memcpy(kaddr, lebytes + part, size - part); \
> > > > > + } \
> > > > > }
> > > > >
> > > > > DEFINE_BTRFS_SETGET_BITS(8)
> > > > > --
> > > > > 2.31.1
> > > > >
next prev parent reply other threads:[~2021-07-02 0:37 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-01 16:00 [PATCH] btrfs: add special case to setget helpers for 64k pages David Sterba
2021-07-01 21:57 ` Gustavo A. R. Silva
2021-07-01 23:59 ` Qu Wenruo
2021-07-02 0:09 ` Gustavo A. R. Silva
2021-07-02 0:21 ` Qu Wenruo
2021-07-02 0:39 ` Gustavo A. R. Silva [this message]
2021-07-02 0:39 ` Qu Wenruo
2021-07-02 1:09 ` Gustavo A. R. Silva
2021-07-02 10:22 ` David Sterba
2021-07-02 7:10 ` Christoph Hellwig
2021-07-02 11:06 ` David Sterba
2021-07-05 8:33 ` Christoph Hellwig
2021-07-08 14:34 ` David Sterba
2021-07-14 23:37 ` Gustavo A. R. Silva
2021-07-28 15:32 ` David Sterba
2021-07-28 16:00 ` David Sterba
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210702003936.GA13456@embeddedor \
--to=gustavoars@kernel.org \
--cc=dsterba@suse.com \
--cc=gustavo@embeddedor.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=quwenruo.btrfs@gmx.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.