Re: [PATCH v2 7/8] mm: drop page_index/page_file_offset and convert swap helpers to use folio

[Kairui Song]

On Wed, Apr 24, 2024 at 10:19 AM Huang, Ying <ying.huang@xxxxxxxxx> wrote:
>
> Kairui Song <ryncsn@xxxxxxxxx> writes:
>
> > From: Kairui Song <kasong@xxxxxxxxxxx>
> >
> > There are four helpers for retrieving the page index within address
> > space, or offset within mapped file:
> >
> > - page_index
> > - page_file_offset
> > - folio_index (equivalence of page_index)
> > - folio_file_pos (equivalence of page_file_offset)
> >
> > And they are only needed for mixed usage of swap cache & page cache (eg.
> > migration, huge memory split). Else users can just use folio->index or
> > folio_pos.
> >
> > This commit drops page_index and page_file_offset as we have eliminated
> > all users, and converts folio_index and folio_file_pos (they were simply
> > wrappers of page_index and page_file_offset, and implemented in a not
> > very clean way) to use folio internally.
> >
> > After this commit, there will be only two helpers for users that may
> > encounter mixed usage of swap cache and page cache:
> >
> > - folio_index (calls __folio_swap_cache_index for swap cache folio)
> > - folio_file_pos (calls __folio_swap_dev_pos for swap cache folio)
> >
> > The index in swap cache and offset in swap device are still basically
> > the same thing, but will be different in following commits.
> >
> > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> > ---
> >  include/linux/mm.h      | 13 -------------
> >  include/linux/pagemap.h | 19 +++++++++----------
> >  mm/swapfile.c           | 13 +++++++++----
> >  3 files changed, 18 insertions(+), 27 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 0436b919f1c7..797480e76c9c 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2245,19 +2245,6 @@ static inline void *folio_address(const struct folio *folio)
> >       return page_address(&folio->page);
> >  }
> >
> > -extern pgoff_t __page_file_index(struct page *page);
> > -
> > -/*
> > - * Return the pagecache index of the passed page.  Regular pagecache pages
> > - * use ->index whereas swapcache pages use swp_offset(->private)
> > - */
> > -static inline pgoff_t page_index(struct page *page)
> > -{
> > -     if (unlikely(PageSwapCache(page)))
> > -             return __page_file_index(page);
> > -     return page->index;
> > -}
> > -
> >  /*
> >   * Return true only if the page has been allocated with
> >   * ALLOC_NO_WATERMARKS and the low watermark was not
> > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> > index 2df35e65557d..a7d025571ee6 100644
> > --- a/include/linux/pagemap.h
> > +++ b/include/linux/pagemap.h
> > @@ -780,7 +780,7 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
> >                       mapping_gfp_mask(mapping));
> >  }
> >
> > -#define swapcache_index(folio)       __page_file_index(&(folio)->page)
> > +extern pgoff_t __folio_swap_cache_index(struct folio *folio);
> >
> >  /**
> >   * folio_index - File index of a folio.
> > @@ -795,9 +795,9 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
> >   */
> >  static inline pgoff_t folio_index(struct folio *folio)
> >  {
> > -        if (unlikely(folio_test_swapcache(folio)))
> > -                return swapcache_index(folio);
> > -        return folio->index;
> > +     if (unlikely(folio_test_swapcache(folio)))
> > +             return __folio_swap_cache_index(folio);
> > +     return folio->index;
> >  }
> >
> >  /**
> > @@ -920,11 +920,6 @@ static inline loff_t page_offset(struct page *page)
> >       return ((loff_t)page->index) << PAGE_SHIFT;
> >  }
> >
> > -static inline loff_t page_file_offset(struct page *page)
> > -{
> > -     return ((loff_t)page_index(page)) << PAGE_SHIFT;
> > -}
> > -
> >  /**
> >   * folio_pos - Returns the byte position of this folio in its file.
> >   * @folio: The folio.
> > @@ -934,6 +929,8 @@ static inline loff_t folio_pos(struct folio *folio)
> >       return page_offset(&folio->page);
> >  }
> >
> > +extern loff_t __folio_swap_dev_pos(struct folio *folio);
> > +
> >  /**
> >   * folio_file_pos - Returns the byte position of this folio in its file.
> >   * @folio: The folio.
> > @@ -943,7 +940,9 @@ static inline loff_t folio_pos(struct folio *folio)
> >   */
> >  static inline loff_t folio_file_pos(struct folio *folio)
> >  {
> > -     return page_file_offset(&folio->page);
> > +     if (unlikely(folio_test_swapcache(folio)))
> > +             return __folio_swap_dev_pos(folio);
> > +     return ((loff_t)folio->index << PAGE_SHIFT);
>
> This still looks confusing for me.  The function returns the byte
> position of the folio in its file.  But we returns the swap device
> position of the folio.

Thanks for the comment.

This doesn't look too confusing to me, __folio_swap_dev_pos ->
swap_dev_pos also returns the byte position of the folio in swap
device. If we agree swap device is kind of equivalent to the page
cache file here, it shouldn't be too hard to understand.

>
> Tried to search folio_file_pos() usage.  The 2 usage in page_io.c is
> swap specific, we can use swap_dev_pos() directly.

The 2 usage in page_io.c is already converted to use swap_dev_pos
directly in this series (patch 6/8).

>
> There are also other file system users (NFS and AFS) of
> folio_file_pos(), I don't know why they need to work with swap
> cache. Cced file system maintainers for help.
>

Thanks, I'm not very sure if we can just drop folio_file_pos and
convert all users to use folio_pos directly. Swap cache mapping
shouldn't be exposed to fs, but I'm not confident enough that this is
a safe move. It looks OK to do so just by examining NFS code, but
let's wait for feedback from FS people.