Re: [RFC PATCH 2/4] mm/shmem: use SGP_GET in read operations

[Chi Zhiling <chizhiling@163.com>]

On 5/18/26 14:39, Baolin Wang wrote:
> 
> 
> On 5/15/26 5:47 PM, Chi Zhiling wrote:
>> From: Chi Zhiling <chizhiling@kylinos.cn>
>>
>> Replace SGP_READ with SGP_GET in shmem_file_read_iter(),
>> shmem_file_splice_read(), and shmem_get_link(). These functions
>> immediately unlock the folio after getting it, making the lock
>> acquisition redundant.
>>
>> Even though folio_lock can protect folio data consistency or prevent
>> truncate while holding the lock, these can still happen after unlock.
>> Since these functions continue reading data after unlocking, the lock
>> does not provide effective protection. The folio reference count is
>> what actually prevents reclamation during access, making the lock
>> unnecessary.
>>
>> Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
>> ---
> 
> Thanks for your patch. It would be better to squash patch 1 and patch 2 
> into a one commit, so that the usage of the newly introduced flag can be 
> easily seen.

Hi, Baolin

Thanks for your review. I will squash patches 1 and 2 in the next 
version, unless v2 is unnecessary :D

> 
>>   mm/shmem.c | 12 +++---------
>>   1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index ef19968cc51c..767610f78d0d 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -3370,15 +3370,13 @@ 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);
>> +        error = shmem_get_folio(inode, index, 0, &folio, SGP_GET);
>>           if (error) {
>>               if (error == -EINVAL)
>>                   error = 0;
>>               break;
>>           }
>>           if (folio) {
>> -            folio_unlock(folio);
>> -
> 
> This is incorrect. Since a shmem folio can still be locked if it is 
> being swapped in from the swap device.

I don't know how I missed that, that's truly a serious bug. Thanks for 
pointing that out.

> 
> Essentially, you need to show reviewers the benefit of introducing a new 
> flag. In what scenario would you see overhead from locking the folio in 
> shmem_get_folio()? And how much performance improvement does your patch 
> bring? Data is the easiest way to convince people:)

Honestly, this may not bring significant performance improvements in 
single thread scenarios.

I think the main benefit is that during concurrent reads and writes, 
especially when large folio is enabled, there are more chances that 
reads and writes occur in the same folio. Removing the folio lock can 
prevent read operations from being blocked by write operations.

If necessary, I will include the test data in v2.

A more important reason is that folio batches are not suitable for 
storing locked folios, as this could lead to some issues.


Thanks,