On Mon, Jan 25, 2021 at 8:37 PM Matthew Wilcox [off-list ref] wrote:

For Subject: s/readpage/readpages/

Fixed

On Mon, Jan 25, 2021 at 09:37:29PM +0000, David Howells wrote:

quoted

+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

quoted

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

quoted

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

quoted

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