| Message ID | 161161064956.2537118.3354798147866150631.stgit@warthog.procyon.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | Network fs helper library & fscache kiocb API [ver #2] | expand |
For Subject: s/readpage/readpages/ On Mon, Jan 25, 2021 at 09:37:29PM +0000, David Howells wrote: > +int __nfs_readahead_from_fscache(struct nfs_readdesc *desc, > + struct readahead_control *rac) I thought you wanted it called ractl instead of rac? That's what I've been using in new code. > - dfprintk(FSCACHE, "NFS: nfs_getpages_from_fscache (0x%p/%u/0x%p)\n", > - nfs_i_fscache(inode), npages, inode); > + dfprintk(FSCACHE, "NFS: nfs_readahead_from_fscache (0x%p/0x%p)\n", > + nfs_i_fscache(inode), inode); We do have readahead_count() if this is useful information to be logging. > +static inline int nfs_readahead_from_fscache(struct nfs_readdesc *desc, > + struct readahead_control *rac) > { > - if (NFS_I(inode)->fscache) > - return __nfs_readpages_from_fscache(ctx, inode, mapping, pages, > - nr_pages); > + if (NFS_I(rac->mapping->host)->fscache) > + return __nfs_readahead_from_fscache(desc, rac); > return -ENOBUFS; > } Not entirely sure that it's worth having the two functions separated any more. > /* attempt to read as many of the pages as possible from the cache > * - this returns -ENOBUFS immediately if the cookie is negative > */ > - ret = nfs_readpages_from_fscache(desc.ctx, inode, mapping, > - pages, &nr_pages); > + ret = nfs_readahead_from_fscache(&desc, rac); > if (ret == 0) > goto read_complete; /* all pages were read */ > > nfs_pageio_init_read(&desc.pgio, inode, false, > &nfs_async_read_completion_ops); > > - ret = read_cache_pages(mapping, pages, readpage_async_filler, &desc); > + while ((page = readahead_page(rac))) { > + ret = readpage_async_filler(&desc, page); > + put_page(page); > + } I thought with the new API we didn't need to do this kind of thing any more? ie no matter whether fscache is configured in or not, it'll submit the I/Os.
> On Jan 25, 2021, at 8:36 PM, Matthew Wilcox <willy@infradead.org> wrote: > > > For Subject: s/readpage/readpages/ > > On Mon, Jan 25, 2021 at 09:37:29PM +0000, David Howells wrote: >> +int __nfs_readahead_from_fscache(struct nfs_readdesc *desc, >> + struct readahead_control *rac) > > I thought you wanted it called ractl instead of rac? That's what I've > been using in new code. > >> - dfprintk(FSCACHE, "NFS: nfs_getpages_from_fscache (0x%p/%u/0x%p)\n", >> - nfs_i_fscache(inode), npages, inode); >> + dfprintk(FSCACHE, "NFS: nfs_readahead_from_fscache (0x%p/0x%p)\n", >> + nfs_i_fscache(inode), inode); > > We do have readahead_count() if this is useful information to be logging. As a sidebar, the Linux NFS community is transitioning to tracepoints. It would be helpful (but not completely necessary) to use tracepoints in new code instead of printk. >> +static inline int nfs_readahead_from_fscache(struct nfs_readdesc *desc, >> + struct readahead_control *rac) >> { >> - if (NFS_I(inode)->fscache) >> - return __nfs_readpages_from_fscache(ctx, inode, mapping, pages, >> - nr_pages); >> + if (NFS_I(rac->mapping->host)->fscache) >> + return __nfs_readahead_from_fscache(desc, rac); >> return -ENOBUFS; >> } > > Not entirely sure that it's worth having the two functions separated any more. > >> /* attempt to read as many of the pages as possible from the cache >> * - this returns -ENOBUFS immediately if the cookie is negative >> */ >> - ret = nfs_readpages_from_fscache(desc.ctx, inode, mapping, >> - pages, &nr_pages); >> + ret = nfs_readahead_from_fscache(&desc, rac); >> if (ret == 0) >> goto read_complete; /* all pages were read */ >> >> nfs_pageio_init_read(&desc.pgio, inode, false, >> &nfs_async_read_completion_ops); >> >> - ret = read_cache_pages(mapping, pages, readpage_async_filler, &desc); >> + while ((page = readahead_page(rac))) { >> + ret = readpage_async_filler(&desc, page); >> + put_page(page); >> + } > > I thought with the new API we didn't need to do this kind of thing > any more? ie no matter whether fscache is configured in or not, it'll > submit the I/Os. -- Chuck Lever
On Mon, Jan 25, 2021 at 8:37 PM Matthew Wilcox <willy@infradead.org> wrote: > > > For Subject: s/readpage/readpages/ > Fixed > On Mon, Jan 25, 2021 at 09:37:29PM +0000, David Howells wrote: > > +int __nfs_readahead_from_fscache(struct nfs_readdesc *desc, > > + struct readahead_control *rac) > > I thought you wanted it called ractl instead of rac? That's what I've > been using in new code. > Fixed > > - dfprintk(FSCACHE, "NFS: nfs_getpages_from_fscache (0x%p/%u/0x%p)\n", > > - nfs_i_fscache(inode), npages, inode); > > + dfprintk(FSCACHE, "NFS: nfs_readahead_from_fscache (0x%p/0x%p)\n", > > + nfs_i_fscache(inode), inode); > > We do have readahead_count() if this is useful information to be logging. > Right, I used it elsewhere so I'll add here as well. > > +static inline int nfs_readahead_from_fscache(struct nfs_readdesc *desc, > > + struct readahead_control *rac) > > { > > - if (NFS_I(inode)->fscache) > > - return __nfs_readpages_from_fscache(ctx, inode, mapping, pages, > > - nr_pages); > > + if (NFS_I(rac->mapping->host)->fscache) > > + return __nfs_readahead_from_fscache(desc, rac); > > return -ENOBUFS; > > } > > Not entirely sure that it's worth having the two functions separated any more. > Yeah it's questionable so I'll collapse. I'll also do that with nfs_readpage_from_fscache(). > > /* attempt to read as many of the pages as possible from the cache > > * - this returns -ENOBUFS immediately if the cookie is negative > > */ > > - ret = nfs_readpages_from_fscache(desc.ctx, inode, mapping, > > - pages, &nr_pages); > > + ret = nfs_readahead_from_fscache(&desc, rac); > > if (ret == 0) > > goto read_complete; /* all pages were read */ > > > > nfs_pageio_init_read(&desc.pgio, inode, false, > > &nfs_async_read_completion_ops); > > > > - ret = read_cache_pages(mapping, pages, readpage_async_filler, &desc); > > + while ((page = readahead_page(rac))) { > > + ret = readpage_async_filler(&desc, page); > > + put_page(page); > > + } > > I thought with the new API we didn't need to do this kind of thing > any more? ie no matter whether fscache is configured in or not, it'll > submit the I/Os. > We don't. This patchset was only intended as a stepping stone to get the netfs API accepted with minimal invasiveness in NFS. I have another patch which will unconditionally call netfs API but I didn't post it. Since I'm not an NFS maintainer, and maintainer's didn't weigh in on the approach, I opted to go with the least invasive approach. There's an NFS "remote bakeathon" coming up at the end of Feb. That would probably be a good time to get further testing on NFS unconditionally calling the netfs API, and we should be able to cover things like any performance concerns, etc.
On Tue, Jan 26, 2021 at 10:25 AM Chuck Lever <chuck.lever@oracle.com> wrote: > > > > > On Jan 25, 2021, at 8:36 PM, Matthew Wilcox <willy@infradead.org> wrote: > > > > > > For Subject: s/readpage/readpages/ > > > > On Mon, Jan 25, 2021 at 09:37:29PM +0000, David Howells wrote: > >> +int __nfs_readahead_from_fscache(struct nfs_readdesc *desc, > >> + struct readahead_control *rac) > > > > I thought you wanted it called ractl instead of rac? That's what I've > > been using in new code. > > > >> - dfprintk(FSCACHE, "NFS: nfs_getpages_from_fscache (0x%p/%u/0x%p)\n", > >> - nfs_i_fscache(inode), npages, inode); > >> + dfprintk(FSCACHE, "NFS: nfs_readahead_from_fscache (0x%p/0x%p)\n", > >> + nfs_i_fscache(inode), inode); > > > > We do have readahead_count() if this is useful information to be logging. > > As a sidebar, the Linux NFS community is transitioning to tracepoints. > It would be helpful (but not completely necessary) to use tracepoints > in new code instead of printk. > The netfs API has a lot of good tracepoints and to be honest I think we can get rid of fscache's rpcdebug, but let me take a closer look to be sure. I didn't use rpcdebug much, if at all, in the latest rounds of debugging. > > >> +static inline int nfs_readahead_from_fscache(struct nfs_readdesc *desc, > >> + struct readahead_control *rac) > >> { > >> - if (NFS_I(inode)->fscache) > >> - return __nfs_readpages_from_fscache(ctx, inode, mapping, pages, > >> - nr_pages); > >> + if (NFS_I(rac->mapping->host)->fscache) > >> + return __nfs_readahead_from_fscache(desc, rac); > >> return -ENOBUFS; > >> } > > > > Not entirely sure that it's worth having the two functions separated any more. > > > >> /* attempt to read as many of the pages as possible from the cache > >> * - this returns -ENOBUFS immediately if the cookie is negative > >> */ > >> - ret = nfs_readpages_from_fscache(desc.ctx, inode, mapping, > >> - pages, &nr_pages); > >> + ret = nfs_readahead_from_fscache(&desc, rac); > >> if (ret == 0) > >> goto read_complete; /* all pages were read */ > >> > >> nfs_pageio_init_read(&desc.pgio, inode, false, > >> &nfs_async_read_completion_ops); > >> > >> - ret = read_cache_pages(mapping, pages, readpage_async_filler, &desc); > >> + while ((page = readahead_page(rac))) { > >> + ret = readpage_async_filler(&desc, page); > >> + put_page(page); > >> + } > > > > I thought with the new API we didn't need to do this kind of thing > > any more? ie no matter whether fscache is configured in or not, it'll > > submit the I/Os. > > -- > Chuck Lever > > >
diff --git a/fs/nfs/file.c b/fs/nfs/file.c index 63940a7a70be..ebcaa164db5f 100644 --- a/fs/nfs/file.c +++ b/fs/nfs/file.c @@ -515,7 +515,7 @@ static void nfs_swap_deactivate(struct file *file) const struct address_space_operations nfs_file_aops = { .readpage = nfs_readpage, - .readpages = nfs_readpages, + .readahead = nfs_readahead, .set_page_dirty = __set_page_dirty_nobuffers, .writepage = nfs_writepage, .writepages = nfs_writepages, diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c index d3445bb1cc9c..b432bf931470 100644 --- a/fs/nfs/fscache.c +++ b/fs/nfs/fscache.c @@ -499,51 +499,18 @@ int __nfs_readpage_from_fscache(struct file *filp, /* * Retrieve a set of pages from fscache */ -int __nfs_readpages_from_fscache(struct nfs_open_context *ctx, - struct inode *inode, - struct address_space *mapping, - struct list_head *pages, - unsigned *nr_pages) +int __nfs_readahead_from_fscache(struct nfs_readdesc *desc, + struct readahead_control *rac) { - unsigned npages = *nr_pages; - int ret; + struct inode *inode = rac->mapping->host; - dfprintk(FSCACHE, "NFS: nfs_getpages_from_fscache (0x%p/%u/0x%p)\n", - nfs_i_fscache(inode), npages, inode); - - ret = fscache_read_or_alloc_pages(nfs_i_fscache(inode), - mapping, pages, nr_pages, - nfs_readpage_from_fscache_complete, - ctx, - mapping_gfp_mask(mapping)); - if (*nr_pages < npages) - nfs_add_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_OK, - npages); - if (*nr_pages > 0) - nfs_add_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_FAIL, - *nr_pages); + dfprintk(FSCACHE, "NFS: nfs_readahead_from_fscache (0x%p/0x%p)\n", + nfs_i_fscache(inode), inode); - switch (ret) { - case 0: /* read submitted to the cache for all pages */ - BUG_ON(!list_empty(pages)); - BUG_ON(*nr_pages != 0); - dfprintk(FSCACHE, - "NFS: nfs_getpages_from_fscache: submitted\n"); + netfs_readahead(rac, &nfs_fscache_req_ops, desc); - return ret; - - case -ENOBUFS: /* some pages aren't cached and can't be */ - case -ENODATA: /* some pages aren't cached */ - dfprintk(FSCACHE, - "NFS: nfs_getpages_from_fscache: no page: %d\n", ret); - return 1; - - default: - dfprintk(FSCACHE, - "NFS: nfs_getpages_from_fscache: ret %d\n", ret); - } - - return ret; + /* FIXME: NFSIOS_NFSIOS_FSCACHE_ stats */ + return 0; } /* diff --git a/fs/nfs/fscache.h b/fs/nfs/fscache.h index 4a76a5f31772..d095e513af12 100644 --- a/fs/nfs/fscache.h +++ b/fs/nfs/fscache.h @@ -98,11 +98,10 @@ extern int nfs_fscache_release_page(struct page *, gfp_t); extern int __nfs_readpage_from_fscache(struct file *filp, struct page *page, struct nfs_readdesc *desc); -extern int __nfs_readpages_from_fscache(struct nfs_open_context *, - struct inode *, struct address_space *, - struct list_head *, unsigned *); -extern void __nfs_readpage_to_fscache(struct inode *, struct page *, int); - +extern int __nfs_readahead_from_fscache(struct nfs_readdesc *desc, + struct readahead_control *rac); +extern void __nfs_read_completion_to_fscache(struct nfs_pgio_header *hdr, + unsigned long bytes); /* * wait for a page to complete writing to the cache */ @@ -139,15 +138,11 @@ static inline int nfs_readpage_from_fscache(struct file *filp, /* * Retrieve a set of pages from an inode data storage object. */ -static inline int nfs_readpages_from_fscache(struct nfs_open_context *ctx, - struct inode *inode, - struct address_space *mapping, - struct list_head *pages, - unsigned *nr_pages) +static inline int nfs_readahead_from_fscache(struct nfs_readdesc *desc, + struct readahead_control *rac) { - if (NFS_I(inode)->fscache) - return __nfs_readpages_from_fscache(ctx, inode, mapping, pages, - nr_pages); + if (NFS_I(rac->mapping->host)->fscache) + return __nfs_readahead_from_fscache(desc, rac); return -ENOBUFS; } @@ -217,11 +212,8 @@ static inline int nfs_readpage_from_fscache(struct file *filp, { return -ENOBUFS; } -static inline int nfs_readpages_from_fscache(struct nfs_open_context *ctx, - struct inode *inode, - struct address_space *mapping, - struct list_head *pages, - unsigned *nr_pages) +static inline int nfs_readahead_from_fscache(struct nfs_readdesc *desc, + struct readahead_control *rac) { return -ENOBUFS; } diff --git a/fs/nfs/read.c b/fs/nfs/read.c index 7a76ab474fe0..da44ce68488c 100644 --- a/fs/nfs/read.c +++ b/fs/nfs/read.c @@ -390,50 +390,50 @@ int nfs_readpage(struct file *filp, struct page *page) return ret; } -int nfs_readpages(struct file *filp, struct address_space *mapping, - struct list_head *pages, unsigned nr_pages) +void nfs_readahead(struct readahead_control *rac) { struct nfs_readdesc desc; - struct inode *inode = mapping->host; + struct inode *inode = rac->mapping->host; + struct page *page; int ret; - dprintk("NFS: nfs_readpages (%s/%Lu %d)\n", - inode->i_sb->s_id, - (unsigned long long)NFS_FILEID(inode), - nr_pages); + dprintk("NFS: %s (%s/%llu %lld)\n", __func__, + inode->i_sb->s_id, + (unsigned long long)NFS_FILEID(inode), + readahead_length(rac)); nfs_inc_stats(inode, NFSIOS_VFSREADPAGES); ret = -ESTALE; if (NFS_STALE(inode)) - goto out; + return; - if (filp == NULL) { + if (rac->file == NULL) { ret = -EBADF; desc.ctx = nfs_find_open_context(inode, NULL, FMODE_READ); if (desc.ctx == NULL) - goto out; + return; } else - desc.ctx = get_nfs_open_context(nfs_file_open_context(filp)); + desc.ctx = get_nfs_open_context(nfs_file_open_context(rac->file)); /* attempt to read as many of the pages as possible from the cache * - this returns -ENOBUFS immediately if the cookie is negative */ - ret = nfs_readpages_from_fscache(desc.ctx, inode, mapping, - pages, &nr_pages); + ret = nfs_readahead_from_fscache(&desc, rac); if (ret == 0) goto read_complete; /* all pages were read */ nfs_pageio_init_read(&desc.pgio, inode, false, &nfs_async_read_completion_ops); - ret = read_cache_pages(mapping, pages, readpage_async_filler, &desc); + while ((page = readahead_page(rac))) { + ret = readpage_async_filler(&desc, page); + put_page(page); + } nfs_pageio_complete_read(&desc.pgio, inode); read_complete: put_nfs_open_context(desc.ctx); -out: - return ret; } int __init nfs_init_readpagecache(void) diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h index 3cfcf219e96b..968c79b1b09b 100644 --- a/include/linux/nfs_fs.h +++ b/include/linux/nfs_fs.h @@ -568,8 +568,7 @@ nfs_have_writebacks(struct inode *inode) * linux/fs/nfs/read.c */ extern int nfs_readpage(struct file *, struct page *); -extern int nfs_readpages(struct file *, struct address_space *, - struct list_head *, unsigned); +extern void nfs_readahead(struct readahead_control *rac); /* * inline functions diff --git a/include/linux/nfs_iostat.h b/include/linux/nfs_iostat.h index 027874c36c88..8baf8fb7551d 100644 --- a/include/linux/nfs_iostat.h +++ b/include/linux/nfs_iostat.h @@ -53,7 +53,7 @@ * NFS page counters * * These count the number of pages read or written via nfs_readpage(), - * nfs_readpages(), or their write equivalents. + * nfs_readahead(), or their write equivalents. * * NB: When adding new byte counters, please include the measured * units in the name of each byte counter to help users of this