[RFC PATCH 0/3] Tiny optimization for large read operations

[RFC PATCH 0/3] Tiny optimization for large read operations

[Chi Zhiling]

From: Chi Zhiling <chizhiling@kylinos.cn>

Fine-Tuned Optimization for Large IO Read Operations

When reading data exceeding the maximum IO size, the operation is split into multiple IO requests, but the data isn't immediately copied to user space after IO completion. 
For example, when reading 2560k data from a device with 1280k maximum IO size, the following sequence occurs:

1. read 1280k
2. copy 31 pages to user buffer
3. copy 10 pages and issue read ahead for next 1280k
4. copy 31 pages to user buffer
5. wait the next 1280k
6. copy 8 pages to user buffer
7. copy 20 folios(64k) to user buffer

The 8 pages in step 6 are copied after the second 1280k completes due to waiting for non-uptodate folio in filemap_update_page.
After applying the patch, these 8 pages will be copied before the next IO completes:

1. read 1280k
2. copy 31 pages to user buffer
3. copy 10 pages and issue read ahead for next 1280k
4. copy 31 pages to user buffer
5. copy 8 pages to user buffer
6. wait the next 1280k
7. copy 20 folios(64k) to user buffer

Chi Zhiling (3):
  mm/filemap: Do not use is_partially_uptodate for entire folio
  mm/filemap: Avoid modifying iocb->ki_flags for AIO in
    filemap_get_pages()
  mm/filemap: Skip non-uptodate folio when folios are available

 mm/filemap.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

-- 
2.43.0

[RFC PATCH 1/3] mm/filemap: Do not use is_partially_uptodate for entire folio

[Chi Zhiling]

From: Chi Zhiling <chizhiling@kylinos.cn>

When reading the entire folio, we should not use is_partially_uptodate()
to determine if the folio is up-to-date. If the folio is not marked as
uptodate, then it is not safe to treat the entire folio as up-to-date,
even if the partial check returns true.

If all data in a folio is actually up-to-date but the folio lacks the
uptodate flag, this could cause issues during pipe reads

This change adds a condition to skip the partial check for the entire folio
case, ensuring that we only consider a folio as up-to-date for the entire
range if it is marked as uptodate.

Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
---
 mm/filemap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/filemap.c b/mm/filemap.c
index bada249b9fb7..af12d1cecc7d 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2446,6 +2446,9 @@ static bool filemap_range_uptodate(struct address_space *mapping,
 		pos -= folio_pos(folio);
 	}
 
+	if (pos == 0 && count >= folio_size(folio))
+		return false;
+
 	return mapping->a_ops->is_partially_uptodate(folio, pos, count);
 }
 
-- 
2.43.0

[RFC PATCH 2/3] mm/filemap: Avoid modifying iocb->ki_flags for AIO in filemap_get_pages()

[Chi Zhiling]

From: Chi Zhiling <chizhiling@kylinos.cn>

Setting IOCB_NOWAIT in filemap_get_pages() for AIO is only used to
indicate not to block in the filemap_update_page(), with no other purpose.
Moreover, in filemap_read(), IOCB_NOWAIT will be set again for AIO.

Therefore, adding a parameter to the filemap_update_page function to
explicitly indicate not to block serves the same purpose as indicating
through iocb->ki_flags, thus avoiding modifications to iocb->ki_flags.

This patch does not change the original logic and is preparation for the
next patch.

Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
---
 mm/filemap.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index af12d1cecc7d..350080f579ef 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2454,11 +2454,15 @@ static bool filemap_range_uptodate(struct address_space *mapping,
 
 static int filemap_update_page(struct kiocb *iocb,
 		struct address_space *mapping, size_t count,
-		struct folio *folio, bool need_uptodate)
+		struct folio *folio, bool need_uptodate,
+		bool no_wait)
 {
 	int error;
+	int flags = iocb->ki_flags;
+	if (no_wait)
+		flags |= IOCB_NOWAIT;
 
-	if (iocb->ki_flags & IOCB_NOWAIT) {
+	if (flags & IOCB_NOWAIT) {
 		if (!filemap_invalidate_trylock_shared(mapping))
 			return -EAGAIN;
 	} else {
@@ -2467,9 +2471,9 @@ static int filemap_update_page(struct kiocb *iocb,
 
 	if (!folio_trylock(folio)) {
 		error = -EAGAIN;
-		if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_NOIO))
+		if (flags & (IOCB_NOWAIT | IOCB_NOIO))
 			goto unlock_mapping;
-		if (!(iocb->ki_flags & IOCB_WAITQ)) {
+		if (!(flags & IOCB_WAITQ)) {
 			filemap_invalidate_unlock_shared(mapping);
 			/*
 			 * This is where we usually end up waiting for a
@@ -2493,7 +2497,7 @@ static int filemap_update_page(struct kiocb *iocb,
 		goto unlock;
 
 	error = -EAGAIN;
-	if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ))
+	if (flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ))
 		goto unlock;
 
 	error = filemap_read_folio(iocb->ki_filp, mapping->a_ops->read_folio,
@@ -2621,11 +2625,13 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
 			goto err;
 	}
 	if (!folio_test_uptodate(folio)) {
+		bool no_wait = false;
+
 		if ((iocb->ki_flags & IOCB_WAITQ) &&
 		    folio_batch_count(fbatch) > 1)
-			iocb->ki_flags |= IOCB_NOWAIT;
+			no_wait = true;
 		err = filemap_update_page(iocb, mapping, count, folio,
-					  need_uptodate);
+					  need_uptodate, no_wait);
 		if (err)
 			goto err;
 	}
-- 
2.43.0

[RFC PATCH 3/3] mm/filemap: Skip non-uptodate folio when folios are available

[Chi Zhiling]

From: Chi Zhiling <chizhiling@kylinos.cn>

This optimization primarily targets read operations that trigger multiple
IOs, aiming to complete the copy from cache to user buffer as quickly as
possible after the final IO completes.

This patch achieves the goal by minimizing the number of folios left for
the final copy loop.

In filemap_get_pages(), when encountering a non-uptodate folio while the
fbatch already contains folios, we skip waiting for the non-uptodate folio
and proceed to copy the available folios.

Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
---
 mm/filemap.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 350080f579ef..894584a5bff5 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2625,13 +2625,9 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
 			goto err;
 	}
 	if (!folio_test_uptodate(folio)) {
-		bool no_wait = false;
-
-		if ((iocb->ki_flags & IOCB_WAITQ) &&
-		    folio_batch_count(fbatch) > 1)
-			no_wait = true;
 		err = filemap_update_page(iocb, mapping, count, folio,
-					  need_uptodate, no_wait);
+					  need_uptodate,
+					  folio_batch_count(fbatch) > 1);
 		if (err)
 			goto err;
 	}
-- 
2.43.0

Re: [RFC PATCH 2/3] mm/filemap: Avoid modifying iocb->ki_flags for AIO in filemap_get_pages()

[Matthew Wilcox]

On Wed, Jul 23, 2025 at 06:18:24PM +0800, Chi Zhiling wrote:
> From: Chi Zhiling <chizhiling@kylinos.cn>
> 
> Setting IOCB_NOWAIT in filemap_get_pages() for AIO is only used to
> indicate not to block in the filemap_update_page(), with no other purpose.
> Moreover, in filemap_read(), IOCB_NOWAIT will be set again for AIO.
> 
> Therefore, adding a parameter to the filemap_update_page function to
> explicitly indicate not to block serves the same purpose as indicating
> through iocb->ki_flags, thus avoiding modifications to iocb->ki_flags.
> 
> This patch does not change the original logic and is preparation for the
> next patch.

Passing multiple booleans to a function is an antipattern.
Particularly in this case, since we could just pass iocb->ki_flags
to the function.

But I think there's a less complicated way to do what you want.
Just don't call filemap_update_page() if there are uptodate folios
in the batch:

+++ b/mm/filemap.c
@@ -2616,9 +2616,10 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
                        goto err;
        }
        if (!folio_test_uptodate(folio)) {
-               if ((iocb->ki_flags & IOCB_WAITQ) &&
-                   folio_batch_count(fbatch) > 1)
-                       iocb->ki_flags |= IOCB_NOWAIT;
+               if (folio_batch_count(fbatch) > 1) {
+                       err = -EAGAIN;
+                       goto err;
+               }
                err = filemap_update_page(iocb, mapping, count, folio,
                                          need_uptodate);
                if (err)

Re: [RFC PATCH 2/3] mm/filemap: Avoid modifying iocb->ki_flags for AIO in filemap_get_pages()

[Chi Zhiling]

On 2025/7/23 22:33, Matthew Wilcox wrote:
> On Wed, Jul 23, 2025 at 06:18:24PM +0800, Chi Zhiling wrote:
>> From: Chi Zhiling <chizhiling@kylinos.cn>
>>
>> Setting IOCB_NOWAIT in filemap_get_pages() for AIO is only used to
>> indicate not to block in the filemap_update_page(), with no other purpose.
>> Moreover, in filemap_read(), IOCB_NOWAIT will be set again for AIO.
>>
>> Therefore, adding a parameter to the filemap_update_page function to
>> explicitly indicate not to block serves the same purpose as indicating
>> through iocb->ki_flags, thus avoiding modifications to iocb->ki_flags.
>>
>> This patch does not change the original logic and is preparation for the
>> next patch.
> 
> Passing multiple booleans to a function is an antipattern.
> Particularly in this case, since we could just pass iocb->ki_flags
> to the function.
> 
> But I think there's a less complicated way to do what you want.
> Just don't call filemap_update_page() if there are uptodate folios
> in the batch:
> 
> +++ b/mm/filemap.c
> @@ -2616,9 +2616,10 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
>                          goto err;
>          }
>          if (!folio_test_uptodate(folio)) {
> -               if ((iocb->ki_flags & IOCB_WAITQ) &&
> -                   folio_batch_count(fbatch) > 1)
> -                       iocb->ki_flags |= IOCB_NOWAIT;
> +               if (folio_batch_count(fbatch) > 1) {
> +                       err = -EAGAIN;
> +                       goto err;
> +               }


Yes, this is a completely better way.


Would you mind if I add
"Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>"
in the next version?

>                  err = filemap_update_page(iocb, mapping, count, folio,
>                                            need_uptodate);
>                  if (err)

Thanks,