| Message ID | 159897769535.405783.17587409235571100774.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| Headers | show |
| Series | mm: Make more use of readahead_control | expand |
On Tue, Sep 01, 2020 at 05:28:15PM +0100, David Howells wrote: > > Hi Willy, > > Here's a set of patches to expand the use of the readahead_control struct, > essentially from do_sync_mmap_readahead() down. Note that I've been > passing the number of pages to read in rac->_nr_pages, and then saving it > and changing it certain points, e.g. page_cache_readahead_unbounded(). > > Also pass file_ra_state into force_page_cache_readahead(). > > Also there's an apparent minor bug in khugepaged.c that I've included a > patch for: page_cache_sync_readahead() looks to be given the wrong size in > collapse_file(). > What branch does this apply to? - Eric
On Tue, Sep 01, 2020 at 09:41:32AM -0700, Eric Biggers wrote: > On Tue, Sep 01, 2020 at 05:28:15PM +0100, David Howells wrote: > > > > Hi Willy, > > > > Here's a set of patches to expand the use of the readahead_control struct, > > essentially from do_sync_mmap_readahead() down. Note that I've been > > passing the number of pages to read in rac->_nr_pages, and then saving it > > and changing it certain points, e.g. page_cache_readahead_unbounded(). > > > > Also pass file_ra_state into force_page_cache_readahead(). > > > > Also there's an apparent minor bug in khugepaged.c that I've included a > > patch for: page_cache_sync_readahead() looks to be given the wrong size in > > collapse_file(). > > > > What branch does this apply to? He's done it on top of http://git.infradead.org/users/willy/pagecache.git I was hoping he'd include http://git.infradead.org/users/willy/pagecache.git/commitdiff/c71de787328809026cfabbcc5485cb01caca8646 http://git.infradead.org/users/willy/pagecache.git/commitdiff/f3a1cd6447e29a54b03efc2189d943f12ac1c9b9 http://git.infradead.org/users/willy/pagecache.git/commitdiff/c03d3a5a5716bb0df2fe15ec528bbd895cd18e6e as the first three patches in the series; then it should apply to Linus' tree.
On Tue, Sep 01, 2020 at 05:28:15PM +0100, David Howells wrote: > Here's a set of patches to expand the use of the readahead_control struct, > essentially from do_sync_mmap_readahead() down. I like this. > Note that I've been > passing the number of pages to read in rac->_nr_pages, and then saving it > and changing it certain points, e.g. page_cache_readahead_unbounded(). I do not like this. You're essentially mutating the meaning of _nr_pages as the ractl moves down the stack, and that's going to lead to bugs. I'd keep it as a separate argument. > Also there's an apparent minor bug in khugepaged.c that I've included a > patch for: page_cache_sync_readahead() looks to be given the wrong size in > collapse_file(). This needs to go in as an independent fix. But you didn't actually cc Song.
Matthew Wilcox <willy@infradead.org> wrote: > > Also there's an apparent minor bug in khugepaged.c that I've included a > > patch for: page_cache_sync_readahead() looks to be given the wrong size in > > collapse_file(). > > This needs to go in as an independent fix. But you didn't actually cc Song. Bah. I forgot to pass --auto to stgit. No matter, he saw it anyway. David
Matthew Wilcox <willy@infradead.org> wrote: > > Note that I've been > > passing the number of pages to read in rac->_nr_pages, and then saving it > > and changing it certain points, e.g. page_cache_readahead_unbounded(). > > I do not like this. You're essentially mutating the meaning of _nr_pages > as the ractl moves down the stack, and that's going to lead to bugs. > I'd keep it as a separate argument. The meaning isn't specified in linux/pagemap.h. Can you adjust the comments on the struct and on readahead_count() to make it more clear what the value represents? Thanks, David
On Tue, Sep 01, 2020 at 08:44:14PM +0100, David Howells wrote: > Matthew Wilcox <willy@infradead.org> wrote: > > > > Note that I've been > > > passing the number of pages to read in rac->_nr_pages, and then saving it > > > and changing it certain points, e.g. page_cache_readahead_unbounded(). > > > > I do not like this. You're essentially mutating the meaning of _nr_pages > > as the ractl moves down the stack, and that's going to lead to bugs. > > I'd keep it as a separate argument. > > The meaning isn't specified in linux/pagemap.h. Can you adjust the comments > on the struct and on readahead_count() to make it more clear what the value > represents? It's intentionally not documented. This documentation is for the filesystem author, who should not be looking at it. Neither should you :-P
Matthew Wilcox <willy@infradead.org> wrote: > He's done it on top of http://git.infradead.org/users/willy/pagecache.git Sorry, yes, I should've mentioned that. > I was hoping he'd include > http://git.infradead.org/users/willy/pagecache.git/commitdiff/c71de787328809026cfabbcc5485cb01caca8646 > http://git.infradead.org/users/willy/pagecache.git/commitdiff/f3a1cd6447e29a54b03efc2189d943f12ac1c9b9 > http://git.infradead.org/users/willy/pagecache.git/commitdiff/c03d3a5a5716bb0df2fe15ec528bbd895cd18e6e > > as the first three patches in the series; then it should apply to Linus' > tree. Did you want me to carry those patches and pass them to Linus through my fscache branch? David
Hi Willy, Here's a set of patches to expand the use of the readahead_control struct, essentially from do_sync_mmap_readahead() down. Note that I've been passing the number of pages to read in rac->_nr_pages, and then saving it and changing it certain points, e.g. page_cache_readahead_unbounded(). Also pass file_ra_state into force_page_cache_readahead(). Also there's an apparent minor bug in khugepaged.c that I've included a patch for: page_cache_sync_readahead() looks to be given the wrong size in collapse_file(). David --- David Howells (7): Fix khugepaged's request size in collapse_file() mm: Make ondemand_readahead() take a readahead_control struct mm: Push readahead_control down into force_page_cache_readahead() mm: Pass readahead_control into page_cache_{sync,async}_readahead() mm: Make __do_page_cache_readahead() use rac->_nr_pages mm: Fold ra_submit() into do_sync_mmap_readahead() mm: Pass a file_ra_state struct into force_page_cache_readahead() fs/btrfs/free-space-cache.c | 7 +-- fs/btrfs/ioctl.c | 10 +++-- fs/btrfs/relocation.c | 14 +++--- fs/btrfs/send.c | 15 ++++--- fs/ext4/dir.c | 12 ++--- fs/ext4/verity.c | 8 ++-- fs/f2fs/dir.c | 10 +++-- fs/f2fs/verity.c | 8 ++-- include/linux/pagemap.h | 11 ++--- mm/fadvise.c | 6 ++- mm/filemap.c | 33 +++++++------- mm/internal.h | 16 +------ mm/khugepaged.c | 6 +-- mm/readahead.c | 89 ++++++++++++++++++------------------- 14 files changed, 127 insertions(+), 118 deletions(-)