[PATCH 0/2] mm: document read_pages and simplify usage

[PATCH 0/2] mm: document read_pages and simplify usage

[Frederick Mayle]

Add a kerneldoc for read_pages() to formalize an invariant and then use
it to simplify the callers in page_cache_ra_unbounded().

Frederick Mayle (2):
  mm/readahead: add kerneldoc for read_pages
  mm/readahead: simplify page_cache_ra_unbounded loop counter reset

 mm/readahead.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)


base-commit: 972c53e0ec3abfc6f5fe2cb503640710fb23cf95
-- 
2.54.0.563.g4f69b47b94-goog

[PATCH 1/2] mm/readahead: add kerneldoc for read_pages

[Frederick Mayle]

Formalize one of the invariants provided by the current implementation
so that callers can depend on it, as discussed in [1].

Link: https://lore.kernel.org/all/20260501061146.6e61392d125cf1847d7cc181@linux-foundation.org/ [1]
Signed-off-by: Frederick Mayle <fmayle@google.com>
---
 mm/readahead.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/mm/readahead.c b/mm/readahead.c
index 8c12b63ccd4a..23bec5497308 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -146,6 +146,17 @@ file_ra_state_init(struct file_ra_state *ra, struct address_space *mapping)
 }
 EXPORT_SYMBOL_GPL(file_ra_state_init);
 
+/**
+ * read_pages() - Start IO for a contiguous range of allocated folios in the
+ *                page cache.
+ * @rac: Readahead control.
+ *
+ * When read_pages() returns, it is guaranteed that all of the folios will have
+ * been processed or removed so that ``readahead_count(rac) == 0``. However,
+ * that does not imply that ``readahead_index(rac)`` will be updated to point
+ * to the end of the originally requested range because, for example, the
+ * filesystem may expand the range upwards.
+ */
 static void read_pages(struct readahead_control *rac)
 {
 	const struct address_space_operations *aops = rac->mapping->a_ops;
-- 
2.54.0.563.g4f69b47b94-goog

[PATCH 2/2] mm/readahead: simplify page_cache_ra_unbounded loop counter reset

[Frederick Mayle]

Minor cleanup, no behavior change intended.

`read_pages` ensures that `ractl->_nr_pages` is zero before it returns,
so the `ractl->_nr_pages` term in these expressions contributes nothing.
This seems to have been true since the statements were introduced in
commit f615bd5c4725f ("mm/readahead: Handle ractl nr_pages being
modified").

The new expression has an intuitive explanation. When filesystems
perform readahead, they increment `ractl->_index` by the number of pages
processed, so, after `read_pages` returns, `ractl->_index` points to the
first page after those already processed. `index` points to the first
page considered in the loop. So, `ractl->_index - index` is the number
of pages processed by the loop so far.

Signed-off-by: Frederick Mayle <fmayle@google.com>
---
 mm/readahead.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/readahead.c b/mm/readahead.c
index 23bec5497308..42f2f20633b0 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -281,7 +281,7 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 			 */
 			read_pages(ractl);
 			ractl->_index += min_nrpages;
-			i = ractl->_index + ractl->_nr_pages - index;
+			i = ractl->_index - index;
 			continue;
 		}
 
@@ -297,7 +297,7 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 				break;
 			read_pages(ractl);
 			ractl->_index += min_nrpages;
-			i = ractl->_index + ractl->_nr_pages - index;
+			i = ractl->_index - index;
 			continue;
 		}
 		if (i == mark)
-- 
2.54.0.563.g4f69b47b94-goog

Re: [PATCH 0/2] mm: document read_pages and simplify usage

[Frederick Mayle]

This is a combined v2 of the following patches. Sorry that I didn't send it as
such.


https://lore.kernel.org/r/20260508015402.735441-1-fmayle@google.com/
https://lore.kernel.org/r/20260501011908.3630802-1-fmayle@google.com/



On Tue, May 12, 2026 at 1:32 PM Frederick Mayle <fmayle@google.com> wrote:
>
> Add a kerneldoc for read_pages() to formalize an invariant and then use
> it to simplify the callers in page_cache_ra_unbounded().
>
> Frederick Mayle (2):
>   mm/readahead: add kerneldoc for read_pages
>   mm/readahead: simplify page_cache_ra_unbounded loop counter reset
>
>  mm/readahead.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
>
> base-commit: 972c53e0ec3abfc6f5fe2cb503640710fb23cf95
> --
> 2.54.0.563.g4f69b47b94-goog
>

Re: [PATCH 1/2] mm/readahead: add kerneldoc for read_pages

[Matthew Wilcox]

On Tue, May 12, 2026 at 01:31:35PM -0700, Frederick Mayle wrote:
> Formalize one of the invariants provided by the current implementation
> so that callers can depend on it, as discussed in [1].

We don't normally write kerneldoc for static functions.  I'm not sure
why Andrew told you to do this.  I have no objection to adding
documentation, but I don't think it should be marked as kerneldoc.
And if it's not kerneldoc, it doesn't have to be written in such a
stifling style.

Re: [PATCH 1/2] mm/readahead: add kerneldoc for read_pages

[Andrew Morton]

On Tue, 12 May 2026 22:05:34 +0100 Matthew Wilcox <willy@infradead.org> wrote:

> On Tue, May 12, 2026 at 01:31:35PM -0700, Frederick Mayle wrote:
> > Formalize one of the invariants provided by the current implementation
> > so that callers can depend on it, as discussed in [1].
> 
> We don't normally write kerneldoc for static functions.  I'm not sure
> why Andrew told you to do this.  I have no objection to adding
> documentation, but I don't think it should be marked as kerneldoc.
> And if it's not kerneldoc, it doesn't have to be written in such a
> stifling style.

I'm OK either way.  The main point here is that [2/2] relies on
unobvious and possibly fragile behaviour of read_pages().

Re: [PATCH 1/2] mm/readahead: add kerneldoc for read_pages

[Frederick Mayle]

On Tue, May 12, 2026 at 2:05 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, May 12, 2026 at 01:31:35PM -0700, Frederick Mayle wrote:
> > Formalize one of the invariants provided by the current implementation
> > so that callers can depend on it, as discussed in [1].
>
> We don't normally write kerneldoc for static functions.  I'm not sure
> why Andrew told you to do this.  I have no objection to adding
> documentation, but I don't think it should be marked as kerneldoc.
> And if it's not kerneldoc, it doesn't have to be written in such a
> stifling style.

If it isn't a kerneldoc, I can delete the essentially redundant argument
line. Besides that, I think the stifling style might just be me. Let me
know if there is something specific you don't like.