Message ID | 1603375114-58419-1-git-send-email-haoxu@linux.alibaba.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [RFC] mm: fix the sync buffered read to the old way | expand |
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.
在 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? >
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;
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(-)