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

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

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.