Dave Chinner
2020-Feb-18 05:08 UTC
[Ocfs2-devel] [PATCH v6 04/19] mm: Rearrange readahead loop
On Mon, Feb 17, 2020 at 10:45:45AM -0800, Matthew Wilcox wrote:> From: "Matthew Wilcox (Oracle)" <willy at infradead.org> > > Move the declaration of 'page' to inside the loop and move the 'kick > off a fresh batch' code to the end of the function for easier use in > subsequent patches.Stale? the "kick off" code is moved to the tail of the loop, not the end of the function.> @@ -183,14 +183,14 @@ void __do_page_cache_readahead(struct address_space *mapping, > page = xa_load(&mapping->i_pages, page_offset); > if (page && !xa_is_value(page)) { > /* > - * Page already present? Kick off the current batch of > - * contiguous pages before continuing with the next > - * batch. > + * Page already present? Kick off the current batch > + * of contiguous pages before continuing with the > + * next batch. This page may be the one we would > + * have intended to mark as Readahead, but we don't > + * have a stable reference to this page, and it's > + * not worth getting one just for that. > */ > - if (readahead_count(&rac)) > - read_pages(&rac, &page_pool, gfp_mask); > - rac._nr_pages = 0; > - continue; > + goto read; > } > > page = __page_cache_alloc(gfp_mask); > @@ -201,6 +201,11 @@ void __do_page_cache_readahead(struct address_space *mapping, > if (page_idx == nr_to_read - lookahead_size) > SetPageReadahead(page); > rac._nr_pages++; > + continue; > +read: > + if (readahead_count(&rac)) > + read_pages(&rac, &page_pool, gfp_mask); > + rac._nr_pages = 0; > }Also, why? This adds a goto from branched code that continues, then adds a continue so the unbranched code doesn't execute the code the goto jumps to. In absence of any explanation, this isn't an improvement and doesn't make any sense... -- Dave Chinner david at fromorbit.com
Matthew Wilcox
2020-Feb-18 13:57 UTC
[Ocfs2-devel] [PATCH v6 04/19] mm: Rearrange readahead loop
On Tue, Feb 18, 2020 at 04:08:24PM +1100, Dave Chinner wrote:> On Mon, Feb 17, 2020 at 10:45:45AM -0800, Matthew Wilcox wrote: > > From: "Matthew Wilcox (Oracle)" <willy at infradead.org> > > > > Move the declaration of 'page' to inside the loop and move the 'kick > > off a fresh batch' code to the end of the function for easier use in > > subsequent patches. > > Stale? the "kick off" code is moved to the tail of the loop, not the > end of the function.Braino; I meant to write end of the loop.> > @@ -183,14 +183,14 @@ void __do_page_cache_readahead(struct address_space *mapping, > > page = xa_load(&mapping->i_pages, page_offset); > > if (page && !xa_is_value(page)) { > > /* > > - * Page already present? Kick off the current batch of > > - * contiguous pages before continuing with the next > > - * batch. > > + * Page already present? Kick off the current batch > > + * of contiguous pages before continuing with the > > + * next batch. This page may be the one we would > > + * have intended to mark as Readahead, but we don't > > + * have a stable reference to this page, and it's > > + * not worth getting one just for that. > > */ > > - if (readahead_count(&rac)) > > - read_pages(&rac, &page_pool, gfp_mask); > > - rac._nr_pages = 0; > > - continue; > > + goto read; > > } > > > > page = __page_cache_alloc(gfp_mask); > > @@ -201,6 +201,11 @@ void __do_page_cache_readahead(struct address_space *mapping, > > if (page_idx == nr_to_read - lookahead_size) > > SetPageReadahead(page); > > rac._nr_pages++; > > + continue; > > +read: > > + if (readahead_count(&rac)) > > + read_pages(&rac, &page_pool, gfp_mask); > > + rac._nr_pages = 0; > > } > > Also, why? This adds a goto from branched code that continues, then > adds a continue so the unbranched code doesn't execute the code the > goto jumps to. In absence of any explanation, this isn't an > improvement and doesn't make any sense...I thought I was explaining it ... "for easier use in subsequent patches".