[PATCH 0/1] Convert nfs_readpages() to nfs_readahead()

[PATCH 0/1] Convert nfs_readpages() to nfs_readahead()

[Dave Wysochanski]

This patch converts nfs_readpages() to nfs_readahead().  It was
applied on top of the set I just posted [1] plus Chuck's v1 patch to
convert dfprintks to tracepoints [2].  It is being tested on top of
these patches at BakeAThon against various servers, cthon, xfstests,
and my fscache unit tests.  Note that Chuck may post a v3 of his
patch and this would need rebased on that patch.

Note that a nfs-utils patch will need to be done to properly account
for the changed NFSIOS_* counters.

I would consider this a "nice to have" for this cycle but I wanted
to get it out there sooner than later.  As far as I know this has
been an outstanding item for the NFS client for a while and the
fscache fallback IO API clears the way for this patch.

[1] https://marc.info/?l=linux-nfs&m=163364580324243&w=2
[2] https://marc.info/?l=linux-nfs&m=163345402812426&w=2


Dave Wysochanski (1):
  NFS: Convert from readpages() to readahead()

 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(-)

-- 
1.8.3.1

[PATCH 1/1] NFS: Convert from readpages() to readahead()

[Dave Wysochanski]

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@redhat.com>
---
 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,
 		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,
 	NFSIOS_VFSWRITEPAGE,
 	NFSIOS_VFSWRITEPAGES,
 	NFSIOS_VFSGETDENTS,
-- 
1.8.3.1

Re: [PATCH 1/1] NFS: Convert from readpages() to readahead()

[Chuck Lever III]

> On Oct 7, 2021, at 7:32 PM, Dave Wysochanski <dwysocha@redhat.com> 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@redhat.com>
> ---
> 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.


> 		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.


> 	NFSIOS_VFSWRITEPAGE,
> 	NFSIOS_VFSWRITEPAGES,
> 	NFSIOS_VFSGETDENTS,
> -- 
> 1.8.3.1
> 

--
Chuck Lever

Re: [PATCH 1/1] NFS: Convert from readpages() to readahead()

[David Wysochanski]

On Fri, Oct 8, 2021 at 10:10 AM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Oct 7, 2021, at 7:32 PM, Dave Wysochanski <dwysocha@redhat.com> 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@redhat.com>
> > ---
> > 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.
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
>
>
>

Re: [PATCH 1/1] NFS: Convert from readpages() to readahead()

[David Wysochanski]

On Sat, Oct 9, 2021 at 8:35 AM David Wysochanski <dwysocha@redhat.com> wrote:
>
> On Fri, Oct 8, 2021 at 10:10 AM Chuck Lever III <chuck.lever@oracle.com> wrote:
> >
> >
> >
> > > On Oct 7, 2021, at 7:32 PM, Dave Wysochanski <dwysocha@redhat.com> 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@redhat.com>
> > > ---
> > > 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
> >
> >
> >

Re: [PATCH 1/1] NFS: Convert from readpages() to readahead()

[Trond Myklebust]

On Fri, 2021-10-08 at 14:09 +0000, Chuck Lever III wrote:
> 
> 
> > On Oct 7, 2021, at 7:32 PM, Dave Wysochanski <dwysocha@redhat.com>
> > 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@redhat.com>
> > ---
> > 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.

Does that mean you're going to resend, Chuck? All I have is your v1..



-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 1/1] NFS: Convert from readpages() to readahead()

[Chuck Lever III]

> On Oct 15, 2021, at 3:42 AM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Fri, 2021-10-08 at 14:09 +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Oct 7, 2021, at 7:32 PM, Dave Wysochanski <dwysocha@redhat.com>
>>> 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@redhat.com>
>>> ---
>>> 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.
> 
> Does that mean you're going to resend, Chuck?

I re-sent this one patch a few days ago, but I can re-send the whole series again to be sure everyone has the latest.