Re: [PATCH v2 07/17] btrfs: make rbio_add_io_page() subpage compatible

[David Sterba <dsterba@suse.cz>]

On Fri, Apr 15, 2022 at 06:48:07AM +0800, Qu Wenruo wrote:
> 
> 
> On 2022/4/14 23:51, David Sterba wrote:
> > On Thu, Apr 14, 2022 at 06:59:58PM +0800, Qu Wenruo wrote:
> >>>> to see what's wrong.
> >>>
> >>> It's a rebase error.
> >>>
> >>> At least one patch has incorrect diff snippet.
> >>>
> >>> For the patch "btrfs: raid56: introduce btrfs_raid_bio::stripe_sectors":
> >>>
> >>> For alloc_rbio(), in my branch and patch (v2) submitted to the mailing
> >>> list:
> >>>
> >>> @@ -978,6 +1014,7 @@ static struct btrfs_raid_bio *alloc_rbio(struct
> >>> btrfs_fs_info *fs_info,
> >>>           rbio = kzalloc(sizeof(*rbio) +
> >>>                          sizeof(*rbio->stripe_pages) * num_pages +
> >>>                          sizeof(*rbio->bio_pages) * num_pages +
> >>> +                      sizeof(*rbio->stripe_sectors) * num_sectors +
> >>
> >> The more I check this part of the existing code, the worse it looks to me.
> >>
> >> We're really dancing on the edge, it's just a problem of time for us to
> >> get a cut.
> >>
> >> Shouldn't we go the regular way, kzalloc the structure only, then alloc
> >> needed pages/bitmaps?
> >
> > Yeah I agree that this has become unwieldy with the definitions and
> > calculations. One thing to consider is the real memory footprint and the
> > overhead of one kmalloc vs several kmalloc of smaller memory chunks.
> 
> One thing to mention is, although our one kmalloc seems to be some
> optimization, I guess the benefit is already small or none, especially
> after subpage patchset.
> 
> Just check for 3 disks RAID5, one btrfs_raid_bio already takes over 2Kilo:
> 
>   After:  2320 (+97.8%) (From the cover letter)
> 
> In this case, I doubt smaller kmallocs would cause anything worse, as
> kmalloc() will give us a 4K page/range anyway.

Small allocations fall into the kmalloc-<size> slabs.

> > Both have pros and cons but if there is no real effect then we may have
> > better code with the separate allocations.
> 
> There are some ways to optimize, but they may be a little aggressive.
> 
> The most aggressive one would be, replace "struct page **stripe_pages"
> to "struct page *stripe_page", and use alloc_pages_exact() to allocate a
> range of physically adjacent pages.
> 
> In fact I'm even considering this for extent buffer pages, but it would
> cause way more unexpected ENOMEM when memory is fragmented, thus too
> aggressive.

Yeah we must assume the memory can get fragmented so allocating
individual pages must stay as a fallback. As extent bufferes represent
the metadata, we can't afford to return such errors in case there's
enough memory but not in the shape we'd like it.

I've thought about using the contig allocaiton too as it makes the
memcpy in the eb helpers much easier. What could be done is to have
both, optimistically allocating contiguous range and falling back to
what we have now, tracking the status in the eb flags. This means that
there would be a fast path if we're lucky and get the contiguous range
and this could speed some things up.