Re: [PATCH] btrfs: add special case to setget helpers for 64k pages

[David Sterba <dsterba@suse.cz>]

On Thu, Jul 01, 2021 at 07:39:36PM -0500, Gustavo A. R. Silva wrote:
> 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[];

IMHO this is going the wrong way, INLINE_EXTENT_BUFFER_PAGES is a
compile time constant and the array is not variable sized at all, so
adding the end member and using struct_size is just manually coding
what would compiler do for free.