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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

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@linux.alibaba.com>
>> ---
>>   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+@casper.infradead.org/
> 
> Which I submitted here:
> https://lore.kernel.org/all/20231028211518.3424020-5-da.gomez@samsung.com/
> 
> 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.