On Sat, Oct 9, 2021 at 8:35 AM David Wysochanski <dwysocha@xxxxxxxxxx> wrote:
>
> On Fri, Oct 8, 2021 at 10:10 AM Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
> >
> >
> >
> > > On Oct 7, 2021, at 7:32 PM, Dave Wysochanski <dwysocha@xxxxxxxxxx> wrote:
> > >
> > > Convert to the new VM readahead() API which is the preferred API
> > > to read multiple pages, and rename the NFSIOS_* counters and the
> > > tracepoint as needed.
> > >
> > > Signed-off-by: Dave Wysochanski <dwysocha@xxxxxxxxxx>
> > > ---
> > > fs/nfs/file.c | 2 +-
> > > fs/nfs/nfstrace.h | 2 +-
> > > fs/nfs/read.c | 21 +++++++++++++++------
> > > include/linux/nfs_fs.h | 3 +--
> > > include/linux/nfs_iostat.h | 6 +++---
> > > 5 files changed, 21 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > > index 209dac208477..cc76d17fa97f 100644
> > > --- a/fs/nfs/file.c
> > > +++ b/fs/nfs/file.c
> > > @@ -519,7 +519,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/nfstrace.h b/fs/nfs/nfstrace.h
> > > index 78b0f649dd09..d2b2080765a6 100644
> > > --- a/fs/nfs/nfstrace.h
> > > +++ b/fs/nfs/nfstrace.h
> > > @@ -915,7 +915,7 @@
> > > )
> > > );
> > >
> > > -TRACE_EVENT(nfs_aops_readpages,
> > > +TRACE_EVENT(nfs_aops_readahead,
> >
> > In v2 and v3 of my patch, this tracepoint has already been
> > renamed to nfs_aop_readahead.
> >
>
> Ack.
>
> >
> > > TP_PROTO(
> > > const struct inode *inode,
> > > unsigned int nr_pages
> > > diff --git a/fs/nfs/read.c b/fs/nfs/read.c
> > > index 927504605e0f..5c2aab47cf1d 100644
> > > --- a/fs/nfs/read.c
> > > +++ b/fs/nfs/read.c
> > > @@ -395,15 +395,19 @@ int nfs_readpage(struct file *file, struct page *page)
> > > return ret;
> > > }
> > >
> > > -int nfs_readpages(struct file *file, struct address_space *mapping,
> > > - struct list_head *pages, unsigned nr_pages)
> > > +void nfs_readahead(struct readahead_control *ractl)
> > > {
> > > + struct file *file = ractl->file;
> > > + struct address_space *mapping = ractl->mapping;
> > > + struct page *page;
> > > + unsigned int nr_pages = readahead_count(ractl);
> > > +
> > > struct nfs_readdesc desc;
> > > struct inode *inode = mapping->host;
> > > int ret;
> > >
> > > - trace_nfs_aops_readpages(inode, nr_pages);
> > > - nfs_inc_stats(inode, NFSIOS_VFSREADPAGES);
> > > + trace_nfs_aops_readahead(inode, nr_pages);
> > > + nfs_inc_stats(inode, NFSIOS_VFSREADAHEAD);
> > >
> > > ret = -ESTALE;
> > > if (NFS_STALE(inode))
> > > @@ -420,13 +424,18 @@ int nfs_readpages(struct file *file, struct address_space *mapping,
> > > nfs_pageio_init_read(&desc.pgio, inode, false,
> > > &nfs_async_read_completion_ops);
> > >
> > > - ret = read_cache_pages(mapping, pages, readpage_async_filler, &desc);
> > > + ret = 0;
> > > + while (!ret && (page = readahead_page(ractl))) {
> > > + prefetchw(&page->flags);
> > > + ret = readpage_async_filler(&desc, page);
> > > + put_page(page);
> > > + }
> > >
> > > nfs_pageio_complete_read(&desc.pgio);
> > >
> > > put_nfs_open_context(desc.ctx);
> > > out:
> > > - return ret;
> > > + return;
> > > }
> > >
> > > int __init nfs_init_readpagecache(void)
> > > diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> > > index b9a8b925db43..6cbe3f2c5669 100644
> > > --- a/include/linux/nfs_fs.h
> > > +++ b/include/linux/nfs_fs.h
> > > @@ -580,8 +580,7 @@ extern int nfs_access_get_cached(struct inode *inode, const struct cred *cred, s
> > > * 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 *);
> > >
> > > /*
> > > * inline functions
> > > diff --git a/include/linux/nfs_iostat.h b/include/linux/nfs_iostat.h
> > > index 027874c36c88..418145f23700 100644
> > > --- a/include/linux/nfs_iostat.h
> > > +++ b/include/linux/nfs_iostat.h
> > > @@ -22,7 +22,7 @@
> > > #ifndef _LINUX_NFS_IOSTAT
> > > #define _LINUX_NFS_IOSTAT
> > >
> > > -#define NFS_IOSTAT_VERS "1.1"
> > > +#define NFS_IOSTAT_VERS "1.2"
> > >
> > > /*
> > > * NFS byte counters
> > > @@ -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
> > > @@ -98,7 +98,7 @@ enum nfs_stat_eventcounters {
> > > NFSIOS_VFSACCESS,
> > > NFSIOS_VFSUPDATEPAGE,
> > > NFSIOS_VFSREADPAGE,
> > > - NFSIOS_VFSREADPAGES,
> > > + NFSIOS_VFSREADAHEAD,
> >
> > I'm wondering if you should add NFSIOS_VFSREADAHEAD
> > but leave NFSIOS_VFSREADPAGES? I don't remember exactly
> > how the mountstats API versioning is supposed to work.
> >
>
> I don't think we need to keep this but correct me if I'm wrong.
> That would mean we would have a 0 count for later kernels and I'm not
> sure about that given the similarities.
It may matter in the case of "old nfs-utils" and newer kernel.
Do we want nfs-utils to display 0 for VFSREADPAGES count in that case?
Or would users be more confused by the fact this value is always 0 and
there's no notion of "VFSREADAHEAD" counts in older nfs-utils?
Would users be more surprised by 0 here or the fact there's a more
subtle change in the interface?
I would say the permanent 0 value would be most surprising to users so
I'd shy away from it.
> You can see my nfs-utils patch for the attempt to use the versioning
> in nfs-iostat.
>
> >
> > > NFSIOS_VFSWRITEPAGE,
> > > NFSIOS_VFSWRITEPAGES,
> > > NFSIOS_VFSGETDENTS,
> > > --
> > > 1.8.3.1
> > >
> >
> > --
> > Chuck Lever
> >
> >
> >
