| Message ID | 20200219210103.32400-2-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Change readahead API | expand |
On 2/19/20 1:00 PM, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > The readahead code is part of the page cache so should be found in the > pagemap.h file. force_page_cache_readahead is only used within mm, > so move it to mm/internal.h instead. Remove the parameter names where > they add no value, and rename the ones which were actively misleading. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > block/blk-core.c | 1 + > include/linux/mm.h | 19 ------------------- > include/linux/pagemap.h | 8 ++++++++ > mm/fadvise.c | 2 ++ > mm/internal.h | 2 ++ > 5 files changed, 13 insertions(+), 19 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index 089e890ab208..41417bb93634 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -20,6 +20,7 @@ > #include <linux/blk-mq.h> > #include <linux/highmem.h> > #include <linux/mm.h> > +#include <linux/pagemap.h> Yes. But I think these files also need a similar change: fs/btrfs/disk-io.c fs/nfs/super.c ...because they also use VM_READAHEAD_PAGES, and do not directly include pagemap.h yet. > #include <linux/kernel_stat.h> > #include <linux/string.h> > #include <linux/init.h> > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 52269e56c514..68dcda9a2112 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2401,25 +2401,6 @@ extern vm_fault_t filemap_page_mkwrite(struct vm_fault *vmf); > int __must_check write_one_page(struct page *page); > void task_dirty_inc(struct task_struct *tsk); > > -/* readahead.c */ > -#define VM_READAHEAD_PAGES (SZ_128K / PAGE_SIZE) > - > -int force_page_cache_readahead(struct address_space *mapping, struct file *filp, > - pgoff_t offset, unsigned long nr_to_read); > - > -void page_cache_sync_readahead(struct address_space *mapping, > - struct file_ra_state *ra, > - struct file *filp, > - pgoff_t offset, > - unsigned long size); > - > -void page_cache_async_readahead(struct address_space *mapping, > - struct file_ra_state *ra, > - struct file *filp, > - struct page *pg, > - pgoff_t offset, > - unsigned long size); > - > extern unsigned long stack_guard_gap; > /* Generic expand stack which grows the stack according to GROWS{UP,DOWN} */ > extern int expand_stack(struct vm_area_struct *vma, unsigned long address); > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h > index ccb14b6a16b5..24894b9b90c9 100644 > --- a/include/linux/pagemap.h > +++ b/include/linux/pagemap.h > @@ -614,6 +614,14 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask); > void delete_from_page_cache_batch(struct address_space *mapping, > struct pagevec *pvec); > > +#define VM_READAHEAD_PAGES (SZ_128K / PAGE_SIZE) > + > +void page_cache_sync_readahead(struct address_space *, struct file_ra_state *, > + struct file *, pgoff_t index, unsigned long req_count); Yes, "struct address_space *mapping" is weird, but I don't know if it's "misleading", given that it's actually one of the things you have to learn right from the beginning, with linux-mm, right? Or is that about to change? I'm not asking to restore this to "struct address_space *mapping", but I thought it's worth mentioning out loud, especially if you or others are planning on changing those names or something. Just curious. thanks,
On Thu, Feb 20, 2020 at 06:43:31PM -0800, John Hubbard wrote: > Yes. But I think these files also need a similar change: > > fs/btrfs/disk-io.c That gets pagemap.h through ctree.h, so I think it's fine. It's already using mapping_set_gfp_mask(), so it already depends on pagemap.h. > fs/nfs/super.c That gets it through linux/nfs_fs.h. I was reluctant to not add it to blk-core.c because it doesn't seem necessarily intuitive that the block device core would include pagemap.h. That said, blkdev.h does include pagemap.h, so maybe I don't need to include it here. > ...because they also use VM_READAHEAD_PAGES, and do not directly include > pagemap.h yet. > > +#define VM_READAHEAD_PAGES (SZ_128K / PAGE_SIZE) > > + > > +void page_cache_sync_readahead(struct address_space *, struct file_ra_state *, > > + struct file *, pgoff_t index, unsigned long req_count); > > Yes, "struct address_space *mapping" is weird, but I don't know if it's > "misleading", given that it's actually one of the things you have to learn > right from the beginning, with linux-mm, right? Or is that about to change? > > I'm not asking to restore this to "struct address_space *mapping", but I thought > it's worth mentioning out loud, especially if you or others are planning on > changing those names or something. Just curious. No plans (on my part) to change the name, although I have heard people grumbling that there's very little need for it to be a separate struct from inode, except for the benefit of coda, which is not exactly a filesystem with a lot of users ... Anyway, no plans to change it. If there were something _special_ about it like a theoretical: void mapping_dedup(struct address_space *canonical, struct address_space *victim); then that's useful information and shouldn't be deleted. But I don't think the word 'mapping' there conveys anything useful (other than the convention is to call a 'struct address_space' a mapping, which you'll see soon enough once you look at any of the .c files).
On 2/21/20 1:48 PM, Matthew Wilcox wrote: > On Thu, Feb 20, 2020 at 06:43:31PM -0800, John Hubbard wrote: >> Yes. But I think these files also need a similar change: >> >> fs/btrfs/disk-io.c > > That gets pagemap.h through ctree.h, so I think it's fine. It's > already using mapping_set_gfp_mask(), so it already depends on pagemap.h. > >> fs/nfs/super.c > > That gets it through linux/nfs_fs.h. > > I was reluctant to not add it to blk-core.c because it doesn't seem > necessarily intuitive that the block device core would include pagemap.h. > > That said, blkdev.h does include pagemap.h, so maybe I don't need to > include it here. OK. Looks good (either through blkdev.h or as-is), so: Reviewed-by: John Hubbard <jhubbard@nvidia.com> > >> ...because they also use VM_READAHEAD_PAGES, and do not directly include >> pagemap.h yet. > >>> +#define VM_READAHEAD_PAGES (SZ_128K / PAGE_SIZE) >>> + >>> +void page_cache_sync_readahead(struct address_space *, struct file_ra_state *, >>> + struct file *, pgoff_t index, unsigned long req_count); >> >> Yes, "struct address_space *mapping" is weird, but I don't know if it's >> "misleading", given that it's actually one of the things you have to learn >> right from the beginning, with linux-mm, right? Or is that about to change? >> >> I'm not asking to restore this to "struct address_space *mapping", but I thought >> it's worth mentioning out loud, especially if you or others are planning on >> changing those names or something. Just curious. > > No plans (on my part) to change the name, although I have heard people > grumbling that there's very little need for it to be a separate struct > from inode, except for the benefit of coda, which is not exactly a > filesystem with a lot of users ... > > Anyway, no plans to change it. If there were something _special_ about > it like a theoretical: > > void mapping_dedup(struct address_space *canonical, > struct address_space *victim); > > then that's useful information and shouldn't be deleted. But I don't > think the word 'mapping' there conveys anything useful (other than the > convention is to call a 'struct address_space' a mapping, which you'll > see soon enough once you look at any of the .c files). > OK, that's consistent and makes sense. thanks,
On Wed, Feb 19, 2020 at 01:00:40PM -0800, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > The readahead code is part of the page cache so should be found in the > pagemap.h file. force_page_cache_readahead is only used within mm, > so move it to mm/internal.h instead. Remove the parameter names where > they add no value, and rename the ones which were actively misleading. Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/block/blk-core.c b/block/blk-core.c index 089e890ab208..41417bb93634 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -20,6 +20,7 @@ #include <linux/blk-mq.h> #include <linux/highmem.h> #include <linux/mm.h> +#include <linux/pagemap.h> #include <linux/kernel_stat.h> #include <linux/string.h> #include <linux/init.h> diff --git a/include/linux/mm.h b/include/linux/mm.h index 52269e56c514..68dcda9a2112 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2401,25 +2401,6 @@ extern vm_fault_t filemap_page_mkwrite(struct vm_fault *vmf); int __must_check write_one_page(struct page *page); void task_dirty_inc(struct task_struct *tsk); -/* readahead.c */ -#define VM_READAHEAD_PAGES (SZ_128K / PAGE_SIZE) - -int force_page_cache_readahead(struct address_space *mapping, struct file *filp, - pgoff_t offset, unsigned long nr_to_read); - -void page_cache_sync_readahead(struct address_space *mapping, - struct file_ra_state *ra, - struct file *filp, - pgoff_t offset, - unsigned long size); - -void page_cache_async_readahead(struct address_space *mapping, - struct file_ra_state *ra, - struct file *filp, - struct page *pg, - pgoff_t offset, - unsigned long size); - extern unsigned long stack_guard_gap; /* Generic expand stack which grows the stack according to GROWS{UP,DOWN} */ extern int expand_stack(struct vm_area_struct *vma, unsigned long address); diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index ccb14b6a16b5..24894b9b90c9 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -614,6 +614,14 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask); void delete_from_page_cache_batch(struct address_space *mapping, struct pagevec *pvec); +#define VM_READAHEAD_PAGES (SZ_128K / PAGE_SIZE) + +void page_cache_sync_readahead(struct address_space *, struct file_ra_state *, + struct file *, pgoff_t index, unsigned long req_count); +void page_cache_async_readahead(struct address_space *, struct file_ra_state *, + struct file *, struct page *, pgoff_t index, + unsigned long req_count); + /* * Like add_to_page_cache_locked, but used to add newly allocated pages: * the page is new, so we can just run __SetPageLocked() against it. diff --git a/mm/fadvise.c b/mm/fadvise.c index 4f17c83db575..3efebfb9952c 100644 --- a/mm/fadvise.c +++ b/mm/fadvise.c @@ -22,6 +22,8 @@ #include <asm/unistd.h> +#include "internal.h" + /* * POSIX_FADV_WILLNEED could set PG_Referenced, and POSIX_FADV_NOREUSE could * deactivate the pages and clear PG_Referenced. diff --git a/mm/internal.h b/mm/internal.h index 3cf20ab3ca01..83f353e74654 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -49,6 +49,8 @@ void unmap_page_range(struct mmu_gather *tlb, unsigned long addr, unsigned long end, struct zap_details *details); +int force_page_cache_readahead(struct address_space *, struct file *, + pgoff_t index, unsigned long nr_to_read); extern unsigned int __do_page_cache_readahead(struct address_space *mapping, struct file *filp, pgoff_t offset, unsigned long nr_to_read, unsigned long lookahead_size);