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

[Chi Zhiling <chizhiling@163.com>]

On 6/1/26 7:20 PM, Baolin Wang wrote:
> 
> 
> On 5/26/26 3:12 PM, Chi Zhiling wrote:
>> On 5/26/26 11:11 AM, Baolin Wang wrote:
>>>
>>>
>>> On 5/25/26 5:47 PM, Chi Zhiling wrote:
>>>> 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.
>>>
>>> Good.
>>>
>>>> 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?
>>>
>>> I'm not concerned about swap-in folios, since we already ensure the 
>>> folio is uptodate when swapping out.
>>>
>>> What I'm worried about is that this change moves the 
>>> folio_test_uptodate() check outside of the folio lock, which could 
>>> introduce some race? Of course, with the original logic, the folio 
>>> state could also transition from !uptodate to uptodate after the folio
>>> is unlocked (I need more thought).
>>>
>>> Another race condition that needs to be addressed is, when getting 
>>> the folio locklessly via filemap_get_folios_contig(), there could be 
>>> a concurrent race with swap-out (because you removed the folio- 
>>> >mapping check under folio lock). This could result in reading zero 
>>> data when it shouldn't (the folio actually needs to be swapped in).
>>
>> Sorry, I don't fully understand what you mean. It seems that swap-out 
>> does not clear the uptodate flag, and the folio cannot be freed during 
>> the read operation because its reference count is held. So a swapped- 
>> out folio should not be mistaken for a zero-filled folio. Right?
> 
> Sorry for the late reply. Ah, I missed the held refcnt, and now sounds 
> reasonable.

I've just sent out v2. Feel free to comment. :)

https://lore.kernel.org/linux-fsdevel/20260601055704.167436-1-chizhiling@163.com/