Re: [PATCH 5/7] mm: add new 'orders' parameter for find_get_entries() and find_lock_entries()

[Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>]

On 2024/6/10 23:23, Daniel Gomez wrote:
> Hi Baolin,
>
> On Thu, Jun 06, 2024 at 07:58:55PM +0800, Baolin Wang wrote:
> > In the following patches, shmem will support the swap out of large folios,
> > which means the shmem mappings may contain large order swap entries, so an
> > 'orders' array is added for find_get_entries() and find_lock_entries() to
> > obtain the order size of shmem swap entries, which will help in the release
> > of shmem large folio swap entries.
> >
> > Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> > ---
> >   mm/filemap.c  | 27 +++++++++++++++++++++++++--
> >   mm/internal.h |  4 ++--
> >   mm/shmem.c    | 17 +++++++++--------
> >   mm/truncate.c |  8 ++++----
> >   4 files changed, 40 insertions(+), 16 deletions(-)
> >
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 37061aafd191..47fcd9ee6012 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -2036,14 +2036,24 @@ static inline struct folio *find_get_entry(struct xa_state *xas, pgoff_t max,
> >    * Return: The number of entries which were found.
> >    */
> >   unsigned find_get_entries(struct address_space *mapping, pgoff_t *start,
> > -		pgoff_t end, struct folio_batch *fbatch, pgoff_t *indices)
> > +		pgoff_t end, struct folio_batch *fbatch, pgoff_t *indices,
> > +		int *orders)
> >   {
> >   	XA_STATE(xas, &mapping->i_pages, *start);
> >   	struct folio *folio;
> > +	int order;
> >   
> >   	rcu_read_lock();
> >   	while ((folio = find_get_entry(&xas, end, XA_PRESENT)) != NULL) {
> >   		indices[fbatch->nr] = xas.xa_index;
> > +		if (orders) {
> > +			if (!xa_is_value(folio))
> > +				order = folio_order(folio);
> > +			else
> > +				order = xa_get_order(xas.xa, xas.xa_index);
> > +
> > +			orders[fbatch->nr] = order;
> > +		}
> >   		if (!folio_batch_add(fbatch, folio))
> >   			break;
> >   	}
> > @@ -2056,6 +2066,8 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t *start,
> >   		folio = fbatch->folios[idx];
> >   		if (!xa_is_value(folio))
> >   			nr = folio_nr_pages(folio);
> > +		else if (orders)
> > +			nr = 1 << orders[idx];
> >   		*start = indices[idx] + nr;
> >   	}
> >   	return folio_batch_count(fbatch);
> > @@ -2082,10 +2094,12 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t *start,
> >    * Return: The number of entries which were found.
> >    */
> >   unsigned find_lock_entries(struct address_space *mapping, pgoff_t *start,
> > -		pgoff_t end, struct folio_batch *fbatch, pgoff_t *indices)
> > +		pgoff_t end, struct folio_batch *fbatch, pgoff_t *indices,
> > +		int *orders)
> >   {
> >   	XA_STATE(xas, &mapping->i_pages, *start);
> >   	struct folio *folio;
> > +	int order;
> >   
> >   	rcu_read_lock();
> >   	while ((folio = find_get_entry(&xas, end, XA_PRESENT))) {
> > @@ -2099,9 +2113,16 @@ unsigned find_lock_entries(struct address_space *mapping, pgoff_t *start,
> >   			if (folio->mapping != mapping ||
> >   			    folio_test_writeback(folio))
> >   				goto unlock;
> > +			if (orders)
> > +				order = folio_order(folio);
> >   			VM_BUG_ON_FOLIO(!folio_contains(folio, xas.xa_index),
> >   					folio);
> > +		} else if (orders) {
> > +			order = xa_get_order(xas.xa, xas.xa_index);
> >   		}
> > +
> > +		if (orders)
> > +			orders[fbatch->nr] = order;
> >   		indices[fbatch->nr] = xas.xa_index;
> >   		if (!folio_batch_add(fbatch, folio))
> >   			break;
> > @@ -2120,6 +2141,8 @@ unsigned find_lock_entries(struct address_space *mapping, pgoff_t *start,
> >   		folio = fbatch->folios[idx];
> >   		if (!xa_is_value(folio))
> >   			nr = folio_nr_pages(folio);
> > +		else if (orders)
> > +			nr = 1 << orders[idx];
> >   		*start = indices[idx] + nr;
> >   	}
> >   	return folio_batch_count(fbatch);
> > diff --git a/mm/internal.h b/mm/internal.h
> > index 3419c329b3bc..0b5adb6c33cc 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -339,9 +339,9 @@ static inline void force_page_cache_readahead(struct address_space *mapping,
> >   }
> >   
> >   unsigned find_lock_entries(struct address_space *mapping, pgoff_t *start,
> > -		pgoff_t end, struct folio_batch *fbatch, pgoff_t *indices);
> > +		pgoff_t end, struct folio_batch *fbatch, pgoff_t *indices, int *orders);
> >   unsigned find_get_entries(struct address_space *mapping, pgoff_t *start,
> > -		pgoff_t end, struct folio_batch *fbatch, pgoff_t *indices);
> > +		pgoff_t end, struct folio_batch *fbatch, pgoff_t *indices, int *orders);
> >   void filemap_free_folio(struct address_space *mapping, struct folio *folio);
> >   int truncate_inode_folio(struct address_space *mapping, struct folio *folio);
> >   bool truncate_inode_partial_folio(struct folio *folio, loff_t start,
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 0ac71580decb..28ba603d87b8 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -840,14 +840,14 @@ static void shmem_delete_from_page_cache(struct folio *folio, void *radswap)
> >    * Remove swap entry from page cache, free the swap and its page cache.
> >    */
> >   static int shmem_free_swap(struct address_space *mapping,
> > -			   pgoff_t index, void *radswap)
> > +			   pgoff_t index, void *radswap, int order)
> >   {
> >   	void *old;
>
> Matthew Wilcox suggested [1] returning the number of pages freed in shmem_free_swap().
>
> [1] https://lore.kernel.org/all/ZQRf2pGWurrE0uO+@xxxxxxxxxxxxxxxxxxxx/
>
> Which I submitted here:
> https://lore.kernel.org/all/20231028211518.3424020-5-da.gomez@xxxxxxxxxxx/
>
> Do you agree with the suggestion? If so, could we update my patch to use
> free_swap_and_cache_nr() or include that here?

Yes, this looks good to me. But we still need some modification for 
find_lock_entries() and find_get_entries() to update the '*start' 
correctly. I will include your changes into this patch in next version. 
Thanks.