| Message ID | 164879346851.25542.18299715584610241983@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | MM: minor improvements to readahead documentation | expand |
On Fri, Apr 01, 2022 at 05:11:08PM +1100, NeilBrown wrote: > > - use "readahead" consistently - not "read-ahead" or "read ahead". > - clarify some sections that, on reflection, weren't very clear > - minor punctuation/spelling fixes. Coincidentally, I had a number of other changes to this documentation which conflicted throughout with yours. Here's a merge of the two: @@ -13,29 +13,29 @@ * * Readahead is used to read content into the page cache before it is * explicitly requested by the application. Readahead only ever - * attempts to read pages that are not yet in the page cache. If a - * page is present but not up-to-date, readahead will not try to read + * attempts to read folios that are not yet in the page cache. If a + * folio is present but not up-to-date, readahead will not try to read * it. In that case a simple ->readpage() will be requested. * * Readahead is triggered when an application read request (whether a - * systemcall or a page fault) finds that the requested page is not in + * system call or a page fault) finds that the requested folio is not in * the page cache, or that it is in the page cache and has the - * %PG_readahead flag set. This flag indicates that the page was loaded - * as part of a previous read-ahead request and now that it has been - * accessed, it is time for the next read-ahead. + * readahead flag set. This flag indicates that the folio was read + * as part of a previous readahead request and now that it has been + * accessed, it is time for the next readahead. * * Each readahead request is partly synchronous read, and partly async - * read-ahead. This is reflected in the struct file_ra_state which - * contains ->size being to total number of pages, and ->async_size - * which is the number of pages in the async section. The first page in - * this async section will have %PG_readahead set as a trigger for a - * subsequent read ahead. Once a series of sequential reads has been + * readahead. This is reflected in the struct file_ra_state which + * contains ->size being the total number of pages, and ->async_size + * which is the number of pages in the async section. The readahead + * flag will be set on the first folio in this async section to trigger + * a subsequent readahead. Once a series of sequential reads has been * established, there should be no need for a synchronous component and - * all read ahead request will be fully asynchronous. + * all readahead request will be fully asynchronous. * - * When either of the triggers causes a readahead, three numbers need to - * be determined: the start of the region, the size of the region, and - * the size of the async tail. + * When either of the triggers causes a readahead, three numbers need + * to be determined: the start of the region to read, the size of the + * region, and the size of the async tail. * * The start of the region is simply the first page address at or after * the accessed address, which is not currently populated in the page @@ -45,14 +45,14 @@ * was explicitly requested from the determined request size, unless * this would be less than zero - then zero is used. NOTE THIS * CALCULATION IS WRONG WHEN THE START OF THE REGION IS NOT THE ACCESSED - * PAGE. + * PAGE. ALSO THIS CALCULATION IS NOT USED CONSISTENTLY. * * The size of the region is normally determined from the size of the * previous readahead which loaded the preceding pages. This may be * discovered from the struct file_ra_state for simple sequential reads, * or from examining the state of the page cache when multiple * sequential reads are interleaved. Specifically: where the readahead - * was triggered by the %PG_readahead flag, the size of the previous + * was triggered by the readahead flag, the size of the previous * readahead is assumed to be the number of pages from the triggering * page to the start of the new readahead. In these cases, the size of * the previous readahead is scaled, often doubled, for the new @@ -65,52 +65,52 @@ * larger than the current request, and it is not scaled up, unless it * is at the start of file. * - * In general read ahead is accelerated at the start of the file, as + * In general readahead is accelerated at the start of the file, as * reads from there are often sequential. There are other minor - * adjustments to the read ahead size in various special cases and these + * adjustments to the readahead size in various special cases and these * are best discovered by reading the code. * - * The above calculation determines the readahead, to which any requested - * read size may be added. + * The above calculation, based on the previous readahead size, + * determines the size of the readahead, to which any requested read + * size may be added. * * Readahead requests are sent to the filesystem using the ->readahead() * address space operation, for which mpage_readahead() is a canonical * implementation. ->readahead() should normally initiate reads on all - * pages, but may fail to read any or all pages without causing an IO + * folios, but may fail to read any or all folios without causing an I/O * error. The page cache reading code will issue a ->readpage() request - * for any page which ->readahead() does not provided, and only an error + * for any folio which ->readahead() did not read, and only an error * from this will be final. * - * ->readahead() will generally call readahead_page() repeatedly to get - * each page from those prepared for read ahead. It may fail to read a - * page by: + * ->readahead() will generally call readahead_folio() repeatedly to get + * each folio from those prepared for readahead. It may fail to read a + * folio by: * - * * not calling readahead_page() sufficiently many times, effectively - * ignoring some pages, as might be appropriate if the path to + * * not calling readahead_folio() sufficiently many times, effectively + * ignoring some folios, as might be appropriate if the path to * storage is congested. * - * * failing to actually submit a read request for a given page, + * * failing to actually submit a read request for a given folio, * possibly due to insufficient resources, or * * * getting an error during subsequent processing of a request. * - * In the last two cases, the page should be unlocked to indicate that - * the read attempt has failed. In the first case the page will be - * unlocked by the caller. + * In the last two cases, the folio should be unlocked by the filesystem + * to indicate that the read attempt has failed. In the first case the + * folio will be unlocked by the VFS. * - * Those pages not in the final ``async_size`` of the request should be + * Those folios not in the final ``async_size`` of the request should be * considered to be important and ->readahead() should not fail them due * to congestion or temporary resource unavailability, but should wait * for necessary resources (e.g. memory or indexing information) to - * become available. Pages in the final ``async_size`` may be + * become available. Folios in the final ``async_size`` may be * considered less urgent and failure to read them is more acceptable. - * In this case it is best to use delete_from_page_cache() to remove the - * pages from the page cache as is automatically done for pages that - * were not fetched with readahead_page(). This will allow a - * subsequent synchronous read ahead request to try them again. If they + * In this case it is best to use filemap_remove_folio() to remove the + * folios from the page cache as is automatically done for folios that + * were not fetched with readahead_folio(). This will allow a + * subsequent synchronous readahead request to try them again. If they * are left in the page cache, then they will be read individually using - * ->readpage(). - * + * ->readpage() which may be less efficient. */ #include <linux/kernel.h> @@ -157,7 +157,7 @@ static void read_pages(struct readahead_control *rac) aops->readahead(rac); /* * Clean up the remaining pages. The sizes in ->ra - * maybe be used to size next read-ahead, so make sure + * maybe be used to size next readahead, so make sure * they accurately reflect what happened. */ while ((page = readahead_page(rac))) { @@ -420,7 +420,7 @@ static pgoff_t count_history_pages(struct address_space *mapping, } /* - * page cache context based read-ahead + * page cache context based readahead */ static int try_context_readahead(struct address_space *mapping, struct file_ra_state *ra, @@ -671,9 +671,9 @@ void page_cache_sync_ra(struct readahead_control *ractl, bool do_forced_ra = ractl->file && (ractl->file->f_mode & FMODE_RANDOM); /* - * Even if read-ahead is disabled, issue this request as read-ahead + * Even if readahead is disabled, issue this request as readahead * as we'll need it to satisfy the requested range. The forced - * read-ahead will do the right thing and limit the read to just the + * readahead will do the right thing and limit the read to just the * requested range, which we'll set to 1 page for this case. */ if (!ractl->ra->ra_pages || blk_cgroup_congested()) { @@ -689,7 +689,6 @@ void page_cache_sync_ra(struct readahead_control *ractl, return; } - /* do read-ahead */ ondemand_readahead(ractl, NULL, req_count); } EXPORT_SYMBOL_GPL(page_cache_sync_ra); @@ -697,7 +696,7 @@ EXPORT_SYMBOL_GPL(page_cache_sync_ra); void page_cache_async_ra(struct readahead_control *ractl, struct folio *folio, unsigned long req_count) { - /* no read-ahead */ + /* no readahead */ if (!ractl->ra->ra_pages) return; @@ -712,7 +711,6 @@ void page_cache_async_ra(struct readahead_control *ractl, if (blk_cgroup_congested()) return; - /* do read-ahead */ ondemand_readahead(ractl, folio, req_count); } EXPORT_SYMBOL_GPL(page_cache_async_ra);
On Sat, 02 Apr 2022, Matthew Wilcox wrote: > On Fri, Apr 01, 2022 at 05:11:08PM +1100, NeilBrown wrote: > > > > - use "readahead" consistently - not "read-ahead" or "read ahead". > > - clarify some sections that, on reflection, weren't very clear > > - minor punctuation/spelling fixes. > > Coincidentally, I had a number of other changes to this documentation > which conflicted throughout with yours. Here's a merge of the two: Thanks. > > @@ -13,29 +13,29 @@ > * > * Readahead is used to read content into the page cache before it is > * explicitly requested by the application. Readahead only ever > - * attempts to read pages that are not yet in the page cache. If a > - * page is present but not up-to-date, readahead will not try to read > + * attempts to read folios that are not yet in the page cache. If a > + * folio is present but not up-to-date, readahead will not try to read > * it. In that case a simple ->readpage() will be requested. > * > * Readahead is triggered when an application read request (whether a > - * systemcall or a page fault) finds that the requested page is not in > + * system call or a page fault) finds that the requested folio is not in > * the page cache, or that it is in the page cache and has the > - * %PG_readahead flag set. This flag indicates that the page was loaded > - * as part of a previous read-ahead request and now that it has been > - * accessed, it is time for the next read-ahead. > + * readahead flag set. This flag indicates that the folio was read Ugh. Why don't you like %PG_readahead? I absolutely loath the practice of hiding flags inside accessor functions, and hiding the truth in documentation is just as bad. It all makes grepping that much harder. I would MUCH prefer that the %PG_ were restored. Please. > + * as part of a previous readahead request and now that it has been > + * accessed, it is time for the next readahead. > * > * Each readahead request is partly synchronous read, and partly async > - * read-ahead. This is reflected in the struct file_ra_state which > - * contains ->size being to total number of pages, and ->async_size > - * which is the number of pages in the async section. The first page in > - * this async section will have %PG_readahead set as a trigger for a > - * subsequent read ahead. Once a series of sequential reads has been > + * readahead. This is reflected in the struct file_ra_state which > + * contains ->size being the total number of pages, and ->async_size > + * which is the number of pages in the async section. The readahead > + * flag will be set on the first folio in this async section to trigger > + * a subsequent readahead. Once a series of sequential reads has been > * established, there should be no need for a synchronous component and > - * all read ahead request will be fully asynchronous. > + * all readahead request will be fully asynchronous. > * > - * When either of the triggers causes a readahead, three numbers need to > - * be determined: the start of the region, the size of the region, and > - * the size of the async tail. > + * When either of the triggers causes a readahead, three numbers need > + * to be determined: the start of the region to read, the size of the > + * region, and the size of the async tail. > * > * The start of the region is simply the first page address at or after > * the accessed address, which is not currently populated in the page > @@ -45,14 +45,14 @@ > * was explicitly requested from the determined request size, unless > * this would be less than zero - then zero is used. NOTE THIS > * CALCULATION IS WRONG WHEN THE START OF THE REGION IS NOT THE ACCESSED > - * PAGE. > + * PAGE. ALSO THIS CALCULATION IS NOT USED CONSISTENTLY. > * > * The size of the region is normally determined from the size of the > * previous readahead which loaded the preceding pages. This may be > * discovered from the struct file_ra_state for simple sequential reads, > * or from examining the state of the page cache when multiple > * sequential reads are interleaved. Specifically: where the readahead > - * was triggered by the %PG_readahead flag, the size of the previous > + * was triggered by the readahead flag, the size of the previous > * readahead is assumed to be the number of pages from the triggering > * page to the start of the new readahead. In these cases, the size of > * the previous readahead is scaled, often doubled, for the new > @@ -65,52 +65,52 @@ > * larger than the current request, and it is not scaled up, unless it > * is at the start of file. > * > - * In general read ahead is accelerated at the start of the file, as > + * In general readahead is accelerated at the start of the file, as > * reads from there are often sequential. There are other minor > - * adjustments to the read ahead size in various special cases and these > + * adjustments to the readahead size in various special cases and these > * are best discovered by reading the code. > * > - * The above calculation determines the readahead, to which any requested > - * read size may be added. > + * The above calculation, based on the previous readahead size, > + * determines the size of the readahead, to which any requested read > + * size may be added. > * > * Readahead requests are sent to the filesystem using the ->readahead() > * address space operation, for which mpage_readahead() is a canonical > * implementation. ->readahead() should normally initiate reads on all > - * pages, but may fail to read any or all pages without causing an IO > + * folios, but may fail to read any or all folios without causing an I/O > * error. The page cache reading code will issue a ->readpage() request > - * for any page which ->readahead() does not provided, and only an error > + * for any folio which ->readahead() did not read, and only an error > * from this will be final. > * > - * ->readahead() will generally call readahead_page() repeatedly to get > - * each page from those prepared for read ahead. It may fail to read a > - * page by: > + * ->readahead() will generally call readahead_folio() repeatedly to get > + * each folio from those prepared for readahead. It may fail to read a > + * folio by: > * > - * * not calling readahead_page() sufficiently many times, effectively > - * ignoring some pages, as might be appropriate if the path to > + * * not calling readahead_folio() sufficiently many times, effectively > + * ignoring some folios, as might be appropriate if the path to > * storage is congested. > * > - * * failing to actually submit a read request for a given page, > + * * failing to actually submit a read request for a given folio, > * possibly due to insufficient resources, or > * > * * getting an error during subsequent processing of a request. > * > - * In the last two cases, the page should be unlocked to indicate that > - * the read attempt has failed. In the first case the page will be > - * unlocked by the caller. > + * In the last two cases, the folio should be unlocked by the filesystem > + * to indicate that the read attempt has failed. In the first case the > + * folio will be unlocked by the VFS. VFS?? The code is in mm/readahead.c, not in fs/*.c Why didn't you like "caller" ?? Thanks, NeilBrown
On Mon, Apr 04, 2022 at 02:10:51PM +1000, NeilBrown wrote: > > * Readahead is triggered when an application read request (whether a > > - * systemcall or a page fault) finds that the requested page is not in > > + * system call or a page fault) finds that the requested folio is not in > > * the page cache, or that it is in the page cache and has the > > - * %PG_readahead flag set. This flag indicates that the page was loaded > > - * as part of a previous read-ahead request and now that it has been > > - * accessed, it is time for the next read-ahead. > > + * readahead flag set. This flag indicates that the folio was read > > Ugh. Why don't you like %PG_readahead? I absolutely loath the > practice of hiding flags inside accessor functions, and hiding the truth > in documentation is just as bad. It all makes grepping that much > harder. > I would MUCH prefer that the %PG_ were restored. Please. I absolutely loathe it that there are references to PG_* anywhere outside page-flags.h. We have the abstraction layer, we want people to use it, and we shouldn't needlessly multiply entities by referring to the implementation of the abstraction. I remove references to PG_ flags wherever I find them. I agree that grepping for page/folio flags doesn't work, and it's something I spend a lot of time thinking about. In particular, I want to produce decent kernel-doc for them. > > - * In the last two cases, the page should be unlocked to indicate that > > - * the read attempt has failed. In the first case the page will be > > - * unlocked by the caller. > > + * In the last two cases, the folio should be unlocked by the filesystem > > + * to indicate that the read attempt has failed. In the first case the > > + * folio will be unlocked by the VFS. > > VFS?? The code is in mm/readahead.c, not in fs/*.c > Why didn't you like "caller" ?? I view mm/readahead.c, mm/filemap.c and mm/page-writeback.c as part of the VFS more than as part of the VM. But that's something that reasonable people can disagree on. I think from the point of view of the filesystem author, it's all VFS.
On Mon, 04 Apr 2022, Matthew Wilcox wrote: > On Mon, Apr 04, 2022 at 02:10:51PM +1000, NeilBrown wrote: > > > * Readahead is triggered when an application read request (whether a > > > - * systemcall or a page fault) finds that the requested page is not in > > > + * system call or a page fault) finds that the requested folio is not in > > > * the page cache, or that it is in the page cache and has the > > > - * %PG_readahead flag set. This flag indicates that the page was loaded > > > - * as part of a previous read-ahead request and now that it has been > > > - * accessed, it is time for the next read-ahead. > > > + * readahead flag set. This flag indicates that the folio was read > > > > Ugh. Why don't you like %PG_readahead? I absolutely loath the > > practice of hiding flags inside accessor functions, and hiding the truth > > in documentation is just as bad. It all makes grepping that much > > harder. > > I would MUCH prefer that the %PG_ were restored. Please. > > I absolutely loathe it that there are references to PG_* anywhere > outside page-flags.h. We have the abstraction layer, we want people > to use it, and we shouldn't needlessly multiply entities by referring > to the implementation of the abstraction. I remove references to PG_ > flags wherever I find them. I agree that grepping for page/folio flags > doesn't work, and it's something I spend a lot of time thinking about. > In particular, I want to produce decent kernel-doc for them. Yes, we have an abstraction layer - but WHY do you have an abstraction layer? I can't see that it adds anything other than obfuscation. Do you WANT to keep the learning curve nice and steep? > > > > - * In the last two cases, the page should be unlocked to indicate that > > > - * the read attempt has failed. In the first case the page will be > > > - * unlocked by the caller. > > > + * In the last two cases, the folio should be unlocked by the filesystem > > > + * to indicate that the read attempt has failed. In the first case the > > > + * folio will be unlocked by the VFS. > > > > VFS?? The code is in mm/readahead.c, not in fs/*.c > > Why didn't you like "caller" ?? > > I view mm/readahead.c, mm/filemap.c and mm/page-writeback.c as part > of the VFS more than as part of the VM. But that's something that > reasonable people can disagree on. I think from the point of view of > the filesystem author, it's all VFS. > You didn't answer the second question. NeilBrown
diff --git a/mm/readahead.c b/mm/readahead.c index d3a47546d17d..83f4345a400d 100644 --- a/mm/readahead.c +++ b/mm/readahead.c @@ -18,24 +18,24 @@ * it. In that case a simple ->readpage() will be requested. * * Readahead is triggered when an application read request (whether a - * systemcall or a page fault) finds that the requested page is not in + * system call or a page fault) finds that the requested page is not in * the page cache, or that it is in the page cache and has the * %PG_readahead flag set. This flag indicates that the page was loaded - * as part of a previous read-ahead request and now that it has been - * accessed, it is time for the next read-ahead. + * as part of a previous readahead request and now that it has been + * accessed, it is time for the next readahead. * * Each readahead request is partly synchronous read, and partly async - * read-ahead. This is reflected in the struct file_ra_state which + * readahead. This is reflected in the struct file_ra_state which * contains ->size being to total number of pages, and ->async_size - * which is the number of pages in the async section. The first page in - * this async section will have %PG_readahead set as a trigger for a - * subsequent read ahead. Once a series of sequential reads has been - * established, there should be no need for a synchronous component and - * all read ahead request will be fully asynchronous. + * which is the number of pages in the async section. The %PG_readahead + * flag will be set on the first page in this async section as a trigger + * for a subsequent readahead. Once a series of sequential reads has + * been established, there should be no need for a synchronous component + * and all readahead requests will be fully asynchronous. * * When either of the triggers causes a readahead, three numbers need to - * be determined: the start of the region, the size of the region, and - * the size of the async tail. + * be determined: the start of the region to read, the size of the + * region, and the size of the async tail. * * The start of the region is simply the first page address at or after * the accessed address, which is not currently populated in the page @@ -45,7 +45,7 @@ * was explicitly requested from the determined request size, unless * this would be less than zero - then zero is used. NOTE THIS * CALCULATION IS WRONG WHEN THE START OF THE REGION IS NOT THE ACCESSED - * PAGE. + * PAGE. ALSO THIS CALCULATION IS NOT USED CONSISTENTLY. * * The size of the region is normally determined from the size of the * previous readahead which loaded the preceding pages. This may be @@ -65,13 +65,14 @@ * larger than the current request, and it is not scaled up, unless it * is at the start of file. * - * In general read ahead is accelerated at the start of the file, as + * In general, readahead is accelerated at the start of the file, as * reads from there are often sequential. There are other minor - * adjustments to the read ahead size in various special cases and these + * adjustments to the readahead size in various special cases and these * are best discovered by reading the code. * - * The above calculation determines the readahead, to which any requested - * read size may be added. + * The above calculation, based on the previous readahead size, + * determines the size of the readahead, to which any requested read + * size may be added. * * Readahead requests are sent to the filesystem using the ->readahead() * address space operation, for which mpage_readahead() is a canonical @@ -82,7 +83,7 @@ * from this will be final. * * ->readahead() will generally call readahead_page() repeatedly to get - * each page from those prepared for read ahead. It may fail to read a + * each page from those prepared for readahead. It may fail to read a * page by: * * * not calling readahead_page() sufficiently many times, effectively @@ -107,9 +108,9 @@ * In this case it is best to use delete_from_page_cache() to remove the * pages from the page cache as is automatically done for pages that * were not fetched with readahead_page(). This will allow a - * subsequent synchronous read ahead request to try them again. If they + * subsequent synchronous readahead request to try them again. If they * are left in the page cache, then they will be read individually using - * ->readpage(). + * ->readpage() which may be less efficient. * */
- use "readahead" consistently - not "read-ahead" or "read ahead". - clarify some sections that, on reflection, weren't very clear - minor punctuation/spelling fixes. Signed-off-by: NeilBrown <neilb@suse.de> --- mm/readahead.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-)