| Message ID | 164549983736.9187.16755913785880819183.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove remaining parts of congestion tracking code. | expand |
On Tue, 22 Feb 2022 at 04:18, NeilBrown <neilb@suse.de> wrote: > > If ->readpages doesn't process all the pages, then it is best to act as > though they weren't requested so that a subsequent readahead can try > again. > So: > - remove any 'ahead' pages from the page cache so they can be loaded > with ->readahead() rather then multiple ->read()s > - update the file_ra_state to reflect the reads that were actually > submitted. > > This allows ->readpages() to abort early due e.g. to congestion, which > will then allow us to remove the inode_read_congested() test from > page_Cache_async_ra(). > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > mm/readahead.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/mm/readahead.c b/mm/readahead.c > index 73b2bc5302e0..8a97bd408cf6 100644 > --- a/mm/readahead.c > +++ b/mm/readahead.c > @@ -104,7 +104,13 @@ > * for necessary resources (e.g. memory or indexing information) to > * become available. Pages in the final ``async_size`` may be > * considered less urgent and failure to read them is more acceptable. > - * They will eventually be read individually using ->readpage(). > + * 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 > + * are left in the page cache, then they will be read individually using > + * ->readpage(). > + * > */ > > #include <linux/kernel.h> > @@ -226,8 +232,17 @@ static void read_pages(struct readahead_control *rac, struct list_head *pages, > > if (aops->readahead) { > aops->readahead(rac); > - /* Clean up the remaining pages */ > + /* > + * Clean up the remaining pages. The sizes in ->ra > + * maybe be used to size next read-ahead, so make sure > + * they accurately reflect what happened. > + */ > while ((page = readahead_page(rac))) { > + rac->ra->size -= 1; > + if (rac->ra->async_size > 0) { > + rac->ra->async_size -= 1; > + delete_from_page_cache(page); > + } Does the above imply that filesystem should submit at least ra->size pages, regardless of congestion? Thanks, Miklos
On Wed, 02 Mar 2022, Miklos Szeredi wrote: > On Tue, 22 Feb 2022 at 04:18, NeilBrown <neilb@suse.de> wrote: > > > > If ->readpages doesn't process all the pages, then it is best to act as > > though they weren't requested so that a subsequent readahead can try > > again. > > So: > > - remove any 'ahead' pages from the page cache so they can be loaded > > with ->readahead() rather then multiple ->read()s > > - update the file_ra_state to reflect the reads that were actually > > submitted. > > > > This allows ->readpages() to abort early due e.g. to congestion, which > > will then allow us to remove the inode_read_congested() test from > > page_Cache_async_ra(). > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > mm/readahead.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/mm/readahead.c b/mm/readahead.c > > index 73b2bc5302e0..8a97bd408cf6 100644 > > --- a/mm/readahead.c > > +++ b/mm/readahead.c > > @@ -104,7 +104,13 @@ > > * for necessary resources (e.g. memory or indexing information) to > > * become available. Pages in the final ``async_size`` may be > > * considered less urgent and failure to read them is more acceptable. > > - * They will eventually be read individually using ->readpage(). > > + * 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 > > + * are left in the page cache, then they will be read individually using > > + * ->readpage(). > > + * > > */ > > > > #include <linux/kernel.h> > > @@ -226,8 +232,17 @@ static void read_pages(struct readahead_control *rac, struct list_head *pages, > > > > if (aops->readahead) { > > aops->readahead(rac); > > - /* Clean up the remaining pages */ > > + /* > > + * Clean up the remaining pages. The sizes in ->ra > > + * maybe be used to size next read-ahead, so make sure > > + * they accurately reflect what happened. > > + */ > > while ((page = readahead_page(rac))) { > > + rac->ra->size -= 1; > > + if (rac->ra->async_size > 0) { > > + rac->ra->async_size -= 1; > > + delete_from_page_cache(page); > > + } > > Does the above imply that filesystem should submit at least ra->size > pages, regardless of congestion? ra->size - ra_async_size pages should be submitted reguardless of congestion. NeilBrown
diff --git a/mm/readahead.c b/mm/readahead.c index 73b2bc5302e0..8a97bd408cf6 100644 --- a/mm/readahead.c +++ b/mm/readahead.c @@ -104,7 +104,13 @@ * for necessary resources (e.g. memory or indexing information) to * become available. Pages in the final ``async_size`` may be * considered less urgent and failure to read them is more acceptable. - * They will eventually be read individually using ->readpage(). + * 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 + * are left in the page cache, then they will be read individually using + * ->readpage(). + * */ #include <linux/kernel.h> @@ -226,8 +232,17 @@ static void read_pages(struct readahead_control *rac, struct list_head *pages, if (aops->readahead) { aops->readahead(rac); - /* Clean up the remaining pages */ + /* + * Clean up the remaining pages. The sizes in ->ra + * maybe be used to size next read-ahead, so make sure + * they accurately reflect what happened. + */ while ((page = readahead_page(rac))) { + rac->ra->size -= 1; + if (rac->ra->async_size > 0) { + rac->ra->async_size -= 1; + delete_from_page_cache(page); + } unlock_page(page); put_page(page); }
If ->readpages doesn't process all the pages, then it is best to act as though they weren't requested so that a subsequent readahead can try again. So: - remove any 'ahead' pages from the page cache so they can be loaded with ->readahead() rather then multiple ->read()s - update the file_ra_state to reflect the reads that were actually submitted. This allows ->readpages() to abort early due e.g. to congestion, which will then allow us to remove the inode_read_congested() test from page_Cache_async_ra(). Signed-off-by: NeilBrown <neilb@suse.de> --- mm/readahead.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)