| Message ID | 20200125013553.24899-12-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Change readahead API | expand |
On Fri, Jan 24, 2020 at 05:35:52PM -0800, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > Use the new readahead operation in fuse. Switching away from the > read_cache_pages() helper gets rid of an implicit call to put_page(), > so we can get rid of the get_page() call in fuse_readpages_fill(). > We can also get rid of the call to fuse_wait_on_page_writeback() as > this page is newly allocated and so cannot be under writeback. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> ..... > @@ -968,19 +962,24 @@ static int fuse_readpages(struct file *file, struct address_space *mapping, > data.max_pages = min_t(unsigned int, nr_pages, fc->max_pages); > ; > data.ia = fuse_io_alloc(NULL, data.max_pages); > - err = -ENOMEM; > if (!data.ia) > goto out; > > - err = read_cache_pages(mapping, pages, fuse_readpages_fill, &data); > - if (!err) { > - if (data.ia->ap.num_pages) > - fuse_send_readpages(data.ia, file); > - else > - fuse_io_free(data.ia); > + while (nr_pages--) { > + struct page *page = readahead_page(mapping, start++); > + int err = fuse_readpages_fill(&data, page); > + > + if (!err) > + continue; > + nr_pages++; > + goto out; > } That's some pretty convoluted logic. Perhaps: for (; nr_pages > 0 ; nr_pages--) { struct page *page = readahead_page(mapping, start++); if (fuse_readpages_fill(&data, page)) goto out; } -Dave.
On Sat, Jan 25, 2020 at 2:36 AM Matthew Wilcox <willy@infradead.org> wrote: > > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > Use the new readahead operation in fuse. Switching away from the > read_cache_pages() helper gets rid of an implicit call to put_page(), > so we can get rid of the get_page() call in fuse_readpages_fill(). > We can also get rid of the call to fuse_wait_on_page_writeback() as > this page is newly allocated and so cannot be under writeback. Not sure. fuse_wait_on_page_writeback() waits not on the page but the byte range indicated by the index. Fuse writeback goes through some hoops to prevent reclaim related deadlocks, which means that the byte range could still be under writeback, yet the page belonging to that range be already reclaimed. This fuse_wait_on_page_writeback() is trying to serialize reads against such writes. Thanks, Miklos
On Wed, Jan 29, 2020 at 11:50:37AM +0100, Miklos Szeredi wrote: > On Sat, Jan 25, 2020 at 2:36 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > > > Use the new readahead operation in fuse. Switching away from the > > read_cache_pages() helper gets rid of an implicit call to put_page(), > > so we can get rid of the get_page() call in fuse_readpages_fill(). > > We can also get rid of the call to fuse_wait_on_page_writeback() as > > this page is newly allocated and so cannot be under writeback. > > Not sure. fuse_wait_on_page_writeback() waits not on the page but the > byte range indicated by the index. > > Fuse writeback goes through some hoops to prevent reclaim related > deadlocks, which means that the byte range could still be under > writeback, yet the page belonging to that range be already reclaimed. > This fuse_wait_on_page_writeback() is trying to serialize reads > against such writes. Thanks. I'll put that back in.
On Wed, Jan 29, 2020 at 12:08:29PM +1100, Dave Chinner wrote: > On Fri, Jan 24, 2020 at 05:35:52PM -0800, Matthew Wilcox wrote: > > + while (nr_pages--) { > > + struct page *page = readahead_page(mapping, start++); > > + int err = fuse_readpages_fill(&data, page); > > + > > + if (!err) > > + continue; > > + nr_pages++; > > + goto out; > > } > > That's some pretty convoluted logic. Perhaps: > > for (; nr_pages > 0 ; nr_pages--) { > struct page *page = readahead_page(mapping, start++); > > if (fuse_readpages_fill(&data, page)) > goto out; > } I have a bit of an aversion to that style of for loop ... I like this instead: while (nr_pages) { struct page *page = readahead_page(mapping, start++); if (fuse_readpages_fill(&data, page) != 0) goto out; nr_pages--; }
On Thu, Jan 30, 2020 at 01:35:33PM -0800, Matthew Wilcox wrote: > On Wed, Jan 29, 2020 at 12:08:29PM +1100, Dave Chinner wrote: > > On Fri, Jan 24, 2020 at 05:35:52PM -0800, Matthew Wilcox wrote: > > > + while (nr_pages--) { > > > + struct page *page = readahead_page(mapping, start++); > > > + int err = fuse_readpages_fill(&data, page); > > > + > > > + if (!err) > > > + continue; > > > + nr_pages++; > > > + goto out; > > > } > > > > That's some pretty convoluted logic. Perhaps: > > > > for (; nr_pages > 0 ; nr_pages--) { > > struct page *page = readahead_page(mapping, start++); > > > > if (fuse_readpages_fill(&data, page)) > > goto out; > > } > > I have a bit of an aversion to that style of for loop ... I like this > instead: > > while (nr_pages) { > struct page *page = readahead_page(mapping, start++); > > if (fuse_readpages_fill(&data, page) != 0) > goto out; > nr_pages--; > } yup, that's also fine. -Dave.
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index ce715380143c..b6d0ed7d805b 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -911,16 +911,13 @@ struct fuse_fill_data { unsigned int max_pages; }; -static int fuse_readpages_fill(void *_data, struct page *page) +static int fuse_readpages_fill(struct fuse_fill_data *data, struct page *page) { - struct fuse_fill_data *data = _data; struct fuse_io_args *ia = data->ia; struct fuse_args_pages *ap = &ia->ap; struct inode *inode = data->inode; struct fuse_conn *fc = get_fuse_conn(inode); - fuse_wait_on_page_writeback(inode, page->index); - if (ap->num_pages && (ap->num_pages == fc->max_pages || (ap->num_pages + 1) * PAGE_SIZE > fc->max_read || @@ -942,7 +939,6 @@ static int fuse_readpages_fill(void *_data, struct page *page) return -EIO; } - get_page(page); ap->pages[ap->num_pages] = page; ap->descs[ap->num_pages].length = PAGE_SIZE; ap->num_pages++; @@ -950,15 +946,13 @@ static int fuse_readpages_fill(void *_data, struct page *page) return 0; } -static int fuse_readpages(struct file *file, struct address_space *mapping, - struct list_head *pages, unsigned nr_pages) +static unsigned fuse_readahead(struct file *file, struct address_space *mapping, + pgoff_t start, unsigned nr_pages) { struct inode *inode = mapping->host; struct fuse_conn *fc = get_fuse_conn(inode); struct fuse_fill_data data; - int err; - err = -EIO; if (is_bad_inode(inode)) goto out; @@ -968,19 +962,24 @@ static int fuse_readpages(struct file *file, struct address_space *mapping, data.max_pages = min_t(unsigned int, nr_pages, fc->max_pages); ; data.ia = fuse_io_alloc(NULL, data.max_pages); - err = -ENOMEM; if (!data.ia) goto out; - err = read_cache_pages(mapping, pages, fuse_readpages_fill, &data); - if (!err) { - if (data.ia->ap.num_pages) - fuse_send_readpages(data.ia, file); - else - fuse_io_free(data.ia); + while (nr_pages--) { + struct page *page = readahead_page(mapping, start++); + int err = fuse_readpages_fill(&data, page); + + if (!err) + continue; + nr_pages++; + goto out; } + if (data.ia->ap.num_pages) + fuse_send_readpages(data.ia, file); + else + fuse_io_free(data.ia); out: - return err; + return nr_pages; } static ssize_t fuse_cache_read_iter(struct kiocb *iocb, struct iov_iter *to) @@ -3358,10 +3357,10 @@ static const struct file_operations fuse_file_operations = { static const struct address_space_operations fuse_file_aops = { .readpage = fuse_readpage, + .readahead = fuse_readahead, .writepage = fuse_writepage, .writepages = fuse_writepages, .launder_page = fuse_launder_page, - .readpages = fuse_readpages, .set_page_dirty = __set_page_dirty_nobuffers, .bmap = fuse_bmap, .direct_IO = fuse_direct_IO,