Re: [PATCH v2 0/2] btrfs: migrate extent_buffer::pages[] to folio and more cleanups

David Sterba
2023-12-05T14:11:43Z

On Tue, Dec 05, 2023 at 07:40:55AM +1030, Qu Wenruo wrote:
> On 2023/12/5 02:56, David Sterba wrote:
> > On Fri, Dec 01, 2023 at 04:36:53PM +1030, Qu Wenruo wrote:
> >> [CHANGELOG]
> >> v2:
> >> - Adda new patch to do more cleanups for metadata page pointers usage
> >>
> >> This patchset would migrate extent_buffer::pages[] to folios[], then
> >> cleanup the existing metadata page pointer usage to proper folios ones.
> >>
> >> This cleanup would help future higher order folios usage for metadata in
> >> the following aspects:
> >>
> >> - No more need to iterate through the remaining pages for page flags
> >> We just call folio_set/mark/start_*() helpers, for the single folio.
> >> The helper would only set the flag (mostly for the leading page).
> >>
> >> - Single bio_add_folio() call for the whole eb
> >>
> >> - Better filio helpers naming
> >> PageUptodate() compared to folio_test_uptodate().
> >>
> >> The first patch would do a very simple conversion, then the 2nd patch do
> >> the prepartion for the higher order folio situation.
> >>
> >> There are two locations which won't be converted to folios yet:
> >>
> >> - Subpage code
> >> There is no meaning to support higher order folio for subpage.
> >> The two conditions are just conflicting with each other.
> >>
> >> - Data page pointers
> >> That would be more useful in the future, before we going to support
> >> multi-page sectorsize.
> >>
> >> However the 2nd one would also add a new corner case:
> >>
> >> - Order mismatch in filemap and eb folios
> >> Unforatunately I don't have a better plan other than re-allocate the
> >> folios to the same order.
> >> Maybe in the future we would have better ways to handle it? Like
> >> migrating the pages to the higher order one?
> >
> > As long as it's a no-op for now this is OK, we can do the higher order
> > allocation for eb pages afterwards.
> >
> Yep, it won't cause any problem for now.
>
> Although this corner case is making me wondering if the new
> alloc-then-attach is really any better than the original
> alloc-and-attach solution.
>
> If the mm (filemap) layer can allow us to allocate larger folios, it may
> be much simpler.
> The current code only needs to setlarge folio support for the mapping,
> then go with high order fpg_flags.
> The filemap code is already doing the retry and unalignment check.
>
> But the existing filemap code would also try to reduce the order, which
> can lead to other problems, like one extent buffer with multiple
> different order folios.
> Meanwhile alloc-and-attach gives us full control on the order, thus
> allowing all-or-none (one single large folio, or all single page ones)
> solution required by the 2nd patch.
>
> Anyway, I would continue with the current alloc-then-attach method to
> experiment the higher order folios allocation first to find out all the
> pitfalls first.

Agreed, we'll eventually converge to the full folio API but what you
found so far seems that we can expect surprises.