From: "Matthew Wilcox (Oracle)" <[email protected]>
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) <[email protected]>
---
fs/fuse/file.c | 35 +++++++++++++++++------------------
1 file changed, 17 insertions(+), 18 deletions(-)
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,
--
2.24.1
On Fri, Jan 24, 2020 at 05:35:52PM -0800, Matthew Wilcox wrote:
> From: "Matthew Wilcox (Oracle)" <[email protected]>
>
> 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) <[email protected]>
.....
> @@ -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.
--
Dave Chinner
[email protected]
On Sat, Jan 25, 2020 at 2:36 AM Matthew Wilcox <[email protected]> wrote:
>
> From: "Matthew Wilcox (Oracle)" <[email protected]>
>
> 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 <[email protected]> wrote:
> >
> > From: "Matthew Wilcox (Oracle)" <[email protected]>
> >
> > 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.
--
Dave Chinner
[email protected]