| Message ID | 20200217184613.19668-8-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Change readahead API | expand |
On Mon, Feb 17, 2020 at 10:45:48AM -0800, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > Eliminate the page_offset variable which was confusing with the > 'offset' parameter and record the start of each consecutive run of > pages in the readahead_control. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > mm/readahead.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) Looks ok, but having the readahead dispatch out of line from the case that triggers it makes it hard to follow. Cheers, Dave.
On 2/17/20 10:45 AM, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > Eliminate the page_offset variable which was confusing with the > 'offset' parameter and record the start of each consecutive run of > pages in the readahead_control. ...presumably for the benefit of a subsequent patch, since it's not consumed in this patch. Thanks for breaking these up, btw, it really helps. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > mm/readahead.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/mm/readahead.c b/mm/readahead.c > index 3eca59c43a45..74791b96013f 100644 > --- a/mm/readahead.c > +++ b/mm/readahead.c > @@ -162,6 +162,7 @@ void __do_page_cache_readahead(struct address_space *mapping, > struct readahead_control rac = { > .mapping = mapping, > .file = filp, > + ._start = offset, > ._nr_pages = 0, > }; > > @@ -175,12 +176,11 @@ void __do_page_cache_readahead(struct address_space *mapping, > */ > for (page_idx = 0; page_idx < nr_to_read; page_idx++) { > struct page *page; > - pgoff_t page_offset = offset + page_idx; OK, this is still something I want to mention (I wrote the same thing when reviewing the wrong version of this patch, a moment ago). You know...this ends up incrementing offset each time through the loop, so yes, the behavior is the same as when using "offset + page_idx". However, now it's a little harder to see that. IMHO the page_offset variable is not actually a bad thing, here. I'd rather keep it, all other things being equal (and I don't see any other benefits here: line count is about the same, for example). What do you think? (I don't feel strongly about this fine point.) thanks,
diff --git a/mm/readahead.c b/mm/readahead.c index 3eca59c43a45..74791b96013f 100644 --- a/mm/readahead.c +++ b/mm/readahead.c @@ -162,6 +162,7 @@ void __do_page_cache_readahead(struct address_space *mapping, struct readahead_control rac = { .mapping = mapping, .file = filp, + ._start = offset, ._nr_pages = 0, }; @@ -175,12 +176,11 @@ void __do_page_cache_readahead(struct address_space *mapping, */ for (page_idx = 0; page_idx < nr_to_read; page_idx++) { struct page *page; - pgoff_t page_offset = offset + page_idx; - if (page_offset > end_index) + if (offset > end_index) break; - page = xa_load(&mapping->i_pages, page_offset); + page = xa_load(&mapping->i_pages, offset); if (page && !xa_is_value(page)) { /* * Page already present? Kick off the current batch @@ -196,16 +196,18 @@ void __do_page_cache_readahead(struct address_space *mapping, page = __page_cache_alloc(gfp_mask); if (!page) break; - page->index = page_offset; + page->index = offset; list_add(&page->lru, &page_pool); if (page_idx == nr_to_read - lookahead_size) SetPageReadahead(page); rac._nr_pages++; + offset++; continue; read: if (readahead_count(&rac)) read_pages(&rac, &page_pool, gfp_mask); rac._nr_pages = 0; + rac._start = ++offset; } /*