Re: [PATCH v1 5/5] mm/shmem: optimize file read with folio batching

[Chi Zhiling <chizhiling@163.com>]

On 5/25/26 4:37 PM, Baolin Wang wrote:
> 
> 
> On 5/20/26 6:15 PM, Chi Zhiling wrote:
>> From: Chi Zhiling <chizhiling@kylinos.cn>
>>
>> Optimize shmem file read by using filemap_get_folios_contig() to
>> batch fetch contiguous folios from the page cache, reducing the
>> overhead of repeated shmem_get_folio() calls.
>>
>> When the folio batch is exhausted, attempt to refill it with
>> filemap_get_folios_contig(). If no folios are found (hole or swapped
>> out pages), fall back to shmem_get_folio() to handle these cases
>> individually.
>>
>> Additionally:
>> - Defer folio_put() until the batch is exhausted or on exit
>> - Add folio_test_uptodate() check before copying to ensure data
>>    validity
>>
>> Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
>> ---
>>   mm/shmem.c | 50 ++++++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 36 insertions(+), 14 deletions(-)
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 96ea5a4c0aff..e0eacc23cccd 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -3355,11 +3355,14 @@ static ssize_t shmem_file_read_iter(struct 
>> kiocb *iocb, struct iov_iter *to)
>>       struct file *file = iocb->ki_filp;
>>       struct inode *inode = file_inode(file);
>>       struct address_space *mapping = inode->i_mapping;
>> +    struct folio_batch fbatch;
>>       pgoff_t index;
>>       unsigned long offset;
>>       int error = 0;
>>       ssize_t retval = 0;
>> +    folio_batch_init(&fbatch);
>> +
>>       for (;;) {
>>           struct folio *folio = NULL;
>>           struct page *page = NULL;
>> @@ -3372,18 +3375,38 @@ static ssize_t shmem_file_read_iter(struct 
>> kiocb *iocb, struct iov_iter *to)
>>               break;
>>           index = iocb->ki_pos >> PAGE_SHIFT;
>> -        error = shmem_get_folio(inode, index, 0, &folio, SGP_READ);
>> -        if (error) {
>> -            if (error == -EINVAL)
>> -                error = 0;
>> -            break;
>> +fetch:
>> +        folio = folio_batch_next(&fbatch);
>> +        if (!folio) {
>> +            pgoff_t start = index;
>> +            pgoff_t end = (iocb->ki_pos + to->count - 1) >> PAGE_SHIFT;
>> +
>> +            if (folio_batch_count(&fbatch)) {
>> +                for (int i = 0; i < folio_batch_count(&fbatch); i++)
>> +                    folio_put(fbatch.folios[i]);
>> +                folio_batch_reinit(&fbatch);
>> +            }
>> +
>> +            filemap_get_folios_contig(inode->i_mapping, &start, end, 
>> &fbatch);
>> +            if (folio_batch_count(&fbatch))
>> +                goto fetch;
>> +
>> +            error = shmem_get_folio(inode, index, 0, &folio, SGP_READ);
>> +            if (unlikely(error)) {
>> +                if (error == -EINVAL)
>> +                    error = 0;
>> +                break;
>> +            }
>> +            if (folio) {
>> +                folio_unlock(folio);
>> +                folio_batch_add(&fbatch, folio);
>> +                fbatch.i++;
>> +            }
>>           }
>> -        if (folio) {
>> -            folio_unlock(folio);
>> +        if (folio) {
>>               page = folio_file_page(folio, index);
>>               if (PageHWPoison(page)) {
>> -                folio_put(folio);
>>                   error = -EIO;
>>                   break;
>>               }
> 
> I haven't tested it yet, but I'm sure this will break the 
> fallback_page_copy mode.
> 
> You're fetching from the batch at folio granularity here, but in 
> fallback_page_copy mode, we still copy the data at page granularity.

Yes, that's a bug, Sashika has already reported it. I’ll fix it in the 
next version.

Sashika also reported another potential issue below, but I think that is 
a misunderstanding, because a folio fetched from the swap cache is 
already marked Uptodate before being added to the page cache. What do 
you think?

~~~
Sashika:

Can this lockless check cause readers to receive spurious zeros instead
of the actual data?

Since filemap_get_folios_contig() fetches locklessly, a folio that is
currently being allocated and initialized, or swapped in by another
thread, might not yet be marked uptodate.

By checking folio_test_uptodate() without holding the folio lock, this falls
through to the zero-fill paths if the folio isn't uptodate yet. The original
code avoided this by using shmem_get_folio() which waits for the folio lock
and I/O completion.
~~~

link: 
https://sashiko.dev/#/patchset/20260520101538.58745-1-chizhiling@163.com

Thanks,