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

[Baolin Wang]

On 5/18/26 3:42 PM, Chi Zhiling wrote:
> On 5/18/26 14:39, Baolin Wang wrote:
> > 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.
>
> 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.

If this new flag is necessary, please add these explanations to the 
commit message.

However, if there's no obvious performance improvement, I'd prefer not 
to add an unnecessary flag. Also, looking at the implementation of 
shmem_get_folio_from_batch() in patch 3, couldn't we unlock the folio 
after shmem_get_folio()?