| Message ID | 1598001864-6123-1-git-send-email-zhaoyang.huang@unisoc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm : sync ra->ra_pages with bdi->ra_pages | expand |
On Fri, Aug 21, 2020 at 5:24 PM Zhaoyang Huang <huangzhaoyang@gmail.com> wrote: > > Some system(like android) will turbo read during startup via expanding the > readahead window and then set it back to normal(128kb as usual). However, some > files in the system process context will keep to be opened since it is opened > up and has no chance to sync with the updated value as it is almost impossible > to change the files attached to the inode(processes are unaware of these things) > > We sync ra->ra_pages with bdi->ra_pages when read. Furthermore, in consideration > of the scenario of eio and fadvise(...,POSIX_FADV_SEQUENTIAL).We introduce a > seq_read_fact to record the factors of above two cases. > > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > --- > change from v2: > fix checkpatch error > --- > --- > include/linux/fs.h | 17 +++++++++++++++++ > mm/fadvise.c | 4 +++- > mm/filemap.c | 19 +++++++++++++------ > mm/readahead.c | 37 +++++++++++++++++++++++++++++++++---- > 4 files changed, 66 insertions(+), 11 deletions(-) > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index dd28e76..e3cdc5a 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -66,6 +66,7 @@ > struct fscrypt_operations; > struct fs_context; > struct fs_parameter_description; > +struct file_ra_state; > > extern void __init inode_init(void); > extern void __init inode_init_early(void); > @@ -81,6 +82,7 @@ > extern int sysctl_protected_hardlinks; > extern int sysctl_protected_fifos; > extern int sysctl_protected_regular; > +extern void ra_pages_sync(struct file_ra_state *ra, struct address_space *mapping); > > typedef __kernel_rwf_t rwf_t; > > @@ -900,11 +902,26 @@ struct file_ra_state { > there are only # of pages ahead */ > > unsigned int ra_pages; /* Maximum readahead window */ > + int seq_read_fact; /* turbo factor of sequential read */ > unsigned int mmap_miss; /* Cache miss stat for mmap accesses */ > loff_t prev_pos; /* Cache last read() position */ > }; > > /* > + * ra->seq_read_fact == -1 indicates eio happens > + */ > +#define RA_PAGES(ra) \ > +({ \ > + unsigned int ra_pages; \ > + if (ra->seq_read_fact != -1) \ > + ra_pages = ra->ra_pages * ra->seq_read_fact; \ > + else \ > + ra_pages = ra->ra_pages; \ > + ra_pages; \ > +}) > + > + > +/* > * Check if @index falls in the readahead windows. > */ > static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index) > diff --git a/mm/fadvise.c b/mm/fadvise.c > index 467bcd0..b06e3ca 100644 > --- a/mm/fadvise.c > +++ b/mm/fadvise.c > @@ -78,6 +78,7 @@ static int generic_fadvise(struct file *file, loff_t offset, loff_t len, > switch (advice) { > case POSIX_FADV_NORMAL: > file->f_ra.ra_pages = bdi->ra_pages; > + file->f_ra.seq_read_fact = 1; > spin_lock(&file->f_lock); > file->f_mode &= ~FMODE_RANDOM; > spin_unlock(&file->f_lock); > @@ -88,7 +89,8 @@ static int generic_fadvise(struct file *file, loff_t offset, loff_t len, > spin_unlock(&file->f_lock); > break; > case POSIX_FADV_SEQUENTIAL: > - file->f_ra.ra_pages = bdi->ra_pages * 2; > + file->f_ra.ra_pages = bdi->ra_pages; > + file->f_ra.seq_read_fact = 2; > spin_lock(&file->f_lock); > file->f_mode &= ~FMODE_RANDOM; > spin_unlock(&file->f_lock); > diff --git a/mm/filemap.c b/mm/filemap.c > index d78f577..425d2a2 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -2048,6 +2048,7 @@ unsigned find_get_entries_tag(struct address_space *mapping, pgoff_t start, > static void shrink_readahead_size_eio(struct file *filp, > struct file_ra_state *ra) > { > + ra->seq_read_fact = -1; > ra->ra_pages /= 4; > } > > @@ -2473,13 +2474,16 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf) > /* If we don't want any read-ahead, don't bother */ > if (vmf->vma->vm_flags & VM_RAND_READ) > return fpin; > - if (!ra->ra_pages) > + if (!RA_PAGES(ra)) > return fpin; > > + /* sync ra->ra_pages with bdi->ra_pages*/ > + ra_pages_sync(ra, mapping); > + > if (vmf->vma->vm_flags & VM_SEQ_READ) { > fpin = maybe_unlock_mmap_for_io(vmf, fpin); > page_cache_sync_readahead(mapping, ra, file, offset, > - ra->ra_pages); > + RA_PAGES(ra)); > return fpin; > } > > @@ -2498,9 +2502,9 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf) > * mmap read-around > */ > fpin = maybe_unlock_mmap_for_io(vmf, fpin); > - ra->start = max_t(long, 0, offset - ra->ra_pages / 2); > - ra->size = ra->ra_pages; > - ra->async_size = ra->ra_pages / 4; > + ra->start = max_t(long, 0, offset - RA_PAGES(ra) / 2); > + ra->size = RA_PAGES(ra); > + ra->async_size = RA_PAGES(ra) / 4; > ra_submit(ra, mapping, file); > return fpin; > } > @@ -2519,6 +2523,9 @@ static struct file *do_async_mmap_readahead(struct vm_fault *vmf, > struct file *fpin = NULL; > pgoff_t offset = vmf->pgoff; > > + /* sync ra->ra_pages with bdi->ra_pages*/ > + ra_pages_sync(ra, mapping); > + > /* If we don't want any read-ahead, don't bother */ > if (vmf->vma->vm_flags & VM_RAND_READ) > return fpin; > @@ -2527,7 +2534,7 @@ static struct file *do_async_mmap_readahead(struct vm_fault *vmf, > if (PageReadahead(page)) { > fpin = maybe_unlock_mmap_for_io(vmf, fpin); > page_cache_async_readahead(mapping, ra, file, > - page, offset, ra->ra_pages); > + page, offset, RA_PAGES(ra)); > } > return fpin; > } > diff --git a/mm/readahead.c b/mm/readahead.c > index a459365..50fdf1d 100644 > --- a/mm/readahead.c > +++ b/mm/readahead.c > @@ -32,10 +32,26 @@ > file_ra_state_init(struct file_ra_state *ra, struct address_space *mapping) > { > ra->ra_pages = inode_to_bdi(mapping->host)->ra_pages; > + ra->seq_read_fact = 1; > ra->prev_pos = -1; > } > EXPORT_SYMBOL_GPL(file_ra_state_init); > > +/*sync ra->ra_pages with bdi->ra_pages*/ > +void ra_pages_sync(struct file_ra_state *ra, > + struct address_space *mapping) > +{ > + unsigned int ra_pages = ra->ra_pages; > + > + if (ra->seq_read_fact == -1) > + return; > + > + ra_pages = inode_to_bdi(mapping->host)->ra_pages * ra->seq_read_fact; > + if (RA_PAGES(ra) != ra_pages) > + ra->ra_pages = inode_to_bdi(mapping->host)->ra_pages; > +} > +EXPORT_SYMBOL_GPL(ra_pages_sync); > + > /* > * see if a page needs releasing upon read_cache_pages() failure > * - the caller of read_cache_pages() may have set PG_private or PG_fscache > @@ -228,11 +244,14 @@ int force_page_cache_readahead(struct address_space *mapping, struct file *filp, > if (unlikely(!mapping->a_ops->readpage && !mapping->a_ops->readpages)) > return -EINVAL; > > + /* sync ra->ra_pages with bdi->ra_pages*/ > + ra_pages_sync(ra, mapping); > + > /* > * If the request exceeds the readahead window, allow the read to > * be up to the optimal hardware IO size > */ > - max_pages = max_t(unsigned long, bdi->io_pages, ra->ra_pages); > + max_pages = max_t(unsigned long, bdi->io_pages, RA_PAGES(ra)); > nr_to_read = min(nr_to_read, max_pages); > while (nr_to_read) { > unsigned long this_chunk = (2 * 1024 * 1024) / PAGE_SIZE; > @@ -384,10 +403,14 @@ static int try_context_readahead(struct address_space *mapping, > unsigned long req_size) > { > struct backing_dev_info *bdi = inode_to_bdi(mapping->host); > - unsigned long max_pages = ra->ra_pages; > + unsigned long max_pages; > unsigned long add_pages; > pgoff_t prev_offset; > > + /* sync ra->ra_pages with bdi->ra_pages*/ > + ra_pages_sync(ra, mapping); > + > + max_pages = RA_PAGES(ra); > /* > * If the request exceeds the readahead window, allow the read to > * be up to the optimal hardware IO size > @@ -510,9 +533,12 @@ void page_cache_sync_readahead(struct address_space *mapping, > pgoff_t offset, unsigned long req_size) > { > /* no read-ahead */ > - if (!ra->ra_pages) > + if (!RA_PAGES(ra)) > return; > > + /* sync ra->ra_pages with bdi->ra_pages*/ > + ra_pages_sync(ra, mapping); > + > if (blk_cgroup_congested()) > return; > > @@ -549,9 +575,12 @@ void page_cache_sync_readahead(struct address_space *mapping, > unsigned long req_size) > { > /* no read-ahead */ > - if (!ra->ra_pages) > + if (!RA_PAGES(ra)) > return; > > + /* sync ra->ra_pages with bdi->ra_pages*/ > + ra_pages_sync(ra, mapping); > + > /* > * Same bit is used for PG_readahead and PG_reclaim. > */ > -- > 1.9.1 > This patch has been verified on an android system and reduces 15% of UNITERRUPTIBLE_SLEEP_BLOCKIO which was used to be caused by wrong ra->ra_pages.
On Fri, Aug 21, 2020 at 05:31:52PM +0800, Zhaoyang Huang wrote: > This patch has been verified on an android system and reduces 15% of > UNITERRUPTIBLE_SLEEP_BLOCKIO which was used to be caused by wrong > ra->ra_pages. Wait, what? Readahead doesn't sleep on the pages it's requesting. Unless ... your file access pattern is random, so you end up submitting a readahead I/O that's bigger than needed, so takes longer for the page you actually wanted to be returned. I know we have the LOTSAMISS logic, but that's not really enough. OK, assuming this problem is really about sync mmap (ie executables), this makes a bit more sense. I think the real problem is here: ra->start = max_t(long, 0, offset - ra->ra_pages / 2); ra->size = ra->ra_pages; ra->async_size = ra->ra_pages / 4; ra_submit(ra, mapping, file); which actually skips all the logic we have in ondemand_readahead() for adjusting the readahead size. Ugh, this is a mess. I think a quick fix to your problem will be just replacing ra->ra_pages with bdi->ra_pages in do_sync_mmap_readahead() and leaving ra->ra_pages alone everywhere else. We need a smarter readahead algorithm for mmap'ed files, and I don't have time to work on it right now. So let's stick to the same dumb algorithm, but make it responsive to bdi ra_pages being reset.
On Fri, Aug 21, 2020 at 7:57 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Fri, Aug 21, 2020 at 05:31:52PM +0800, Zhaoyang Huang wrote: > > This patch has been verified on an android system and reduces 15% of > > UNITERRUPTIBLE_SLEEP_BLOCKIO which was used to be caused by wrong > > ra->ra_pages. > > Wait, what? Readahead doesn't sleep on the pages it's requesting. > Unless ... your file access pattern is random, so you end up submitting > a readahead I/O that's bigger than needed, so takes longer for the page > you actually wanted to be returned. I know we have the LOTSAMISS > logic, but that's not really enough. actually, async read even if hitting the marker will also introduce huge mount of read by wrong ra->ra_pages, which will be used as req_size for ondemand_readahead. > > OK, assuming this problem is really about sync mmap (ie executables), > this makes a bit more sense. I think the real problem is here: > > ra->start = max_t(long, 0, offset - ra->ra_pages / 2); > ra->size = ra->ra_pages; > ra->async_size = ra->ra_pages / 4; > ra_submit(ra, mapping, file); > > which actually skips all the logic we have in ondemand_readahead() > for adjusting the readahead size. Ugh, this is a mess. > > I think a quick fix to your problem will be just replacing ra->ra_pages > with bdi->ra_pages in do_sync_mmap_readahead() and leaving ra->ra_pages > alone everywhere else. > > We need a smarter readahead algorithm for mmap'ed files, and I don't have > time to work on it right now. So let's stick to the same dumb algorithm, > but make it responsive to bdi ra_pages being reset.
On Fri, Aug 21, 2020 at 7:57 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Fri, Aug 21, 2020 at 05:31:52PM +0800, Zhaoyang Huang wrote: > > This patch has been verified on an android system and reduces 15% of > > UNITERRUPTIBLE_SLEEP_BLOCKIO which was used to be caused by wrong > > ra->ra_pages. > > Wait, what? Readahead doesn't sleep on the pages it's requesting. > Unless ... your file access pattern is random, so you end up submitting > a readahead I/O that's bigger than needed, so takes longer for the page > you actually wanted to be returned. I know we have the LOTSAMISS > logic, but that's not really enough. > > OK, assuming this problem is really about sync mmap (ie executables), > this makes a bit more sense. I think the real problem is here: > > ra->start = max_t(long, 0, offset - ra->ra_pages / 2); > ra->size = ra->ra_pages; > ra->async_size = ra->ra_pages / 4; > ra_submit(ra, mapping, file); > > which actually skips all the logic we have in ondemand_readahead() > for adjusting the readahead size. Ugh, this is a mess. > > I think a quick fix to your problem will be just replacing ra->ra_pages > with bdi->ra_pages in do_sync_mmap_readahead() and leaving ra->ra_pages > alone everywhere else. > We can't just sync ra->ra_pages with bdi->ra_pages as eio and fadvise will shrink or turbo it, that is why I introduce seq_read_fact in this commit > We need a smarter readahead algorithm for mmap'ed files, and I don't have > time to work on it right now. So let's stick to the same dumb algorithm, > but make it responsive to bdi ra_pages being reset.
diff --git a/include/linux/fs.h b/include/linux/fs.h index dd28e76..e3cdc5a 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -66,6 +66,7 @@ struct fscrypt_operations; struct fs_context; struct fs_parameter_description; +struct file_ra_state; extern void __init inode_init(void); extern void __init inode_init_early(void); @@ -81,6 +82,7 @@ extern int sysctl_protected_hardlinks; extern int sysctl_protected_fifos; extern int sysctl_protected_regular; +extern void ra_pages_sync(struct file_ra_state *ra, struct address_space *mapping); typedef __kernel_rwf_t rwf_t; @@ -900,11 +902,26 @@ struct file_ra_state { there are only # of pages ahead */ unsigned int ra_pages; /* Maximum readahead window */ + int seq_read_fact; /* turbo factor of sequential read */ unsigned int mmap_miss; /* Cache miss stat for mmap accesses */ loff_t prev_pos; /* Cache last read() position */ }; /* + * ra->seq_read_fact == -1 indicates eio happens + */ +#define RA_PAGES(ra) \ +({ \ + unsigned int ra_pages; \ + if (ra->seq_read_fact != -1) \ + ra_pages = ra->ra_pages * ra->seq_read_fact; \ + else \ + ra_pages = ra->ra_pages; \ + ra_pages; \ +}) + + +/* * Check if @index falls in the readahead windows. */ static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index) diff --git a/mm/fadvise.c b/mm/fadvise.c index 467bcd0..b06e3ca 100644 --- a/mm/fadvise.c +++ b/mm/fadvise.c @@ -78,6 +78,7 @@ static int generic_fadvise(struct file *file, loff_t offset, loff_t len, switch (advice) { case POSIX_FADV_NORMAL: file->f_ra.ra_pages = bdi->ra_pages; + file->f_ra.seq_read_fact = 1; spin_lock(&file->f_lock); file->f_mode &= ~FMODE_RANDOM; spin_unlock(&file->f_lock); @@ -88,7 +89,8 @@ static int generic_fadvise(struct file *file, loff_t offset, loff_t len, spin_unlock(&file->f_lock); break; case POSIX_FADV_SEQUENTIAL: - file->f_ra.ra_pages = bdi->ra_pages * 2; + file->f_ra.ra_pages = bdi->ra_pages; + file->f_ra.seq_read_fact = 2; spin_lock(&file->f_lock); file->f_mode &= ~FMODE_RANDOM; spin_unlock(&file->f_lock); diff --git a/mm/filemap.c b/mm/filemap.c index d78f577..425d2a2 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -2048,6 +2048,7 @@ unsigned find_get_entries_tag(struct address_space *mapping, pgoff_t start, static void shrink_readahead_size_eio(struct file *filp, struct file_ra_state *ra) { + ra->seq_read_fact = -1; ra->ra_pages /= 4; } @@ -2473,13 +2474,16 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf) /* If we don't want any read-ahead, don't bother */ if (vmf->vma->vm_flags & VM_RAND_READ) return fpin; - if (!ra->ra_pages) + if (!RA_PAGES(ra)) return fpin; + /* sync ra->ra_pages with bdi->ra_pages*/ + ra_pages_sync(ra, mapping); + if (vmf->vma->vm_flags & VM_SEQ_READ) { fpin = maybe_unlock_mmap_for_io(vmf, fpin); page_cache_sync_readahead(mapping, ra, file, offset, - ra->ra_pages); + RA_PAGES(ra)); return fpin; } @@ -2498,9 +2502,9 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf) * mmap read-around */ fpin = maybe_unlock_mmap_for_io(vmf, fpin); - ra->start = max_t(long, 0, offset - ra->ra_pages / 2); - ra->size = ra->ra_pages; - ra->async_size = ra->ra_pages / 4; + ra->start = max_t(long, 0, offset - RA_PAGES(ra) / 2); + ra->size = RA_PAGES(ra); + ra->async_size = RA_PAGES(ra) / 4; ra_submit(ra, mapping, file); return fpin; } @@ -2519,6 +2523,9 @@ static struct file *do_async_mmap_readahead(struct vm_fault *vmf, struct file *fpin = NULL; pgoff_t offset = vmf->pgoff; + /* sync ra->ra_pages with bdi->ra_pages*/ + ra_pages_sync(ra, mapping); + /* If we don't want any read-ahead, don't bother */ if (vmf->vma->vm_flags & VM_RAND_READ) return fpin; @@ -2527,7 +2534,7 @@ static struct file *do_async_mmap_readahead(struct vm_fault *vmf, if (PageReadahead(page)) { fpin = maybe_unlock_mmap_for_io(vmf, fpin); page_cache_async_readahead(mapping, ra, file, - page, offset, ra->ra_pages); + page, offset, RA_PAGES(ra)); } return fpin; } diff --git a/mm/readahead.c b/mm/readahead.c index a459365..50fdf1d 100644 --- a/mm/readahead.c +++ b/mm/readahead.c @@ -32,10 +32,26 @@ file_ra_state_init(struct file_ra_state *ra, struct address_space *mapping) { ra->ra_pages = inode_to_bdi(mapping->host)->ra_pages; + ra->seq_read_fact = 1; ra->prev_pos = -1; } EXPORT_SYMBOL_GPL(file_ra_state_init); +/*sync ra->ra_pages with bdi->ra_pages*/ +void ra_pages_sync(struct file_ra_state *ra, + struct address_space *mapping) +{ + unsigned int ra_pages = ra->ra_pages; + + if (ra->seq_read_fact == -1) + return; + + ra_pages = inode_to_bdi(mapping->host)->ra_pages * ra->seq_read_fact; + if (RA_PAGES(ra) != ra_pages) + ra->ra_pages = inode_to_bdi(mapping->host)->ra_pages; +} +EXPORT_SYMBOL_GPL(ra_pages_sync); + /* * see if a page needs releasing upon read_cache_pages() failure * - the caller of read_cache_pages() may have set PG_private or PG_fscache @@ -228,11 +244,14 @@ int force_page_cache_readahead(struct address_space *mapping, struct file *filp, if (unlikely(!mapping->a_ops->readpage && !mapping->a_ops->readpages)) return -EINVAL; + /* sync ra->ra_pages with bdi->ra_pages*/ + ra_pages_sync(ra, mapping); + /* * If the request exceeds the readahead window, allow the read to * be up to the optimal hardware IO size */ - max_pages = max_t(unsigned long, bdi->io_pages, ra->ra_pages); + max_pages = max_t(unsigned long, bdi->io_pages, RA_PAGES(ra)); nr_to_read = min(nr_to_read, max_pages); while (nr_to_read) { unsigned long this_chunk = (2 * 1024 * 1024) / PAGE_SIZE; @@ -384,10 +403,14 @@ static int try_context_readahead(struct address_space *mapping, unsigned long req_size) { struct backing_dev_info *bdi = inode_to_bdi(mapping->host); - unsigned long max_pages = ra->ra_pages; + unsigned long max_pages; unsigned long add_pages; pgoff_t prev_offset; + /* sync ra->ra_pages with bdi->ra_pages*/ + ra_pages_sync(ra, mapping); + + max_pages = RA_PAGES(ra); /* * If the request exceeds the readahead window, allow the read to * be up to the optimal hardware IO size @@ -510,9 +533,12 @@ void page_cache_sync_readahead(struct address_space *mapping, pgoff_t offset, unsigned long req_size) { /* no read-ahead */ - if (!ra->ra_pages) + if (!RA_PAGES(ra)) return; + /* sync ra->ra_pages with bdi->ra_pages*/ + ra_pages_sync(ra, mapping); + if (blk_cgroup_congested()) return; @@ -549,9 +575,12 @@ void page_cache_sync_readahead(struct address_space *mapping, unsigned long req_size) { /* no read-ahead */ - if (!ra->ra_pages) + if (!RA_PAGES(ra)) return; + /* sync ra->ra_pages with bdi->ra_pages*/ + ra_pages_sync(ra, mapping); + /* * Same bit is used for PG_readahead and PG_reclaim. */
Some system(like android) will turbo read during startup via expanding the readahead window and then set it back to normal(128kb as usual). However, some files in the system process context will keep to be opened since it is opened up and has no chance to sync with the updated value as it is almost impossible to change the files attached to the inode(processes are unaware of these things) We sync ra->ra_pages with bdi->ra_pages when read. Furthermore, in consideration of the scenario of eio and fadvise(...,POSIX_FADV_SEQUENTIAL).We introduce a seq_read_fact to record the factors of above two cases. Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com> --- change from v2: fix checkpatch error --- --- include/linux/fs.h | 17 +++++++++++++++++ mm/fadvise.c | 4 +++- mm/filemap.c | 19 +++++++++++++------ mm/readahead.c | 37 +++++++++++++++++++++++++++++++++---- 4 files changed, 66 insertions(+), 11 deletions(-)