[PATCH RFC] mm: fix the sync buffered read to the old way

[PATCH RFC] mm: fix the sync buffered read to the old way

[Hao Xu]

The commit 324bcf54c449 changed the code path of async buffered reads
to go with the page_cache_sync_readahead() way when readahead is
disabled, meanwhile the sync buffered reads are forced to do IO in the
above way as well, which makes it go to a more complex code path.

Fixes: 324bcf54c449 ("mm: use limited read-ahead to satisfy read")
Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---

Hi Jens,
I see it from the commit 324bcf54c449 ("mm: use limited read-ahead to
satisfy read") that we have forced normal sync buffered reads go with
the page_cache_sync_readahead() when readahead is disabled. I'm not
sure if this is what you expected. Here I changed the sync buffered
reads to go with the old code path(a_ops->readpage()), and tested the
performance of them, the results of IOPS and cpu time are similar. I
need your opinion on this.

 mm/filemap.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index e4101b5bfa82..0b2a0f633c01 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2224,9 +2224,14 @@ ssize_t generic_file_buffered_read(struct kiocb *iocb,
 		if (!page) {
 			if (iocb->ki_flags & IOCB_NOIO)
 				goto would_block;
-			page_cache_sync_readahead(mapping,
-					ra, filp,
-					index, last_index - index);
+			/*
+			 * when readahead is disabled and IOCB_WAITQ isn't set
+			 * we should go with the readpage() way.
+			 */
+			if (ra->ra_pages || (iocb->ki_flags & IOCB_WAITQ))
+				page_cache_sync_readahead(mapping,
+						ra, filp,
+						index, last_index - index);
 			page = find_get_page(mapping, index);
 			if (unlikely(page == NULL))
 				goto no_cached_page;
-- 
1.8.3.1

Re: [PATCH RFC] mm: fix the sync buffered read to the old way

[Matthew Wilcox]

On Thu, Oct 22, 2020 at 09:58:34PM +0800, Hao Xu wrote:
> The commit 324bcf54c449 changed the code path of async buffered reads
> to go with the page_cache_sync_readahead() way when readahead is
> disabled, meanwhile the sync buffered reads are forced to do IO in the
> above way as well, which makes it go to a more complex code path.

But ->readpage is (increasingly) synchronous while readahead should be
used to start async I/Os.  I'm pretty sure Jens meant to do exactly what
he did.

Re: [PATCH RFC] mm: fix the sync buffered read to the old way

[Hao_Xu]

在 2020/10/22 下午10:10, Matthew Wilcox 写道:
> On Thu, Oct 22, 2020 at 09:58:34PM +0800, Hao Xu wrote:
>> The commit 324bcf54c449 changed the code path of async buffered reads
>> to go with the page_cache_sync_readahead() way when readahead is
>> disabled, meanwhile the sync buffered reads are forced to do IO in the
>> above way as well, which makes it go to a more complex code path.
> 
> But ->readpage is (increasingly) synchronous while readahead should be
> used to start async I/Os.  I'm pretty sure Jens meant to do exactly what
> he did.
Yes, we should start async I/Os with readahead, but why should we do the 
sync I/O like syscall read() in this way too when ra->ra_pages is 0?
>