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

[Baolin Wang]

On 5/15/26 5:47 PM, Chi Zhiling wrote:
> From: Chi Zhiling <chizhiling@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---

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.

>   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.

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:)