[RFC PATCH v2 0/14] Convert NFS to new FS-Cache iter API

[RFC PATCH v2 0/14] Convert NFS to new FS-Cache iter API

[Dave Wysochanski]

These patches update the nfs client to use the new FS-Cache API and are at:
https://github.com/DaveWysochanskiRH/kernel/tree/fscache-iter-nfs
https://github.com/DaveWysochanskiRH/kernel/commit/467796a0c75d64401c8963e9266f27d87863ed3e

They are based on David Howells fscache-iter tree at 757ac8b16a0edd3befa15c9bdcb2ab3811be945d
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=fscache-iter&id=757ac8b16a0edd3befa15c9bdcb2ab3811be945d

The first 5 patches refactor some of the NFS read code to facilitate
re-use, while the last 9 patches do the conversion to the new FS-Cache
API.

Changes since v1
* Refactor and general cleanup of read code paths
* Fixes a few build errors when NFS_FSCACHE is not configured
* Fixes directIO data corruption (needed fscache_invalidate() on directIO write)

Summary
* Takes a "least invasive to existing code" approach
  * most fscache bits stay fs/nfs/fscache.[ch]
  * only enable fscache for files open for READ (disable for WRITE)
  * may not be the best approach (see future patcheset items below)
* Basically works and passes a series of tests (see below)
  * No kernel oopses or hangs seen with tests run

Future patchset items
* Call fscache_read_helper_* directly rather than isolation into
  fs/nfs/fscache.c, similar to the AFS conversion
* Add write-through support
  * Eliminate on/off switching of fscache based on whether a
  file is open for read or write
  * TODO: Work out any limitations of NFS versions
* Rework dfprintks and/or add ftrace points
  * fscache/cachefiles has 'debug' logging similar to rpcdebug
  * convert IO path to ftrace, leave non-IO path as dfprintk?

Tests run
* A few individual NFS/fscache unit tests: PASS
* cthon04 (fsc/non-fsc, vers=3,4.0,4.1,4.2, sec=sys): PASS
* iozone tests (fsc, vers=3,4.0,4.1,4.2, sec=sys): PASS
* xfstests/generic (fsc,vers=4.2): 17/151 (Failed/Ran) 595/444 (Total/NotRan)
Failures: generic/029 generic/030 generic/240 generic/294 generic/306 generic/356 generic/357 generic/452 generic/472 generic/493 generic/494 generic/495 generic/496 generic/497 generic/554 generic/568 generic/569
Failed 17 of 595 tests
* kernel build: FAIL (linking module fails; truncate / invalidate related?)

Test not run
* error injections (for example, connection disruptions, server errors during IO, etc)
* pNFS
* many process mixed read/write on same file
* sec=krb5

Dave Wysochanski (14):
  NFS: Clean up nfs_readpage() and nfs_readpages()
  NFS: In nfs_readpage() only increment NFSIOS_READPAGES when read
    succeeds
  NFS: Refactor nfs_readpage() and nfs_readpage_async() to use
    nfs_readdesc
  NFS: Call readpage_async_filler() from nfs_readpage_async()
  NFS: Add nfs_pageio_complete_read() and remove nfs_readpage_async()
  NFS: Allow internal use of read structs and functions
  NFS: Convert nfs_readpage() and readpages() to new fscache API
  NFS: Convert fscache_acquire_cookie and fscache_relinquish_cookie
  NFS: Only use and unuse an fscache cookie a single time based on
    NFS_INO_FSCACHE
  NFS: Convert fscache invalidation and update aux_data and i_size
  NFS: Call nfs_fscache_invalidate() when write extends the size of the
    file
  NFS: Invalidate fscache for direct writes
  NFS: Call fscache_resize_cookie() when inode size changes due to
    setattr
  NFS: Allow NFS use of new fscache API in build

 fs/nfs/Kconfig           |   2 +-
 fs/nfs/direct.c          |   2 +
 fs/nfs/file.c            |  20 +--
 fs/nfs/fscache-index.c   |  94 --------------
 fs/nfs/fscache.c         | 309 +++++++++++++++++++++++------------------------
 fs/nfs/fscache.h         |  99 ++++++---------
 fs/nfs/inode.c           |   4 +-
 fs/nfs/internal.h        |   9 ++
 fs/nfs/nfs4proc.c        |   2 +-
 fs/nfs/pagelist.c        |   1 +
 fs/nfs/read.c            | 217 +++++++++++++++------------------
 fs/nfs/write.c           |   3 +-
 include/linux/nfs_fs.h   |   2 -
 include/linux/nfs_page.h |   1 +
 include/linux/nfs_xdr.h  |   1 +
 15 files changed, 316 insertions(+), 450 deletions(-)

-- 
1.8.3.1

[RFC PATCH v2 01/14] NFS: Clean up nfs_readpage() and nfs_readpages()

[Dave Wysochanski]

In prep for the new fscache netfs API, refactor nfs_readpage()
and nfs_readpages() for future patches.  No functional change.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/read.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index eb854f1f86e2..a05fb3904ddf 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -310,11 +310,11 @@ static void nfs_readpage_result(struct rpc_task *task,
  *  -	The error flag is set for this page. This happens only when a
  *	previous async read operation failed.
  */
-int nfs_readpage(struct file *file, struct page *page)
+int nfs_readpage(struct file *filp, struct page *page)
 {
 	struct nfs_open_context *ctx;
 	struct inode *inode = page_file_mapping(page)->host;
-	int		error;
+	int ret;
 
 	dprintk("NFS: nfs_readpage (%p %ld@%lu)\n",
 		page, PAGE_SIZE, page_index(page));
@@ -328,43 +328,43 @@ int nfs_readpage(struct file *file, struct page *page)
 	 * be any new pending writes generated at this point
 	 * for this page (other pages can be written to).
 	 */
-	error = nfs_wb_page(inode, page);
-	if (error)
+	ret = nfs_wb_page(inode, page);
+	if (ret)
 		goto out_unlock;
 	if (PageUptodate(page))
 		goto out_unlock;
 
-	error = -ESTALE;
+	ret = -ESTALE;
 	if (NFS_STALE(inode))
 		goto out_unlock;
 
-	if (file == NULL) {
-		error = -EBADF;
+	if (filp == NULL) {
+		ret = -EBADF;
 		ctx = nfs_find_open_context(inode, NULL, FMODE_READ);
 		if (ctx == NULL)
 			goto out_unlock;
 	} else
-		ctx = get_nfs_open_context(nfs_file_open_context(file));
+		ctx = get_nfs_open_context(nfs_file_open_context(filp));
 
 	if (!IS_SYNC(inode)) {
-		error = nfs_readpage_from_fscache(ctx, inode, page);
-		if (error == 0)
+		ret = nfs_readpage_from_fscache(ctx, inode, page);
+		if (ret == 0)
 			goto out;
 	}
 
 	xchg(&ctx->error, 0);
-	error = nfs_readpage_async(ctx, inode, page);
-	if (!error) {
-		error = wait_on_page_locked_killable(page);
-		if (!PageUptodate(page) && !error)
-			error = xchg(&ctx->error, 0);
+	ret = nfs_readpage_async(ctx, inode, page);
+	if (!ret) {
+		ret = wait_on_page_locked_killable(page);
+		if (!PageUptodate(page) && !ret)
+			ret = xchg(&ctx->error, 0);
 	}
 out:
 	put_nfs_open_context(ctx);
-	return error;
+	return ret;
 out_unlock:
 	unlock_page(page);
-	return error;
+	return ret;
 }
 
 struct nfs_readdesc {
@@ -409,12 +409,10 @@ int nfs_readpages(struct file *filp, struct address_space *mapping,
 {
 	struct nfs_pageio_descriptor pgio;
 	struct nfs_pgio_mirror *pgm;
-	struct nfs_readdesc desc = {
-		.pgio = &pgio,
-	};
+	struct nfs_readdesc desc;
 	struct inode *inode = mapping->host;
 	unsigned long npages;
-	int ret = -ESTALE;
+	int ret;
 
 	dprintk("NFS: nfs_readpages (%s/%Lu %d)\n",
 			inode->i_sb->s_id,
@@ -422,13 +420,15 @@ int nfs_readpages(struct file *filp, struct address_space *mapping,
 			nr_pages);
 	nfs_inc_stats(inode, NFSIOS_VFSREADPAGES);
 
+	ret = -ESTALE;
 	if (NFS_STALE(inode))
 		goto out;
 
 	if (filp == NULL) {
+		ret = -EBADF;
 		desc.ctx = nfs_find_open_context(inode, NULL, FMODE_READ);
 		if (desc.ctx == NULL)
-			return -EBADF;
+			goto out;
 	} else
 		desc.ctx = get_nfs_open_context(nfs_file_open_context(filp));
 
@@ -440,6 +440,7 @@ int nfs_readpages(struct file *filp, struct address_space *mapping,
 	if (ret == 0)
 		goto read_complete; /* all pages were read */
 
+	desc.pgio = &pgio;
 	nfs_pageio_init_read(&pgio, inode, false,
 			     &nfs_async_read_completion_ops);
 
-- 
1.8.3.1

[RFC PATCH v2 02/14] NFS: In nfs_readpage() only increment NFSIOS_READPAGES when read succeeds

[Dave Wysochanski]

There is a small inconsistency with nfs_readpage() vs nfs_readpages() with
regards to NFSIOS_READPAGES.  In readpage we unconditionally increment
NFSIOS_READPAGES at the top, which means even if the read fails.  In
readpages, we increment NFSIOS_READPAGES at the bottom based on how
many pages were successfully read.  Change readpage to be consistent with
readpages and so NFSIOS_READPAGES only reflects successful, non-fscache
reads.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/read.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index a05fb3904ddf..1153c4e0a155 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -319,7 +319,6 @@ int nfs_readpage(struct file *filp, struct page *page)
 	dprintk("NFS: nfs_readpage (%p %ld@%lu)\n",
 		page, PAGE_SIZE, page_index(page));
 	nfs_inc_stats(inode, NFSIOS_VFSREADPAGE);
-	nfs_add_stats(inode, NFSIOS_READPAGES, 1);
 
 	/*
 	 * Try to flush any pending writes to the file..
@@ -359,6 +358,7 @@ int nfs_readpage(struct file *filp, struct page *page)
 		if (!PageUptodate(page) && !ret)
 			ret = xchg(&ctx->error, 0);
 	}
+	nfs_add_stats(inode, NFSIOS_READPAGES, 1);
 out:
 	put_nfs_open_context(ctx);
 	return ret;
-- 
1.8.3.1

[RFC PATCH v2 03/14] NFS: Refactor nfs_readpage() and nfs_readpage_async() to use nfs_readdesc

[Dave Wysochanski]

Both nfs_readpage() and nfs_readpages() use similar code.
This patch should be no functional change, and refactors
nfs_readpage_async() to use nfs_readdesc to enable future
merging of nfs_readpage_async() and nfs_readpage_async_filler().

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/read.c          | 62 ++++++++++++++++++++++++--------------------------
 include/linux/nfs_fs.h |  3 +--
 2 files changed, 31 insertions(+), 34 deletions(-)

diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 1153c4e0a155..5fda30742a32 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -114,18 +114,23 @@ static void nfs_readpage_release(struct nfs_page *req, int error)
 	nfs_release_request(req);
 }
 
-int nfs_readpage_async(struct nfs_open_context *ctx, struct inode *inode,
+struct nfs_readdesc {
+	struct nfs_pageio_descriptor pgio;
+	struct nfs_open_context *ctx;
+};
+
+int nfs_readpage_async(void *data, struct inode *inode,
 		       struct page *page)
 {
+	struct nfs_readdesc *desc = (struct nfs_readdesc *)data;
 	struct nfs_page	*new;
 	unsigned int len;
-	struct nfs_pageio_descriptor pgio;
 	struct nfs_pgio_mirror *pgm;
 
 	len = nfs_page_length(page);
 	if (len == 0)
 		return nfs_return_empty_page(page);
-	new = nfs_create_request(ctx, page, 0, len);
+	new = nfs_create_request(desc->ctx, page, 0, len);
 	if (IS_ERR(new)) {
 		unlock_page(page);
 		return PTR_ERR(new);
@@ -133,21 +138,21 @@ int nfs_readpage_async(struct nfs_open_context *ctx, struct inode *inode,
 	if (len < PAGE_SIZE)
 		zero_user_segment(page, len, PAGE_SIZE);
 
-	nfs_pageio_init_read(&pgio, inode, false,
+	nfs_pageio_init_read(&desc->pgio, inode, false,
 			     &nfs_async_read_completion_ops);
-	if (!nfs_pageio_add_request(&pgio, new)) {
+	if (!nfs_pageio_add_request(&desc->pgio, new)) {
 		nfs_list_remove_request(new);
-		nfs_readpage_release(new, pgio.pg_error);
+		nfs_readpage_release(new, desc->pgio.pg_error);
 	}
-	nfs_pageio_complete(&pgio);
+	nfs_pageio_complete(&desc->pgio);
 
 	/* It doesn't make sense to do mirrored reads! */
-	WARN_ON_ONCE(pgio.pg_mirror_count != 1);
+	WARN_ON_ONCE(desc->pgio.pg_mirror_count != 1);
 
-	pgm = &pgio.pg_mirrors[0];
+	pgm = &desc->pgio.pg_mirrors[0];
 	NFS_I(inode)->read_io += pgm->pg_bytes_written;
 
-	return pgio.pg_error < 0 ? pgio.pg_error : 0;
+	return desc->pgio.pg_error < 0 ? desc->pgio.pg_error : 0;
 }
 
 static void nfs_page_group_set_uptodate(struct nfs_page *req)
@@ -312,7 +317,7 @@ static void nfs_readpage_result(struct rpc_task *task,
  */
 int nfs_readpage(struct file *filp, struct page *page)
 {
-	struct nfs_open_context *ctx;
+	struct nfs_readdesc desc;
 	struct inode *inode = page_file_mapping(page)->host;
 	int ret;
 
@@ -339,39 +344,34 @@ int nfs_readpage(struct file *filp, struct page *page)
 
 	if (filp == NULL) {
 		ret = -EBADF;
-		ctx = nfs_find_open_context(inode, NULL, FMODE_READ);
-		if (ctx == NULL)
+		desc.ctx = nfs_find_open_context(inode, NULL, FMODE_READ);
+		if (desc.ctx == NULL)
 			goto out_unlock;
 	} else
-		ctx = get_nfs_open_context(nfs_file_open_context(filp));
+		desc.ctx = get_nfs_open_context(nfs_file_open_context(filp));
 
 	if (!IS_SYNC(inode)) {
-		ret = nfs_readpage_from_fscache(ctx, inode, page);
+		ret = nfs_readpage_from_fscache(desc.ctx, inode, page);
 		if (ret == 0)
 			goto out;
 	}
 
-	xchg(&ctx->error, 0);
-	ret = nfs_readpage_async(ctx, inode, page);
+	xchg(&desc.ctx->error, 0);
+	ret = nfs_readpage_async(&desc, inode, page);
 	if (!ret) {
 		ret = wait_on_page_locked_killable(page);
 		if (!PageUptodate(page) && !ret)
-			ret = xchg(&ctx->error, 0);
+			ret = xchg(&desc.ctx->error, 0);
 	}
 	nfs_add_stats(inode, NFSIOS_READPAGES, 1);
 out:
-	put_nfs_open_context(ctx);
+	put_nfs_open_context(desc.ctx);
 	return ret;
 out_unlock:
 	unlock_page(page);
 	return ret;
 }
 
-struct nfs_readdesc {
-	struct nfs_pageio_descriptor *pgio;
-	struct nfs_open_context *ctx;
-};
-
 static int
 readpage_async_filler(void *data, struct page *page)
 {
@@ -390,9 +390,9 @@ struct nfs_readdesc {
 
 	if (len < PAGE_SIZE)
 		zero_user_segment(page, len, PAGE_SIZE);
-	if (!nfs_pageio_add_request(desc->pgio, new)) {
+	if (!nfs_pageio_add_request(&desc->pgio, new)) {
 		nfs_list_remove_request(new);
-		error = desc->pgio->pg_error;
+		error = desc->pgio.pg_error;
 		nfs_readpage_release(new, error);
 		goto out;
 	}
@@ -407,7 +407,6 @@ struct nfs_readdesc {
 int nfs_readpages(struct file *filp, struct address_space *mapping,
 		struct list_head *pages, unsigned nr_pages)
 {
-	struct nfs_pageio_descriptor pgio;
 	struct nfs_pgio_mirror *pgm;
 	struct nfs_readdesc desc;
 	struct inode *inode = mapping->host;
@@ -440,17 +439,16 @@ int nfs_readpages(struct file *filp, struct address_space *mapping,
 	if (ret == 0)
 		goto read_complete; /* all pages were read */
 
-	desc.pgio = &pgio;
-	nfs_pageio_init_read(&pgio, inode, false,
+	nfs_pageio_init_read(&desc.pgio, inode, false,
 			     &nfs_async_read_completion_ops);
 
 	ret = read_cache_pages(mapping, pages, readpage_async_filler, &desc);
-	nfs_pageio_complete(&pgio);
+	nfs_pageio_complete(&desc.pgio);
 
 	/* It doesn't make sense to do mirrored reads! */
-	WARN_ON_ONCE(pgio.pg_mirror_count != 1);
+	WARN_ON_ONCE(desc.pgio.pg_mirror_count != 1);
 
-	pgm = &pgio.pg_mirrors[0];
+	pgm = &desc.pgio.pg_mirrors[0];
 	NFS_I(inode)->read_io += pgm->pg_bytes_written;
 	npages = (pgm->pg_bytes_written + PAGE_SIZE - 1) >>
 		 PAGE_SHIFT;
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 6ee9119acc5d..7e5b9df9cfe6 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -553,8 +553,7 @@ extern int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fh,
 extern int  nfs_readpage(struct file *, struct page *);
 extern int  nfs_readpages(struct file *, struct address_space *,
 		struct list_head *, unsigned);
-extern int  nfs_readpage_async(struct nfs_open_context *, struct inode *,
-			       struct page *);
+extern int  nfs_readpage_async(void *, struct inode *, struct page *);
 
 /*
  * inline functions
-- 
1.8.3.1

[RFC PATCH v2 04/14] NFS: Call readpage_async_filler() from nfs_readpage_async()

[Dave Wysochanski]

Refactor slightly so nfs_readpage_async() calls into
readpage_async_filler().

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/read.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 5fda30742a32..1401f1c734c0 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -119,31 +119,22 @@ struct nfs_readdesc {
 	struct nfs_open_context *ctx;
 };
 
+static int readpage_async_filler(void *data, struct page *page);
+
 int nfs_readpage_async(void *data, struct inode *inode,
 		       struct page *page)
 {
 	struct nfs_readdesc *desc = (struct nfs_readdesc *)data;
-	struct nfs_page	*new;
-	unsigned int len;
 	struct nfs_pgio_mirror *pgm;
-
-	len = nfs_page_length(page);
-	if (len == 0)
-		return nfs_return_empty_page(page);
-	new = nfs_create_request(desc->ctx, page, 0, len);
-	if (IS_ERR(new)) {
-		unlock_page(page);
-		return PTR_ERR(new);
-	}
-	if (len < PAGE_SIZE)
-		zero_user_segment(page, len, PAGE_SIZE);
+	int error;
 
 	nfs_pageio_init_read(&desc->pgio, inode, false,
 			     &nfs_async_read_completion_ops);
-	if (!nfs_pageio_add_request(&desc->pgio, new)) {
-		nfs_list_remove_request(new);
-		nfs_readpage_release(new, desc->pgio.pg_error);
-	}
+
+	error = readpage_async_filler(desc, page);
+	if (error)
+		goto out;
+
 	nfs_pageio_complete(&desc->pgio);
 
 	/* It doesn't make sense to do mirrored reads! */
@@ -153,6 +144,9 @@ int nfs_readpage_async(void *data, struct inode *inode,
 	NFS_I(inode)->read_io += pgm->pg_bytes_written;
 
 	return desc->pgio.pg_error < 0 ? desc->pgio.pg_error : 0;
+
+out:
+	return error;
 }
 
 static void nfs_page_group_set_uptodate(struct nfs_page *req)
-- 
1.8.3.1

[RFC PATCH v2 05/14] NFS: Add nfs_pageio_complete_read() and remove nfs_readpage_async()

[Dave Wysochanski]

Add nfs_pageio_complete_read() and call this from both nfs_readpage()
and nfs_readpages(), since the submission and accounting is the same
for both functions.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/read.c          | 137 ++++++++++++++++++++++---------------------------
 include/linux/nfs_fs.h |   1 -
 2 files changed, 61 insertions(+), 77 deletions(-)

diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 1401f1c734c0..c2df4040f26c 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -74,6 +74,24 @@ void nfs_pageio_init_read(struct nfs_pageio_descriptor *pgio,
 }
 EXPORT_SYMBOL_GPL(nfs_pageio_init_read);
 
+static void nfs_pageio_complete_read(struct nfs_pageio_descriptor *pgio,
+				     struct inode *inode)
+{
+	struct nfs_pgio_mirror *pgm;
+	unsigned long npages;
+
+	nfs_pageio_complete(pgio);
+
+	/* It doesn't make sense to do mirrored reads! */
+	WARN_ON_ONCE(pgio->pg_mirror_count != 1);
+
+	pgm = &pgio->pg_mirrors[0];
+	NFS_I(inode)->read_io += pgm->pg_bytes_written;
+	npages = (pgm->pg_bytes_written + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	nfs_add_stats(inode, NFSIOS_READPAGES, npages);
+}
+
+
 void nfs_pageio_reset_read_mds(struct nfs_pageio_descriptor *pgio)
 {
 	struct nfs_pgio_mirror *mirror;
@@ -119,36 +137,6 @@ struct nfs_readdesc {
 	struct nfs_open_context *ctx;
 };
 
-static int readpage_async_filler(void *data, struct page *page);
-
-int nfs_readpage_async(void *data, struct inode *inode,
-		       struct page *page)
-{
-	struct nfs_readdesc *desc = (struct nfs_readdesc *)data;
-	struct nfs_pgio_mirror *pgm;
-	int error;
-
-	nfs_pageio_init_read(&desc->pgio, inode, false,
-			     &nfs_async_read_completion_ops);
-
-	error = readpage_async_filler(desc, page);
-	if (error)
-		goto out;
-
-	nfs_pageio_complete(&desc->pgio);
-
-	/* It doesn't make sense to do mirrored reads! */
-	WARN_ON_ONCE(desc->pgio.pg_mirror_count != 1);
-
-	pgm = &desc->pgio.pg_mirrors[0];
-	NFS_I(inode)->read_io += pgm->pg_bytes_written;
-
-	return desc->pgio.pg_error < 0 ? desc->pgio.pg_error : 0;
-
-out:
-	return error;
-}
-
 static void nfs_page_group_set_uptodate(struct nfs_page *req)
 {
 	if (nfs_page_group_sync_on_bit(req, PG_UPTODATE))
@@ -170,8 +158,7 @@ static void nfs_read_completion(struct nfs_pgio_header *hdr)
 
 		if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
 			/* note: regions of the page not covered by a
-			 * request are zeroed in nfs_readpage_async /
-			 * readpage_async_filler */
+			 * request are zeroed in readpage_async_filler */
 			if (bytes > hdr->good_bytes) {
 				/* nothing in this request was good, so zero
 				 * the full extent of the request */
@@ -303,6 +290,38 @@ static void nfs_readpage_result(struct rpc_task *task,
 		nfs_readpage_retry(task, hdr);
 }
 
+static int
+readpage_async_filler(void *data, struct page *page)
+{
+	struct nfs_readdesc *desc = (struct nfs_readdesc *)data;
+	struct nfs_page *new;
+	unsigned int len;
+	int error;
+
+	len = nfs_page_length(page);
+	if (len == 0)
+		return nfs_return_empty_page(page);
+
+	new = nfs_create_request(desc->ctx, page, 0, len);
+	if (IS_ERR(new))
+		goto out_error;
+
+	if (len < PAGE_SIZE)
+		zero_user_segment(page, len, PAGE_SIZE);
+	if (!nfs_pageio_add_request(&desc->pgio, new)) {
+		nfs_list_remove_request(new);
+		error = desc->pgio.pg_error;
+		nfs_readpage_release(new, error);
+		goto out;
+	}
+	return 0;
+out_error:
+	error = PTR_ERR(new);
+	unlock_page(page);
+out:
+	return error;
+}
+
 /*
  * Read a page over NFS.
  * We read the page synchronously in the following case:
@@ -351,13 +370,20 @@ int nfs_readpage(struct file *filp, struct page *page)
 	}
 
 	xchg(&desc.ctx->error, 0);
-	ret = nfs_readpage_async(&desc, inode, page);
+	nfs_pageio_init_read(&desc.pgio, inode, false,
+			     &nfs_async_read_completion_ops);
+
+	ret = readpage_async_filler(&desc, page);
+
+	if (!ret)
+		nfs_pageio_complete_read(&desc.pgio, inode);
+
+	ret = desc.pgio.pg_error < 0 ? desc.pgio.pg_error : 0;
 	if (!ret) {
 		ret = wait_on_page_locked_killable(page);
 		if (!PageUptodate(page) && !ret)
 			ret = xchg(&desc.ctx->error, 0);
 	}
-	nfs_add_stats(inode, NFSIOS_READPAGES, 1);
 out:
 	put_nfs_open_context(desc.ctx);
 	return ret;
@@ -366,45 +392,11 @@ int nfs_readpage(struct file *filp, struct page *page)
 	return ret;
 }
 
-static int
-readpage_async_filler(void *data, struct page *page)
-{
-	struct nfs_readdesc *desc = (struct nfs_readdesc *)data;
-	struct nfs_page *new;
-	unsigned int len;
-	int error;
-
-	len = nfs_page_length(page);
-	if (len == 0)
-		return nfs_return_empty_page(page);
-
-	new = nfs_create_request(desc->ctx, page, 0, len);
-	if (IS_ERR(new))
-		goto out_error;
-
-	if (len < PAGE_SIZE)
-		zero_user_segment(page, len, PAGE_SIZE);
-	if (!nfs_pageio_add_request(&desc->pgio, new)) {
-		nfs_list_remove_request(new);
-		error = desc->pgio.pg_error;
-		nfs_readpage_release(new, error);
-		goto out;
-	}
-	return 0;
-out_error:
-	error = PTR_ERR(new);
-	unlock_page(page);
-out:
-	return error;
-}
-
 int nfs_readpages(struct file *filp, struct address_space *mapping,
 		struct list_head *pages, unsigned nr_pages)
 {
-	struct nfs_pgio_mirror *pgm;
 	struct nfs_readdesc desc;
 	struct inode *inode = mapping->host;
-	unsigned long npages;
 	int ret;
 
 	dprintk("NFS: nfs_readpages (%s/%Lu %d)\n",
@@ -437,16 +429,9 @@ int nfs_readpages(struct file *filp, struct address_space *mapping,
 			     &nfs_async_read_completion_ops);
 
 	ret = read_cache_pages(mapping, pages, readpage_async_filler, &desc);
-	nfs_pageio_complete(&desc.pgio);
 
-	/* It doesn't make sense to do mirrored reads! */
-	WARN_ON_ONCE(desc.pgio.pg_mirror_count != 1);
+	nfs_pageio_complete_read(&desc.pgio, inode);
 
-	pgm = &desc.pgio.pg_mirrors[0];
-	NFS_I(inode)->read_io += pgm->pg_bytes_written;
-	npages = (pgm->pg_bytes_written + PAGE_SIZE - 1) >>
-		 PAGE_SHIFT;
-	nfs_add_stats(inode, NFSIOS_READPAGES, npages);
 read_complete:
 	put_nfs_open_context(desc.ctx);
 out:
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 7e5b9df9cfe6..b52ff4f2014c 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -553,7 +553,6 @@ extern int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fh,
 extern int  nfs_readpage(struct file *, struct page *);
 extern int  nfs_readpages(struct file *, struct address_space *,
 		struct list_head *, unsigned);
-extern int  nfs_readpage_async(void *, struct inode *, struct page *);
 
 /*
  * inline functions
-- 
1.8.3.1

[RFC PATCH v2 06/14] NFS: Allow internal use of read structs and functions

[Dave Wysochanski]

The conversion of the NFS read paths to the new fscache API
will require use of a few read structs and functions,
so move these declarations as required.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/internal.h |  9 +++++++++
 fs/nfs/read.c     | 13 ++++---------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 6673a77884d9..cfa848de5742 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -443,12 +443,21 @@ extern char *nfs_path(char **p, struct dentry *dentry,
 
 struct nfs_pgio_completion_ops;
 /* read.c */
+extern const struct nfs_pgio_completion_ops nfs_async_read_completion_ops;
 extern void nfs_pageio_init_read(struct nfs_pageio_descriptor *pgio,
 			struct inode *inode, bool force_mds,
 			const struct nfs_pgio_completion_ops *compl_ops);
+struct nfs_readdesc {
+	struct nfs_pageio_descriptor pgio;
+	struct nfs_open_context *ctx;
+};
+extern int readpage_async_filler(void *data, struct page *page);
+extern void nfs_pageio_complete_read(struct nfs_pageio_descriptor *pgio,
+				     struct inode *inode);
 extern void nfs_read_prepare(struct rpc_task *task, void *calldata);
 extern void nfs_pageio_reset_read_mds(struct nfs_pageio_descriptor *pgio);
 
+
 /* super.c */
 void nfs_umount_begin(struct super_block *);
 int  nfs_statfs(struct dentry *, struct kstatfs *);
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index c2df4040f26c..13266eda8f60 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -30,7 +30,7 @@
 
 #define NFSDBG_FACILITY		NFSDBG_PAGECACHE
 
-static const struct nfs_pgio_completion_ops nfs_async_read_completion_ops;
+const struct nfs_pgio_completion_ops nfs_async_read_completion_ops;
 static const struct nfs_rw_ops nfs_rw_read_ops;
 
 static struct kmem_cache *nfs_rdata_cachep;
@@ -74,7 +74,7 @@ void nfs_pageio_init_read(struct nfs_pageio_descriptor *pgio,
 }
 EXPORT_SYMBOL_GPL(nfs_pageio_init_read);
 
-static void nfs_pageio_complete_read(struct nfs_pageio_descriptor *pgio,
+void nfs_pageio_complete_read(struct nfs_pageio_descriptor *pgio,
 				     struct inode *inode)
 {
 	struct nfs_pgio_mirror *pgm;
@@ -132,11 +132,6 @@ static void nfs_readpage_release(struct nfs_page *req, int error)
 	nfs_release_request(req);
 }
 
-struct nfs_readdesc {
-	struct nfs_pageio_descriptor pgio;
-	struct nfs_open_context *ctx;
-};
-
 static void nfs_page_group_set_uptodate(struct nfs_page *req)
 {
 	if (nfs_page_group_sync_on_bit(req, PG_UPTODATE))
@@ -215,7 +210,7 @@ static void nfs_initiate_read(struct nfs_pgio_header *hdr,
 	}
 }
 
-static const struct nfs_pgio_completion_ops nfs_async_read_completion_ops = {
+const struct nfs_pgio_completion_ops nfs_async_read_completion_ops = {
 	.error_cleanup = nfs_async_read_error,
 	.completion = nfs_read_completion,
 };
@@ -290,7 +285,7 @@ static void nfs_readpage_result(struct rpc_task *task,
 		nfs_readpage_retry(task, hdr);
 }
 
-static int
+int
 readpage_async_filler(void *data, struct page *page)
 {
 	struct nfs_readdesc *desc = (struct nfs_readdesc *)data;
-- 
1.8.3.1

[RFC PATCH v2 07/14] NFS: Convert nfs_readpage() and readpages() to new fscache API

[Dave Wysochanski]

This patch converts the NFS read paths to the new fscache API,
minimizing changes to the existing code.

The new fscache IO path API uses a different mechanism to read
through the cache.  There are two main read_helper calls:
- readpage: fscache_read_helper_locked_page()
  - replaces old API fscache_read_or_alloc_page()
- readpages: fscache_read_helper_page_list()
  - replaces old API fscache_read_or_alloc_pages()

Once submitted to the read_helper, if pages are inside the cache
fscache will call the done() function of fscache_io_request_ops().
If the pages are not inside the cache, fscache will call issue_op()
so NFS can go through its normal read code paths, such as
nfs_pageio_init_read(), nfs_pageio_add_page_read() and
nfs_pageio_complete_read().

In the read completion code path, from nfs_read_completion() we
must call into fscache via a cache.io_done() function.  In order
to call back into fscache via this function, we must save the
nfs_fscache_req * as a field in the nfs_pgio_header, similar to
nfs_direct_req.  Note also that when fscache is enabled, the
read_helper will lock and unlock the pages so in the completion
path we skip the unlock_page() with fscache.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/fscache.c         | 217 +++++++++++++++++++++++------------------------
 fs/nfs/fscache.h         |  30 +++----
 fs/nfs/pagelist.c        |   1 +
 fs/nfs/read.c            |  12 ++-
 include/linux/nfs_page.h |   1 +
 include/linux/nfs_xdr.h  |   1 +
 6 files changed, 132 insertions(+), 130 deletions(-)

diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
index a60df88efc40..f641f33fa632 100644
--- a/fs/nfs/fscache.c
+++ b/fs/nfs/fscache.c
@@ -328,73 +328,88 @@ void nfs_fscache_open_file(struct inode *inode, struct file *filp)
 }
 EXPORT_SYMBOL_GPL(nfs_fscache_open_file);
 
-/*
- * Release the caching state associated with a page, if the page isn't busy
- * interacting with the cache.
- * - Returns true (can release page) or false (page busy).
- */
-int nfs_fscache_release_page(struct page *page, gfp_t gfp)
-{
-	if (PageFsCache(page)) {
-		struct fscache_cookie *cookie = nfs_i_fscache(page->mapping->host);
-
-		BUG_ON(!cookie);
-		dfprintk(FSCACHE, "NFS: fscache releasepage (0x%p/0x%p/0x%p)\n",
-			 cookie, page, NFS_I(page->mapping->host));
-
-		if (!fscache_maybe_release_page(cookie, page, gfp))
-			return 0;
+struct nfs_fscache_req {
+	struct fscache_io_request	cache;
+	struct nfs_readdesc             desc;
+	refcount_t			usage;
+};
 
-		nfs_inc_fscache_stats(page->mapping->host,
-				      NFSIOS_FSCACHE_PAGES_UNCACHED);
-	}
+static void nfs_done_io_request(struct fscache_io_request *fsreq)
+{
+	struct nfs_fscache_req *req = container_of(fsreq, struct nfs_fscache_req, cache);
+	struct inode *inode = d_inode(req->desc.ctx->dentry);
 
-	return 1;
+	nfs_add_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_OK,
+			      fsreq->transferred >> PAGE_SHIFT);
 }
 
-/*
- * Release the caching state associated with a page if undergoing complete page
- * invalidation.
- */
-void __nfs_fscache_invalidate_page(struct page *page, struct inode *inode)
+static void nfs_get_io_request(struct fscache_io_request *fsreq)
 {
-	struct fscache_cookie *cookie = nfs_i_fscache(inode);
+	struct nfs_fscache_req *req = container_of(fsreq, struct nfs_fscache_req, cache);
 
-	BUG_ON(!cookie);
+	refcount_inc(&req->usage);
+}
 
-	dfprintk(FSCACHE, "NFS: fscache invalidatepage (0x%p/0x%p/0x%p)\n",
-		 cookie, page, NFS_I(inode));
+static void nfs_put_io_request(struct fscache_io_request *fsreq)
+{
+	struct nfs_fscache_req *req = container_of(fsreq, struct nfs_fscache_req, cache);
 
-	fscache_wait_on_page_write(cookie, page);
+	if (refcount_dec_and_test(&req->usage)) {
+		put_nfs_open_context(req->desc.ctx);
+		fscache_free_io_request(fsreq);
+		kfree(req);
+	}
+}
 
-	BUG_ON(!PageLocked(page));
-	fscache_uncache_page(cookie, page);
-	nfs_inc_fscache_stats(page->mapping->host,
-			      NFSIOS_FSCACHE_PAGES_UNCACHED);
+static void nfs_issue_op(struct fscache_io_request *fsreq)
+{
+	struct nfs_fscache_req *req = container_of(fsreq, struct nfs_fscache_req, cache);
+	struct inode *inode = req->cache.mapping->host;
+	struct page *page;
+	pgoff_t index = req->cache.pos >> PAGE_SHIFT;
+	pgoff_t last = index + req->cache.nr_pages - 1;
+
+	nfs_add_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_FAIL,
+			      req->cache.nr_pages);
+	nfs_get_io_request(fsreq);
+	nfs_pageio_init_read(&req->desc.pgio, inode, false,
+			     &nfs_async_read_completion_ops);
+
+	for (; index <= last; index++) {
+		page = find_get_page(req->cache.mapping, index);
+		BUG_ON(!page);
+		req->cache.error = readpage_async_filler(&req->desc, page);
+		if (req->cache.error < 0)
+			break;
+	}
+	nfs_pageio_complete_read(&req->desc.pgio, inode);
 }
 
-/*
- * Handle completion of a page being read from the cache.
- * - Called in process (keventd) context.
- */
-static void nfs_readpage_from_fscache_complete(struct page *page,
-					       void *context,
-					       int error)
+static struct fscache_io_request_ops nfs_fscache_req_ops = {
+	.issue_op	= nfs_issue_op,
+	.done		= nfs_done_io_request,
+	.get		= nfs_get_io_request,
+	.put		= nfs_put_io_request,
+};
+
+struct nfs_fscache_req *nfs_alloc_io_request(struct nfs_open_context *ctx,
+					    struct address_space *mapping)
 {
-	dfprintk(FSCACHE,
-		 "NFS: readpage_from_fscache_complete (0x%p/0x%p/%d)\n",
-		 page, context, error);
-
-	/* if the read completes with an error, we just unlock the page and let
-	 * the VM reissue the readpage */
-	if (!error) {
-		SetPageUptodate(page);
-		unlock_page(page);
-	} else {
-		error = nfs_readpage_async(context, page->mapping->host, page);
-		if (error)
-			unlock_page(page);
+	struct nfs_fscache_req *req;
+	struct inode *inode = mapping->host;
+
+	req = kzalloc(sizeof(*req), GFP_KERNEL);
+	if (req) {
+		refcount_set(&req->usage, 1);
+		req->cache.mapping = mapping;
+		req->desc.ctx = get_nfs_open_context(ctx);
+
+		fscache_init_io_request(&req->cache, nfs_i_fscache(inode),
+					&nfs_fscache_req_ops);
+		req->desc.pgio.pg_fsc_req = req;
 	}
+
+	return req;
 }
 
 /*
@@ -403,36 +418,38 @@ static void nfs_readpage_from_fscache_complete(struct page *page,
 int __nfs_readpage_from_fscache(struct nfs_open_context *ctx,
 				struct inode *inode, struct page *page)
 {
+	struct nfs_fscache_req *req;
 	int ret;
 
 	dfprintk(FSCACHE,
 		 "NFS: readpage_from_fscache(fsc:%p/p:%p(i:%lx f:%lx)/0x%p)\n",
 		 nfs_i_fscache(inode), page, page->index, page->flags, inode);
 
-	ret = fscache_read_or_alloc_page(nfs_i_fscache(inode),
-					 page,
-					 nfs_readpage_from_fscache_complete,
-					 ctx,
-					 GFP_KERNEL);
+	req = nfs_alloc_io_request(ctx, page_file_mapping(page));
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	ret = fscache_read_helper_locked_page(&req->cache, page, ULONG_MAX);
+
+	nfs_put_io_request(&req->cache);
 
 	switch (ret) {
-	case 0: /* read BIO submitted (page in fscache) */
-		dfprintk(FSCACHE,
-			 "NFS:    readpage_from_fscache: BIO submitted\n");
+	case 0: /* read submitted */
+		dfprintk(FSCACHE, "NFS:    readpage_from_fscache: submitted\n");
 		nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_OK);
 		return ret;
 
 	case -ENOBUFS: /* inode not in cache */
 	case -ENODATA: /* page not in cache */
 		nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_FAIL);
-		dfprintk(FSCACHE,
-			 "NFS:    readpage_from_fscache %d\n", ret);
+		dfprintk(FSCACHE, "NFS:    readpage_from_fscache %d\n", ret);
 		return 1;
 
 	default:
 		dfprintk(FSCACHE, "NFS:    readpage_from_fscache %d\n", ret);
 		nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_FAIL);
 	}
+
 	return ret;
 }
 
@@ -442,75 +459,57 @@ int __nfs_readpage_from_fscache(struct nfs_open_context *ctx,
 int __nfs_readpages_from_fscache(struct nfs_open_context *ctx,
 				 struct inode *inode,
 				 struct address_space *mapping,
-				 struct list_head *pages,
-				 unsigned *nr_pages)
+				 struct list_head *pages)
 {
-	unsigned npages = *nr_pages;
+	struct nfs_fscache_req *req;
 	int ret;
 
-	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_readpages_from_fscache (0x%p/0x%p)\n",
+		 nfs_i_fscache(inode), inode);
+
+	while (!list_empty(pages)) {
+		req = nfs_alloc_io_request(ctx, mapping);
+		if (IS_ERR(req))
+			return PTR_ERR(req);
+
+		ret = fscache_read_helper_page_list(&req->cache, pages,
+						    ULONG_MAX);
+		nfs_put_io_request(&req->cache);
+		if (ret < 0)
+			break;
+	}
 
 	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");
+			 "NFS: nfs_readpages_from_fscache: submitted\n");
 
 		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);
+			 "NFS: nfs_readpages_from_fscache: no page: %d\n", ret);
 		return 1;
 
 	default:
 		dfprintk(FSCACHE,
-			 "NFS: nfs_getpages_from_fscache: ret  %d\n", ret);
+			 "NFS: nfs_readpages_from_fscache: ret  %d\n", ret);
 	}
-
 	return ret;
 }
 
 /*
- * Store a newly fetched page in fscache
- * - PG_fscache must be set on the page
+ * Store a newly fetched data in fscache
  */
-void __nfs_readpage_to_fscache(struct inode *inode, struct page *page, int sync)
+void __nfs_read_completion_to_fscache(struct nfs_pgio_header *hdr, unsigned long bytes)
 {
-	int ret;
+	struct nfs_fscache_req *fsc_req = hdr->fsc_req;
 
-	dfprintk(FSCACHE,
-		 "NFS: readpage_to_fscache(fsc:%p/p:%p(i:%lx f:%lx)/%d)\n",
-		 nfs_i_fscache(inode), page, page->index, page->flags, sync);
-
-	ret = fscache_write_page(nfs_i_fscache(inode), page,
-				 inode->i_size, GFP_KERNEL);
-	dfprintk(FSCACHE,
-		 "NFS:     readpage_to_fscache: p:%p(i:%lu f:%lx) ret %d\n",
-		 page, page->index, page->flags, ret);
-
-	if (ret != 0) {
-		fscache_uncache_page(nfs_i_fscache(inode), page);
-		nfs_inc_fscache_stats(inode,
-				      NFSIOS_FSCACHE_PAGES_WRITTEN_FAIL);
-		nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_UNCACHED);
-	} else {
-		nfs_inc_fscache_stats(inode,
-				      NFSIOS_FSCACHE_PAGES_WRITTEN_OK);
+	if (fsc_req && fsc_req->cache.io_done) {
+		fsc_req->cache.transferred = min_t(long long, bytes, fsc_req->cache.len);
+		set_bit(FSCACHE_IO_DATA_FROM_SERVER, &fsc_req->cache.flags);
+		fsc_req->cache.io_done(&fsc_req->cache);
+		nfs_put_io_request(&fsc_req->cache);
 	}
 }
diff --git a/fs/nfs/fscache.h b/fs/nfs/fscache.h
index 6754c8607230..d61721832838 100644
--- a/fs/nfs/fscache.h
+++ b/fs/nfs/fscache.h
@@ -100,8 +100,9 @@ extern int __nfs_readpage_from_fscache(struct nfs_open_context *,
 				       struct inode *, struct page *);
 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);
+					struct list_head *);
+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
@@ -142,25 +143,22 @@ static inline int nfs_readpage_from_fscache(struct nfs_open_context *ctx,
 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)
+					     struct list_head *pages)
 {
 	if (NFS_I(inode)->fscache)
-		return __nfs_readpages_from_fscache(ctx, inode, mapping, pages,
-						    nr_pages);
+		return __nfs_readpages_from_fscache(ctx, inode, mapping, pages);
 	return -ENOBUFS;
 }
 
 /*
- * Store a page newly fetched from the server in an inode data storage object
+ * Store pages newly fetched from the server in an inode data storage object
  * in the cache.
  */
-static inline void nfs_readpage_to_fscache(struct inode *inode,
-					   struct page *page,
-					   int sync)
+static inline void nfs_read_completion_to_fscache(struct nfs_pgio_header *hdr,
+						  unsigned long bytes)
 {
-	if (PageFsCache(page))
-		__nfs_readpage_to_fscache(inode, page, sync);
+	if (NFS_I(hdr->inode)->fscache)
+		__nfs_read_completion_to_fscache(hdr, bytes);
 }
 
 /*
@@ -221,14 +219,12 @@ static inline int nfs_readpage_from_fscache(struct nfs_open_context *ctx,
 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)
+					     struct list_head *pages)
 {
 	return -ENOBUFS;
 }
-static inline void nfs_readpage_to_fscache(struct inode *inode,
-					   struct page *page, int sync) {}
-
+static inline void nfs_read_completion_to_fscache(struct nfs_pgio_header *hdr,
+						  unsigned long bytes) {}
 
 static inline void nfs_fscache_invalidate(struct inode *inode) {}
 static inline void nfs_fscache_wait_on_invalidate(struct inode *inode) {}
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 6ea4cac41e46..c8073b3667d9 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -52,6 +52,7 @@ void nfs_pgheader_init(struct nfs_pageio_descriptor *desc,
 	hdr->good_bytes = mirror->pg_count;
 	hdr->io_completion = desc->pg_io_completion;
 	hdr->dreq = desc->pg_dreq;
+	hdr->fsc_req = desc->pg_fsc_req;
 	hdr->release = release;
 	hdr->completion_ops = desc->pg_completion_ops;
 	if (hdr->completion_ops->init_hdr)
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 13266eda8f60..c92862c83a7f 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -124,10 +124,13 @@ static void nfs_readpage_release(struct nfs_page *req, int error)
 		struct address_space *mapping = page_file_mapping(page);
 
 		if (PageUptodate(page))
-			nfs_readpage_to_fscache(inode, page, 0);
+			; /* FIXME: review fscache page error handling */
 		else if (!PageError(page) && !PagePrivate(page))
 			generic_error_remove_page(mapping, page);
-		unlock_page(page);
+		if (nfs_i_fscache(inode))
+			put_page(page); /* See nfs_issue_op() */
+		else
+			unlock_page(page);
 	}
 	nfs_release_request(req);
 }
@@ -181,6 +184,8 @@ static void nfs_read_completion(struct nfs_pgio_header *hdr)
 		nfs_list_remove_request(req);
 		nfs_readpage_release(req, error);
 	}
+	/* FIXME: Review error handling before writing to fscache */
+	nfs_read_completion_to_fscache(hdr, bytes);
 out:
 	hdr->release(hdr);
 }
@@ -415,8 +420,7 @@ int nfs_readpages(struct file *filp, struct address_space *mapping,
 	/* 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_readpages_from_fscache(desc.ctx, inode, mapping, pages);
 	if (ret == 0)
 		goto read_complete; /* all pages were read */
 
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index c32c15216da3..cf4b1a62108e 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -97,6 +97,7 @@ struct nfs_pageio_descriptor {
 	struct pnfs_layout_segment *pg_lseg;
 	struct nfs_io_completion *pg_io_completion;
 	struct nfs_direct_req	*pg_dreq;
+	struct nfs_fscache_req  *pg_fsc_req; /* fscache req - may be NULL */
 	unsigned int		pg_bsize;	/* default bsize for mirrors */
 
 	u32			pg_mirror_count;
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 5fd0a9ef425f..746548676a51 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1535,6 +1535,7 @@ struct nfs_pgio_header {
 	const struct nfs_rw_ops	*rw_ops;
 	struct nfs_io_completion *io_completion;
 	struct nfs_direct_req	*dreq;
+	struct nfs_fscache_req  *fsc_req;  /* fscache req - may be NULL */
 
 	int			pnfs_error;
 	int			error;		/* merge with pnfs_error */
-- 
1.8.3.1

[RFC PATCH v2 08/14] NFS: Convert fscache_acquire_cookie and fscache_relinquish_cookie

[Dave Wysochanski]

The new FS-Cache netfs API changes the cookie API slightly.

The changes to fscache_acquire_cookie are:
* remove struct fscache_cookie_def
* add 'type' of cookie (was member of fscache_cookie_def)
* add 'name' of cookie (was member of fscache_cookie_def)
* add 'advice' flags (tells cache how to handle object); set to 0
* add 'preferred_cache' tag (if NULL, derive from parent)
* remove 'netfs_data'
* remove 'enable' (See fscache_use_cookie())

The changes to fscache_relinquish_cookie are:
* remove 'aux_data'

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/fscache-index.c | 94 --------------------------------------------------
 fs/nfs/fscache.c       | 56 +++++++++++++++++-------------
 fs/nfs/fscache.h       |  3 --
 3 files changed, 32 insertions(+), 121 deletions(-)

diff --git a/fs/nfs/fscache-index.c b/fs/nfs/fscache-index.c
index b1049815729e..b4fdacd955f3 100644
--- a/fs/nfs/fscache-index.c
+++ b/fs/nfs/fscache-index.c
@@ -44,97 +44,3 @@ void nfs_fscache_unregister(void)
 {
 	fscache_unregister_netfs(&nfs_fscache_netfs);
 }
-
-/*
- * Define the server object for FS-Cache.  This is used to describe a server
- * object to fscache_acquire_cookie().  It is keyed by the NFS protocol and
- * server address parameters.
- */
-const struct fscache_cookie_def nfs_fscache_server_index_def = {
-	.name		= "NFS.server",
-	.type 		= FSCACHE_COOKIE_TYPE_INDEX,
-};
-
-/*
- * Define the superblock object for FS-Cache.  This is used to describe a
- * superblock object to fscache_acquire_cookie().  It is keyed by all the NFS
- * parameters that might cause a separate superblock.
- */
-const struct fscache_cookie_def nfs_fscache_super_index_def = {
-	.name		= "NFS.super",
-	.type 		= FSCACHE_COOKIE_TYPE_INDEX,
-};
-
-/*
- * Consult the netfs about the state of an object
- * - This function can be absent if the index carries no state data
- * - The netfs data from the cookie being used as the target is
- *   presented, as is the auxiliary data
- */
-static
-enum fscache_checkaux nfs_fscache_inode_check_aux(void *cookie_netfs_data,
-						  const void *data,
-						  uint16_t datalen,
-						  loff_t object_size)
-{
-	struct nfs_fscache_inode_auxdata auxdata;
-	struct nfs_inode *nfsi = cookie_netfs_data;
-
-	if (datalen != sizeof(auxdata))
-		return FSCACHE_CHECKAUX_OBSOLETE;
-
-	memset(&auxdata, 0, sizeof(auxdata));
-	auxdata.mtime_sec  = nfsi->vfs_inode.i_mtime.tv_sec;
-	auxdata.mtime_nsec = nfsi->vfs_inode.i_mtime.tv_nsec;
-	auxdata.ctime_sec  = nfsi->vfs_inode.i_ctime.tv_sec;
-	auxdata.ctime_nsec = nfsi->vfs_inode.i_ctime.tv_nsec;
-
-	if (NFS_SERVER(&nfsi->vfs_inode)->nfs_client->rpc_ops->version == 4)
-		auxdata.change_attr = inode_peek_iversion_raw(&nfsi->vfs_inode);
-
-	if (memcmp(data, &auxdata, datalen) != 0)
-		return FSCACHE_CHECKAUX_OBSOLETE;
-
-	return FSCACHE_CHECKAUX_OKAY;
-}
-
-/*
- * Get an extra reference on a read context.
- * - This function can be absent if the completion function doesn't require a
- *   context.
- * - The read context is passed back to NFS in the event that a data read on the
- *   cache fails with EIO - in which case the server must be contacted to
- *   retrieve the data, which requires the read context for security.
- */
-static void nfs_fh_get_context(void *context)
-{
-	get_nfs_open_context(context);
-}
-
-/*
- * Release an extra reference on a read context.
- * - This function can be absent if the completion function doesn't require a
- *   context.
- */
-static void nfs_fh_put_context(void *context)
-{
-	if (context)
-		put_nfs_open_context(context);
-}
-
-/*
- * Define the inode object for FS-Cache.  This is used to describe an inode
- * object to fscache_acquire_cookie().  It is keyed by the NFS file handle for
- * an inode.
- *
- * Coherency is managed by comparing the copies of i_size, i_mtime and i_ctime
- * held in the cache auxiliary data for the data storage object with those in
- * the inode struct in memory.
- */
-const struct fscache_cookie_def nfs_fscache_inode_object_def = {
-	.name		= "NFS.fh",
-	.type		= FSCACHE_COOKIE_TYPE_DATAFILE,
-	.check_aux	= nfs_fscache_inode_check_aux,
-	.get_context	= nfs_fh_get_context,
-	.put_context	= nfs_fh_put_context,
-};
diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
index f641f33fa632..1c4ced273fb7 100644
--- a/fs/nfs/fscache.c
+++ b/fs/nfs/fscache.c
@@ -81,10 +81,15 @@ void nfs_fscache_get_client_cookie(struct nfs_client *clp)
 
 	/* create a cache index for looking up filehandles */
 	clp->fscache = fscache_acquire_cookie(nfs_fscache_netfs.primary_index,
-					      &nfs_fscache_server_index_def,
-					      &key, len,
-					      NULL, 0,
-					      clp, 0, true);
+					      FSCACHE_COOKIE_TYPE_INDEX,
+					      "NFS.server",
+					      0,    /* advice */
+					      NULL, /* preferred_cache */
+					      &key, /* index_key */
+					      len,
+					      NULL, /* aux_data */
+					      0,
+					      0);
 	dfprintk(FSCACHE, "NFS: get client cookie (0x%p/0x%p)\n",
 		 clp, clp->fscache);
 }
@@ -97,7 +102,7 @@ void nfs_fscache_release_client_cookie(struct nfs_client *clp)
 	dfprintk(FSCACHE, "NFS: releasing client cookie (0x%p/0x%p)\n",
 		 clp, clp->fscache);
 
-	fscache_relinquish_cookie(clp->fscache, NULL, false);
+	fscache_relinquish_cookie(clp->fscache, false);
 	clp->fscache = NULL;
 }
 
@@ -185,11 +190,15 @@ void nfs_fscache_get_super_cookie(struct super_block *sb, const char *uniq, int
 
 	/* create a cache index for looking up filehandles */
 	nfss->fscache = fscache_acquire_cookie(nfss->nfs_client->fscache,
-					       &nfs_fscache_super_index_def,
-					       &key->key,
+					       FSCACHE_COOKIE_TYPE_INDEX,
+					       "NFS.super",
+					       0,    /* advice */
+					       NULL, /* preferred_cache */
+					       &key->key,  /* index_key */
 					       sizeof(key->key) + ulen,
-					       NULL, 0,
-					       nfss, 0, true);
+					       NULL, /* aux_data */
+					       0,
+					       0);
 	dfprintk(FSCACHE, "NFS: get superblock cookie (0x%p/0x%p)\n",
 		 nfss, nfss->fscache);
 	return;
@@ -213,7 +222,7 @@ void nfs_fscache_release_super_cookie(struct super_block *sb)
 	dfprintk(FSCACHE, "NFS: releasing superblock cookie (0x%p/0x%p)\n",
 		 nfss, nfss->fscache);
 
-	fscache_relinquish_cookie(nfss->fscache, NULL, false);
+	fscache_relinquish_cookie(nfss->fscache, false);
 	nfss->fscache = NULL;
 
 	if (nfss->fscache_key) {
@@ -254,10 +263,15 @@ void nfs_fscache_init_inode(struct inode *inode)
 	nfs_fscache_update_auxdata(&auxdata, nfsi);
 
 	nfsi->fscache = fscache_acquire_cookie(NFS_SB(inode->i_sb)->fscache,
-					       &nfs_fscache_inode_object_def,
-					       nfsi->fh.data, nfsi->fh.size,
-					       &auxdata, sizeof(auxdata),
-					       nfsi, nfsi->vfs_inode.i_size, false);
+					       FSCACHE_COOKIE_TYPE_DATAFILE,
+					       "NFS.fh",
+					       0,             /* advice */
+					       NULL, /* preferred_cache */
+					       nfsi->fh.data, /* index_key */
+					       nfsi->fh.size,
+					       &auxdata,      /* aux_data */
+					       sizeof(auxdata),
+					       i_size_read(&nfsi->vfs_inode));
 }
 
 /*
@@ -272,7 +286,8 @@ void nfs_fscache_clear_inode(struct inode *inode)
 	dfprintk(FSCACHE, "NFS: clear cookie (0x%p/0x%p)\n", nfsi, cookie);
 
 	nfs_fscache_update_auxdata(&auxdata, nfsi);
-	fscache_relinquish_cookie(cookie, &auxdata, false);
+	fscache_unuse_cookie(cookie, &auxdata, NULL);
+	fscache_relinquish_cookie(cookie, false);
 	nfsi->fscache = NULL;
 }
 
@@ -304,27 +319,20 @@ static bool nfs_fscache_can_enable(void *data)
  */
 void nfs_fscache_open_file(struct inode *inode, struct file *filp)
 {
-	struct nfs_fscache_inode_auxdata auxdata;
 	struct nfs_inode *nfsi = NFS_I(inode);
 	struct fscache_cookie *cookie = nfs_i_fscache(inode);
 
 	if (!fscache_cookie_valid(cookie))
 		return;
 
-	nfs_fscache_update_auxdata(&auxdata, nfsi);
-
 	if (inode_is_open_for_write(inode)) {
 		dfprintk(FSCACHE, "NFS: nfsi 0x%p disabling cache\n", nfsi);
 		clear_bit(NFS_INO_FSCACHE, &nfsi->flags);
-		fscache_disable_cookie(cookie, &auxdata, true);
-		fscache_uncache_all_inode_pages(cookie, inode);
 	} else {
 		dfprintk(FSCACHE, "NFS: nfsi 0x%p enabling cache\n", nfsi);
-		fscache_enable_cookie(cookie, &auxdata, nfsi->vfs_inode.i_size,
-				      nfs_fscache_can_enable, inode);
-		if (fscache_cookie_enabled(cookie))
-			set_bit(NFS_INO_FSCACHE, &NFS_I(inode)->flags);
+		set_bit(NFS_INO_FSCACHE, &NFS_I(inode)->flags);
 	}
+	fscache_use_cookie(cookie, false);
 }
 EXPORT_SYMBOL_GPL(nfs_fscache_open_file);
 
diff --git a/fs/nfs/fscache.h b/fs/nfs/fscache.h
index d61721832838..95a1b29946a0 100644
--- a/fs/nfs/fscache.h
+++ b/fs/nfs/fscache.h
@@ -73,9 +73,6 @@ struct nfs_fscache_inode_auxdata {
  * fscache-index.c
  */
 extern struct fscache_netfs nfs_fscache_netfs;
-extern const struct fscache_cookie_def nfs_fscache_server_index_def;
-extern const struct fscache_cookie_def nfs_fscache_super_index_def;
-extern const struct fscache_cookie_def nfs_fscache_inode_object_def;
 
 extern int nfs_fscache_register(void);
 extern void nfs_fscache_unregister(void);
-- 
1.8.3.1

[RFC PATCH v2 09/14] NFS: Only use and unuse an fscache cookie a single time based on NFS_INO_FSCACHE

[Dave Wysochanski]

For NFS, we only want to make a decision whether to "use" a inode based
fscache cookie one time, not multiple times, and based on whether the
inode is open for write by any process.  Achieve this by gating the
call to fscache_use_cookie / fscache_unuse_cookie by the NFS_INO_FSCACHE
flag on the nfs_inode.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/fscache.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
index 1c4ced273fb7..00bb720cccfa 100644
--- a/fs/nfs/fscache.c
+++ b/fs/nfs/fscache.c
@@ -285,8 +285,10 @@ void nfs_fscache_clear_inode(struct inode *inode)
 
 	dfprintk(FSCACHE, "NFS: clear cookie (0x%p/0x%p)\n", nfsi, cookie);
 
-	nfs_fscache_update_auxdata(&auxdata, nfsi);
-	fscache_unuse_cookie(cookie, &auxdata, NULL);
+	if (test_and_clear_bit(NFS_INO_FSCACHE, &NFS_I(inode)->flags)) {
+		nfs_fscache_update_auxdata(&auxdata, nfsi);
+		fscache_unuse_cookie(cookie, &auxdata, NULL);
+	}
 	fscache_relinquish_cookie(cookie, false);
 	nfsi->fscache = NULL;
 }
@@ -321,18 +323,23 @@ void nfs_fscache_open_file(struct inode *inode, struct file *filp)
 {
 	struct nfs_inode *nfsi = NFS_I(inode);
 	struct fscache_cookie *cookie = nfs_i_fscache(inode);
+	struct nfs_fscache_inode_auxdata auxdata;
 
 	if (!fscache_cookie_valid(cookie))
 		return;
 
 	if (inode_is_open_for_write(inode)) {
-		dfprintk(FSCACHE, "NFS: nfsi 0x%p disabling cache\n", nfsi);
-		clear_bit(NFS_INO_FSCACHE, &nfsi->flags);
+		if (test_and_clear_bit(NFS_INO_FSCACHE, &nfsi->flags)) {
+			dfprintk(FSCACHE, "NFS: nfsi 0x%p disabling cache\n", nfsi);
+			nfs_fscache_update_auxdata(&auxdata, nfsi);
+			fscache_unuse_cookie(cookie, &auxdata, NULL);
+		}
 	} else {
-		dfprintk(FSCACHE, "NFS: nfsi 0x%p enabling cache\n", nfsi);
-		set_bit(NFS_INO_FSCACHE, &NFS_I(inode)->flags);
+		if (!test_and_set_bit(NFS_INO_FSCACHE, &nfsi->flags)) {
+			dfprintk(FSCACHE, "NFS: nfsi 0x%p enabling cache\n", nfsi);
+			fscache_use_cookie(cookie, false);
+		}
 	}
-	fscache_use_cookie(cookie, false);
 }
 EXPORT_SYMBOL_GPL(nfs_fscache_open_file);
 
-- 
1.8.3.1

[RFC PATCH v2 10/14] NFS: Convert fscache invalidation and update aux_data and i_size

[Dave Wysochanski]

Convert nfs_fscache_invalidate to the new FS-Cache API.  Also,
now when invalidating an fscache cookie, be sure to pass the latest
i_size as well as aux_data to fscache.

A few APIs no longer exist so remove them.  We can call directly to
wait_on_page_fscache() because it checks whether a page is an fscache
page before waiting on it.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/file.c    | 20 +++++++++++-------
 fs/nfs/fscache.c | 21 -------------------
 fs/nfs/fscache.h | 64 +++++++++++++++++++-------------------------------------
 fs/nfs/inode.c   |  1 -
 fs/nfs/write.c   |  2 +-
 5 files changed, 35 insertions(+), 73 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index ccd6c1637b27..7af528c423c0 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -413,8 +413,8 @@ static void nfs_invalidate_page(struct page *page, unsigned int offset,
 		return;
 	/* Cancel any unstarted writes on this page */
 	nfs_wb_page_cancel(page_file_mapping(page)->host, page);
-
-	nfs_fscache_invalidate_page(page, page->mapping->host);
+	wait_on_page_fscache(page);
+	nfs_fscache_invalidate(page_file_mapping(page)->host);
 }
 
 /*
@@ -429,8 +429,13 @@ static int nfs_release_page(struct page *page, gfp_t gfp)
 
 	/* If PagePrivate() is set, then the page is not freeable */
 	if (PagePrivate(page))
-		return 0;
-	return nfs_fscache_release_page(page, gfp);
+		return false;
+	if (PageFsCache(page)) {
+		if (!(gfp & __GFP_DIRECT_RECLAIM) || !(gfp & __GFP_FS))
+			return false;
+		wait_on_page_fscache(page);
+	}
+	return true;
 }
 
 static void nfs_check_dirty_writeback(struct page *page,
@@ -473,12 +478,11 @@ static void nfs_check_dirty_writeback(struct page *page,
 static int nfs_launder_page(struct page *page)
 {
 	struct inode *inode = page_file_mapping(page)->host;
-	struct nfs_inode *nfsi = NFS_I(inode);
 
 	dfprintk(PAGECACHE, "NFS: launder_page(%ld, %llu)\n",
 		inode->i_ino, (long long)page_offset(page));
 
-	nfs_fscache_wait_on_page_write(nfsi, page);
+	wait_on_page_fscache(page);
 	return nfs_wb_page(inode, page);
 }
 
@@ -553,7 +557,9 @@ static vm_fault_t nfs_vm_page_mkwrite(struct vm_fault *vmf)
 	sb_start_pagefault(inode->i_sb);
 
 	/* make sure the cache has finished storing the page */
-	nfs_fscache_wait_on_page_write(NFS_I(inode), page);
+	if (PageFsCache(vmf->page) &&
+	    wait_on_page_bit_killable(vmf->page, PG_fscache) < 0)
+		return VM_FAULT_RETRY;
 
 	wait_on_bit_action(&NFS_I(inode)->flags, NFS_INO_INVALIDATING,
 			nfs_wait_bit_killable, TASK_KILLABLE);
diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
index 00bb720cccfa..08cdc78410d5 100644
--- a/fs/nfs/fscache.c
+++ b/fs/nfs/fscache.c
@@ -14,7 +14,6 @@
 #include <linux/in6.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
-#include <linux/iversion.h>
 
 #include "internal.h"
 #include "iostat.h"
@@ -234,19 +233,6 @@ void nfs_fscache_release_super_cookie(struct super_block *sb)
 	}
 }
 
-static void nfs_fscache_update_auxdata(struct nfs_fscache_inode_auxdata *auxdata,
-				  struct nfs_inode *nfsi)
-{
-	memset(auxdata, 0, sizeof(*auxdata));
-	auxdata->mtime_sec  = nfsi->vfs_inode.i_mtime.tv_sec;
-	auxdata->mtime_nsec = nfsi->vfs_inode.i_mtime.tv_nsec;
-	auxdata->ctime_sec  = nfsi->vfs_inode.i_ctime.tv_sec;
-	auxdata->ctime_nsec = nfsi->vfs_inode.i_ctime.tv_nsec;
-
-	if (NFS_SERVER(&nfsi->vfs_inode)->nfs_client->rpc_ops->version == 4)
-		auxdata->change_attr = inode_peek_iversion_raw(&nfsi->vfs_inode);
-}
-
 /*
  * Initialise the per-inode cache cookie pointer for an NFS inode.
  */
@@ -293,13 +279,6 @@ void nfs_fscache_clear_inode(struct inode *inode)
 	nfsi->fscache = NULL;
 }
 
-static bool nfs_fscache_can_enable(void *data)
-{
-	struct inode *inode = data;
-
-	return !inode_is_open_for_write(inode);
-}
-
 /*
  * Enable or disable caching for a file that is being opened as appropriate.
  * The cookie is allocated when the inode is initialised, but is not enabled at
diff --git a/fs/nfs/fscache.h b/fs/nfs/fscache.h
index 95a1b29946a0..de060f61cadd 100644
--- a/fs/nfs/fscache.h
+++ b/fs/nfs/fscache.h
@@ -12,6 +12,7 @@
 #include <linux/nfs_mount.h>
 #include <linux/nfs4_mount.h>
 #include <linux/fscache.h>
+#include <linux/iversion.h>
 
 #ifdef CONFIG_NFS_FSCACHE
 
@@ -90,9 +91,6 @@ struct nfs_fscache_inode_auxdata {
 extern void nfs_fscache_clear_inode(struct inode *);
 extern void nfs_fscache_open_file(struct inode *, struct file *);
 
-extern void __nfs_fscache_invalidate_page(struct page *, struct inode *);
-extern int nfs_fscache_release_page(struct page *, gfp_t);
-
 extern int __nfs_readpage_from_fscache(struct nfs_open_context *,
 				       struct inode *, struct page *);
 extern int __nfs_readpages_from_fscache(struct nfs_open_context *,
@@ -100,28 +98,6 @@ extern int __nfs_readpages_from_fscache(struct nfs_open_context *,
 					struct list_head *);
 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
- */
-static inline void nfs_fscache_wait_on_page_write(struct nfs_inode *nfsi,
-						  struct page *page)
-{
-	if (PageFsCache(page))
-		fscache_wait_on_page_write(nfsi->fscache, page);
-}
-
-/*
- * release the caching state associated with a page if undergoing complete page
- * invalidation
- */
-static inline void nfs_fscache_invalidate_page(struct page *page,
-					       struct inode *inode)
-{
-	if (PageFsCache(page))
-		__nfs_fscache_invalidate_page(page, inode);
-}
-
 /*
  * Retrieve a page from an inode data storage object.
  */
@@ -158,20 +134,32 @@ static inline void nfs_read_completion_to_fscache(struct nfs_pgio_header *hdr,
 		__nfs_read_completion_to_fscache(hdr, bytes);
 }
 
-/*
- * Invalidate the contents of fscache for this inode.  This will not sleep.
- */
-static inline void nfs_fscache_invalidate(struct inode *inode)
+static inline void nfs_fscache_update_auxdata(struct nfs_fscache_inode_auxdata *auxdata,
+				  struct nfs_inode *nfsi)
 {
-	fscache_invalidate(NFS_I(inode)->fscache);
+	memset(auxdata, 0, sizeof(*auxdata));
+	auxdata->mtime_sec  = nfsi->vfs_inode.i_mtime.tv_sec;
+	auxdata->mtime_nsec = nfsi->vfs_inode.i_mtime.tv_nsec;
+	auxdata->ctime_sec  = nfsi->vfs_inode.i_ctime.tv_sec;
+	auxdata->ctime_nsec = nfsi->vfs_inode.i_ctime.tv_nsec;
+
+	if (NFS_SERVER(&nfsi->vfs_inode)->nfs_client->rpc_ops->version == 4)
+		auxdata->change_attr = inode_peek_iversion_raw(&nfsi->vfs_inode);
 }
 
 /*
- * Wait for an object to finish being invalidated.
+ * Invalidate the contents of fscache for this inode.  This will not sleep.
  */
-static inline void nfs_fscache_wait_on_invalidate(struct inode *inode)
+static inline void nfs_fscache_invalidate(struct inode *inode)
 {
-	fscache_wait_on_invalidate(NFS_I(inode)->fscache);
+	struct nfs_fscache_inode_auxdata auxdata;
+	struct nfs_inode *nfsi = NFS_I(inode);
+
+	if (nfsi->fscache) {
+		nfs_fscache_update_auxdata(&auxdata, nfsi);
+		fscache_invalidate(nfsi->fscache, &auxdata,
+				   i_size_read(&nfsi->vfs_inode), 0);
+	}
 }
 
 /*
@@ -198,15 +186,6 @@ static inline void nfs_fscache_clear_inode(struct inode *inode) {}
 static inline void nfs_fscache_open_file(struct inode *inode,
 					 struct file *filp) {}
 
-static inline int nfs_fscache_release_page(struct page *page, gfp_t gfp)
-{
-	return 1; /* True: may release page */
-}
-static inline void nfs_fscache_invalidate_page(struct page *page,
-					       struct inode *inode) {}
-static inline void nfs_fscache_wait_on_page_write(struct nfs_inode *nfsi,
-						  struct page *page) {}
-
 static inline int nfs_readpage_from_fscache(struct nfs_open_context *ctx,
 					    struct inode *inode,
 					    struct page *page)
@@ -224,7 +203,6 @@ static inline void nfs_read_completion_to_fscache(struct nfs_pgio_header *hdr,
 						  unsigned long bytes) {}
 
 static inline void nfs_fscache_invalidate(struct inode *inode) {}
-static inline void nfs_fscache_wait_on_invalidate(struct inode *inode) {}
 
 static inline const char *nfs_server_fscache_state(struct nfs_server *server)
 {
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 0bf1f835de01..b9a84f1c1a5c 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1248,7 +1248,6 @@ static int nfs_invalidate_mapping(struct inode *inode, struct address_space *map
 		spin_unlock(&inode->i_lock);
 	}
 	nfs_inc_stats(inode, NFSIOS_DATAINVALIDATE);
-	nfs_fscache_wait_on_invalidate(inode);
 
 	dfprintk(PAGECACHE, "NFS: (%s/%Lu) data cache invalidated\n",
 			inode->i_sb->s_id,
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 639c34fec04a..005eea29e0ec 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -2112,7 +2112,7 @@ int nfs_migrate_page(struct address_space *mapping, struct page *newpage,
 	if (PagePrivate(page))
 		return -EBUSY;
 
-	if (!nfs_fscache_release_page(page, GFP_KERNEL))
+	if (PageFsCache(page))
 		return -EBUSY;
 
 	return migrate_page(mapping, newpage, page, mode);
-- 
1.8.3.1

[RFC PATCH v2 11/14] NFS: Call nfs_fscache_invalidate() when write extends the size of the file

[Dave Wysochanski]

If a write extends the size of the file and fscache is enabled, we
need to invalidate the object with the new size.  Otherwise, the next
read from the cache may fail inside cachefiles_shape_extent() due to
cookie->zero_point being smaller than the size of the file.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/write.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 005eea29e0ec..2da99814da51 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -290,6 +290,7 @@ static void nfs_grow_file(struct page *page, unsigned int offset, unsigned int c
 		goto out;
 	i_size_write(inode, end);
 	NFS_I(inode)->cache_validity &= ~NFS_INO_INVALID_SIZE;
+	nfs_fscache_invalidate(inode);
 	nfs_inc_stats(inode, NFSIOS_EXTENDWRITE);
 out:
 	spin_unlock(&inode->i_lock);
-- 
1.8.3.1

[RFC PATCH v2 12/14] NFS: Invalidate fscache for direct writes

[Dave Wysochanski]

We must call into fscache to invalidate when doing a direct write.
This fixes issues seen with NFSv4.x when direct writes are
followed by non-direct (buffered) reads.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/direct.c   | 2 ++
 fs/nfs/file.c     | 2 +-
 fs/nfs/fscache.h  | 6 +++---
 fs/nfs/inode.c    | 2 +-
 fs/nfs/nfs4proc.c | 2 +-
 fs/nfs/write.c    | 2 +-
 6 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 3d113cf8908a..3360e161dbf1 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -59,6 +59,7 @@
 #include "internal.h"
 #include "iostat.h"
 #include "pnfs.h"
+#include "fscache.h"
 
 #define NFSDBG_FACILITY		NFSDBG_VFS
 
@@ -967,6 +968,7 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter)
 	} else {
 		result = requested;
 	}
+	nfs_fscache_invalidate(inode, FSCACHE_INVAL_DIO_WRITE);
 out_release:
 	nfs_direct_req_release(dreq);
 out:
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 7af528c423c0..0f65617f84c9 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -414,7 +414,7 @@ static void nfs_invalidate_page(struct page *page, unsigned int offset,
 	/* Cancel any unstarted writes on this page */
 	nfs_wb_page_cancel(page_file_mapping(page)->host, page);
 	wait_on_page_fscache(page);
-	nfs_fscache_invalidate(page_file_mapping(page)->host);
+	nfs_fscache_invalidate(page_file_mapping(page)->host, 0);
 }
 
 /*
diff --git a/fs/nfs/fscache.h b/fs/nfs/fscache.h
index de060f61cadd..7bc2f364a931 100644
--- a/fs/nfs/fscache.h
+++ b/fs/nfs/fscache.h
@@ -150,7 +150,7 @@ static inline void nfs_fscache_update_auxdata(struct nfs_fscache_inode_auxdata *
 /*
  * Invalidate the contents of fscache for this inode.  This will not sleep.
  */
-static inline void nfs_fscache_invalidate(struct inode *inode)
+static inline void nfs_fscache_invalidate(struct inode *inode, int flags)
 {
 	struct nfs_fscache_inode_auxdata auxdata;
 	struct nfs_inode *nfsi = NFS_I(inode);
@@ -158,7 +158,7 @@ static inline void nfs_fscache_invalidate(struct inode *inode)
 	if (nfsi->fscache) {
 		nfs_fscache_update_auxdata(&auxdata, nfsi);
 		fscache_invalidate(nfsi->fscache, &auxdata,
-				   i_size_read(&nfsi->vfs_inode), 0);
+				   i_size_read(&nfsi->vfs_inode), flags);
 	}
 }
 
@@ -202,7 +202,7 @@ static inline int nfs_readpages_from_fscache(struct nfs_open_context *ctx,
 static inline void nfs_read_completion_to_fscache(struct nfs_pgio_header *hdr,
 						  unsigned long bytes) {}
 
-static inline void nfs_fscache_invalidate(struct inode *inode) {}
+static inline void nfs_fscache_invalidate(struct inode *inode, int flags) {}
 
 static inline const char *nfs_server_fscache_state(struct nfs_server *server)
 {
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index b9a84f1c1a5c..45067303348c 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -211,7 +211,7 @@ static void nfs_set_cache_invalid(struct inode *inode, unsigned long flags)
 		flags &= ~(NFS_INO_INVALID_DATA|NFS_INO_DATA_INVAL_DEFER);
 	nfsi->cache_validity |= flags;
 	if (flags & NFS_INO_INVALID_DATA)
-		nfs_fscache_invalidate(inode);
+		nfs_fscache_invalidate(inode, 0);
 }
 
 /*
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index e32717fd1169..2602fd633e56 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1179,7 +1179,7 @@ int nfs4_call_sync(struct rpc_clnt *clnt,
 	nfsi->read_cache_jiffies = timestamp;
 	nfsi->attr_gencount = nfs_inc_attr_generation_counter();
 	nfsi->cache_validity &= ~NFS_INO_INVALID_CHANGE;
-	nfs_fscache_invalidate(dir);
+	nfs_fscache_invalidate(dir, 0);
 }
 
 static void
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 2da99814da51..0de914a581d2 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -290,7 +290,7 @@ static void nfs_grow_file(struct page *page, unsigned int offset, unsigned int c
 		goto out;
 	i_size_write(inode, end);
 	NFS_I(inode)->cache_validity &= ~NFS_INO_INVALID_SIZE;
-	nfs_fscache_invalidate(inode);
+	nfs_fscache_invalidate(inode, 0);
 	nfs_inc_stats(inode, NFSIOS_EXTENDWRITE);
 out:
 	spin_unlock(&inode->i_lock);
-- 
1.8.3.1

[RFC PATCH v2 13/14] NFS: Call fscache_resize_cookie() when inode size changes due to setattr

[Dave Wysochanski]

Handle truncate / setattr when fscache is enabled by calling
fscache_resize_cookie().

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 45067303348c..6b814246d07d 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -667,6 +667,7 @@ static int nfs_vmtruncate(struct inode * inode, loff_t offset)
 	spin_unlock(&inode->i_lock);
 	truncate_pagecache(inode, offset);
 	spin_lock(&inode->i_lock);
+	fscache_resize_cookie(nfs_i_fscache(inode), i_size_read(inode));
 out:
 	return err;
 }
-- 
1.8.3.1

[RFC PATCH v2 14/14] NFS: Allow NFS use of new fscache API in build

[Dave Wysochanski]

Add the ability for NFS to build FS-Cache against the new API.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/Kconfig b/fs/nfs/Kconfig
index 8909ef506073..88e1763e02f3 100644
--- a/fs/nfs/Kconfig
+++ b/fs/nfs/Kconfig
@@ -170,7 +170,7 @@ config ROOT_NFS
 
 config NFS_FSCACHE
 	bool "Provide NFS client caching support"
-	depends on NFS_FS=m && FSCACHE_OLD || NFS_FS=y && FSCACHE_OLD=y
+	depends on NFS_FS=m && FSCACHE || NFS_FS=y && FSCACHE=y
 	help
 	  Say Y here if you want NFS data to be cached locally on disc through
 	  the general filesystem cache manager
-- 
1.8.3.1

Re: [Linux-cachefs] [RFC PATCH v2 13/14] NFS: Call fscache_resize_cookie() when inode size changes due to setattr

[Jeff Layton]

On Wed, 2020-07-29 at 10:12 -0400, Dave Wysochanski wrote:
> Handle truncate / setattr when fscache is enabled by calling
> fscache_resize_cookie().
> 
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
>  fs/nfs/inode.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 45067303348c..6b814246d07d 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -667,6 +667,7 @@ static int nfs_vmtruncate(struct inode * inode, loff_t offset)
>  	spin_unlock(&inode->i_lock);
>  	truncate_pagecache(inode, offset);
>  	spin_lock(&inode->i_lock);
> +	fscache_resize_cookie(nfs_i_fscache(inode), i_size_read(inode));
>  out:
>  	return err;
>  }

truncate can happen even when you have no open file descriptors on the
file and therefore w/o the cookie being "used". In the ceph vmtruncate
handling code, I do an explicit use/unuse around this call. Do you need
to do the same here?
-- 
Jeff Layton <jlayton@redhat.com>

Re: [Linux-cachefs] [RFC PATCH v2 13/14] NFS: Call fscache_resize_cookie() when inode size changes due to setattr

[David Wysochanski]

On Thu, Jul 30, 2020 at 2:39 PM Jeff Layton <jlayton@redhat.com> wrote:
>
> On Wed, 2020-07-29 at 10:12 -0400, Dave Wysochanski wrote:
> > Handle truncate / setattr when fscache is enabled by calling
> > fscache_resize_cookie().
> >
> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> > ---
> >  fs/nfs/inode.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > index 45067303348c..6b814246d07d 100644
> > --- a/fs/nfs/inode.c
> > +++ b/fs/nfs/inode.c
> > @@ -667,6 +667,7 @@ static int nfs_vmtruncate(struct inode * inode, loff_t offset)
> >       spin_unlock(&inode->i_lock);
> >       truncate_pagecache(inode, offset);
> >       spin_lock(&inode->i_lock);
> > +     fscache_resize_cookie(nfs_i_fscache(inode), i_size_read(inode));
> >  out:
> >       return err;
> >  }
>
> truncate can happen even when you have no open file descriptors on the
> file and therefore w/o the cookie being "used". In the ceph vmtruncate
> handling code, I do an explicit use/unuse around this call. Do you need
> to do the same here?
> --
> Jeff Layton <jlayton@redhat.com>
>

Actually I think the case you mention is covered by a patch that I've just
added today on top of my v2 posting.
This was the result of looking deeper into a few xfstest failures with
NFSv4.2.  I think this covers the truncate without a file open:

commit 91d6922df9390ca1c090911be6e5c5ab1a79ea83
Author: Dave Wysochanski <dwysocha@redhat.com>
Date:   Thu Jul 30 12:33:40 2020 -0400

    NFS: Call fscache_invalidate() from nfs_invalidate_mapping()

    Be sure to invalidate fscache cookie for any call to
    nfs_invalidate_mapping().

    This patch fixes the following xfstests on NFS4.x:
      generic/240
    as well as fixes the following xfstests on NFSv4.2:
      generic/029 generic/030

    Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 6b814246d07d..62243ec05917 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1233,6 +1233,7 @@ static int nfs_invalidate_mapping(struct inode
*inode, struct address_space *map
        struct nfs_inode *nfsi = NFS_I(inode);
        int ret;

+       nfs_fscache_invalidate(inode, 0);
        if (mapping->nrpages != 0) {
                if (S_ISREG(inode->i_mode)) {
                        ret = nfs_sync_mapping(mapping);

Re: [Linux-cachefs] [RFC PATCH v2 13/14] NFS: Call fscache_resize_cookie() when inode size changes due to setattr

[David Wysochanski]

On Thu, Jul 30, 2020 at 3:23 PM David Wysochanski <dwysocha@redhat.com> wrote:
>
> On Thu, Jul 30, 2020 at 2:39 PM Jeff Layton <jlayton@redhat.com> wrote:
> >
> > On Wed, 2020-07-29 at 10:12 -0400, Dave Wysochanski wrote:
> > > Handle truncate / setattr when fscache is enabled by calling
> > > fscache_resize_cookie().
> > >
> > > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> > > ---
> > >  fs/nfs/inode.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > > index 45067303348c..6b814246d07d 100644
> > > --- a/fs/nfs/inode.c
> > > +++ b/fs/nfs/inode.c
> > > @@ -667,6 +667,7 @@ static int nfs_vmtruncate(struct inode * inode, loff_t offset)
> > >       spin_unlock(&inode->i_lock);
> > >       truncate_pagecache(inode, offset);
> > >       spin_lock(&inode->i_lock);
> > > +     fscache_resize_cookie(nfs_i_fscache(inode), i_size_read(inode));
> > >  out:
> > >       return err;
> > >  }
> >
> > truncate can happen even when you have no open file descriptors on the
> > file and therefore w/o the cookie being "used". In the ceph vmtruncate
> > handling code, I do an explicit use/unuse around this call. Do you need
> > to do the same here?
> > --
> > Jeff Layton <jlayton@redhat.com>
> >
>
> Actually I think the case you mention is covered by a patch that I've just
> added today on top of my v2 posting.
> This was the result of looking deeper into a few xfstest failures with
> NFSv4.2.  I think this covers the truncate without a file open:
>
> commit 91d6922df9390ca1c090911be6e5c5ab1a79ea83
> Author: Dave Wysochanski <dwysocha@redhat.com>
> Date:   Thu Jul 30 12:33:40 2020 -0400
>
>     NFS: Call fscache_invalidate() from nfs_invalidate_mapping()
>
>     Be sure to invalidate fscache cookie for any call to
>     nfs_invalidate_mapping().
>
>     This patch fixes the following xfstests on NFS4.x:
>       generic/240
>     as well as fixes the following xfstests on NFSv4.2:
>       generic/029 generic/030
>
>     Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
>
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 6b814246d07d..62243ec05917 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -1233,6 +1233,7 @@ static int nfs_invalidate_mapping(struct inode
> *inode, struct address_space *map
>         struct nfs_inode *nfsi = NFS_I(inode);
>         int ret;
>
> +       nfs_fscache_invalidate(inode, 0);
>         if (mapping->nrpages != 0) {
>                 if (S_ISREG(inode->i_mode)) {
>                         ret = nfs_sync_mapping(mapping);


Actually the above patch fixes truncates when a file is open, not the
case that Jeff mentions.

To be honest I'm not sure about needing a call to fscache_use/unuse_cookie()
around the call to fscache_resize_cookie().  If the cookie has a
refcount of 1 when it is created, and a file is never opened, so
we never call fscache_use_cookie(), what might happen inside
fscache_resize_cookie()?  The header on use_cookie() says
/*
 * Start using the cookie for I/O.  This prevents the backing object from being
 * reaped by VM pressure.
 */

But we're not using it for I/O in this case.
I will have to dig deeper to be sure, or maybe David H will elaborate further.

Re: [Linux-cachefs] [RFC PATCH v2 13/14] NFS: Call fscache_resize_cookie() when inode size changes due to setattr

[David Howells]

David Wysochanski <dwysocha@redhat.com> wrote:

> To be honest I'm not sure about needing a call to fscache_use/unuse_cookie()
> around the call to fscache_resize_cookie().  If the cookie has a
> refcount of 1 when it is created, and a file is never opened, so
> we never call fscache_use_cookie(), what might happen inside
> fscache_resize_cookie()?  The header on use_cookie() says

I've have afs_setattr() doing use/unuse on the cookie around resize.

David

Re: [Linux-cachefs] [RFC PATCH v2 13/14] NFS: Call fscache_resize_cookie() when inode size changes due to setattr

[David Wysochanski]

On Thu, Jul 30, 2020 at 4:03 PM David Howells <dhowells@redhat.com> wrote:
>
> David Wysochanski <dwysocha@redhat.com> wrote:
>
> > To be honest I'm not sure about needing a call to fscache_use/unuse_cookie()
> > around the call to fscache_resize_cookie().  If the cookie has a
> > refcount of 1 when it is created, and a file is never opened, so
> > we never call fscache_use_cookie(), what might happen inside
> > fscache_resize_cookie()?  The header on use_cookie() says
>
> I've have afs_setattr() doing use/unuse on the cookie around resize.
>
> David
>

Got it and will be fixed in next series.  Thanks!

Re: [Linux-cachefs] [RFC PATCH v2 07/14] NFS: Convert nfs_readpage() and readpages() to new fscache API

[David Wysochanski]

On Wed, Jul 29, 2020 at 10:12 AM Dave Wysochanski <dwysocha@redhat.com> wrote:
>
> This patch converts the NFS read paths to the new fscache API,
> minimizing changes to the existing code.
>
> The new fscache IO path API uses a different mechanism to read
> through the cache.  There are two main read_helper calls:
> - readpage: fscache_read_helper_locked_page()
>   - replaces old API fscache_read_or_alloc_page()
> - readpages: fscache_read_helper_page_list()
>   - replaces old API fscache_read_or_alloc_pages()
>
> Once submitted to the read_helper, if pages are inside the cache
> fscache will call the done() function of fscache_io_request_ops().
> If the pages are not inside the cache, fscache will call issue_op()
> so NFS can go through its normal read code paths, such as
> nfs_pageio_init_read(), nfs_pageio_add_page_read() and
> nfs_pageio_complete_read().
>
> In the read completion code path, from nfs_read_completion() we
> must call into fscache via a cache.io_done() function.  In order
> to call back into fscache via this function, we must save the
> nfs_fscache_req * as a field in the nfs_pgio_header, similar to
> nfs_direct_req.  Note also that when fscache is enabled, the
> read_helper will lock and unlock the pages so in the completion
> path we skip the unlock_page() with fscache.
>
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
>  fs/nfs/fscache.c         | 217 +++++++++++++++++++++++------------------------
>  fs/nfs/fscache.h         |  30 +++----
>  fs/nfs/pagelist.c        |   1 +
>  fs/nfs/read.c            |  12 ++-
>  include/linux/nfs_page.h |   1 +
>  include/linux/nfs_xdr.h  |   1 +
>  6 files changed, 132 insertions(+), 130 deletions(-)
>
> diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
> index a60df88efc40..f641f33fa632 100644
> --- a/fs/nfs/fscache.c
> +++ b/fs/nfs/fscache.c
> @@ -328,73 +328,88 @@ void nfs_fscache_open_file(struct inode *inode, struct file *filp)
>  }
>  EXPORT_SYMBOL_GPL(nfs_fscache_open_file);
>
> -/*
> - * Release the caching state associated with a page, if the page isn't busy
> - * interacting with the cache.
> - * - Returns true (can release page) or false (page busy).
> - */
> -int nfs_fscache_release_page(struct page *page, gfp_t gfp)
> -{
> -       if (PageFsCache(page)) {
> -               struct fscache_cookie *cookie = nfs_i_fscache(page->mapping->host);
> -
> -               BUG_ON(!cookie);
> -               dfprintk(FSCACHE, "NFS: fscache releasepage (0x%p/0x%p/0x%p)\n",
> -                        cookie, page, NFS_I(page->mapping->host));
> -
> -               if (!fscache_maybe_release_page(cookie, page, gfp))
> -                       return 0;
> +struct nfs_fscache_req {
> +       struct fscache_io_request       cache;
> +       struct nfs_readdesc             desc;
> +       refcount_t                      usage;
> +};
>
> -               nfs_inc_fscache_stats(page->mapping->host,
> -                                     NFSIOS_FSCACHE_PAGES_UNCACHED);
> -       }
> +static void nfs_done_io_request(struct fscache_io_request *fsreq)
> +{
> +       struct nfs_fscache_req *req = container_of(fsreq, struct nfs_fscache_req, cache);
> +       struct inode *inode = d_inode(req->desc.ctx->dentry);
>
> -       return 1;
> +       nfs_add_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_OK,
> +                             fsreq->transferred >> PAGE_SHIFT);
>  }
>
> -/*
> - * Release the caching state associated with a page if undergoing complete page
> - * invalidation.
> - */
> -void __nfs_fscache_invalidate_page(struct page *page, struct inode *inode)
> +static void nfs_get_io_request(struct fscache_io_request *fsreq)
>  {
> -       struct fscache_cookie *cookie = nfs_i_fscache(inode);
> +       struct nfs_fscache_req *req = container_of(fsreq, struct nfs_fscache_req, cache);
>
> -       BUG_ON(!cookie);
> +       refcount_inc(&req->usage);
> +}
>
> -       dfprintk(FSCACHE, "NFS: fscache invalidatepage (0x%p/0x%p/0x%p)\n",
> -                cookie, page, NFS_I(inode));
> +static void nfs_put_io_request(struct fscache_io_request *fsreq)
> +{
> +       struct nfs_fscache_req *req = container_of(fsreq, struct nfs_fscache_req, cache);
>
> -       fscache_wait_on_page_write(cookie, page);
> +       if (refcount_dec_and_test(&req->usage)) {
> +               put_nfs_open_context(req->desc.ctx);
> +               fscache_free_io_request(fsreq);
> +               kfree(req);
> +       }
> +}
>
> -       BUG_ON(!PageLocked(page));
> -       fscache_uncache_page(cookie, page);
> -       nfs_inc_fscache_stats(page->mapping->host,
> -                             NFSIOS_FSCACHE_PAGES_UNCACHED);
> +static void nfs_issue_op(struct fscache_io_request *fsreq)
> +{
> +       struct nfs_fscache_req *req = container_of(fsreq, struct nfs_fscache_req, cache);
> +       struct inode *inode = req->cache.mapping->host;
> +       struct page *page;
> +       pgoff_t index = req->cache.pos >> PAGE_SHIFT;
> +       pgoff_t last = index + req->cache.nr_pages - 1;
> +
> +       nfs_add_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_FAIL,
> +                             req->cache.nr_pages);
> +       nfs_get_io_request(fsreq);
> +       nfs_pageio_init_read(&req->desc.pgio, inode, false,
> +                            &nfs_async_read_completion_ops);
> +
> +       for (; index <= last; index++) {
> +               page = find_get_page(req->cache.mapping, index);
> +               BUG_ON(!page);
> +               req->cache.error = readpage_async_filler(&req->desc, page);
> +               if (req->cache.error < 0)
> +                       break;
> +       }
> +       nfs_pageio_complete_read(&req->desc.pgio, inode);
>  }
>

When testing pnfs, I realize the above is wrong / needs fixed up.
The high-level problem is that xfstest generic/001 panics inside
fscache / mm due to a page either not having PG_fscache set on it,
or a page not having PG_locked.  This is due to NFS calling into fscache
twice to write the same data (see nfs_read_completion_to_fscache)
with essentially the same fscache_io_request.

I think the root is the above code does not handle the
nfs_pageio_descriptor and properly when we hit the following code path:
nfs_issue_op()
  readpage_async_filler()
    nfs_pageio_add_request()
      nfs_pageio_add_request_mirror()
         __nfs_pageio_add_request()
            nfs_pageio_doio()  /* Can't coalesce any more, so do I/O */

I overlooked the above call to nfs_pageio_doio() and erroneously
assumed this was only called via nfs_pageio_complete().

> -/*
> - * Handle completion of a page being read from the cache.
> - * - Called in process (keventd) context.
> - */
> -static void nfs_readpage_from_fscache_complete(struct page *page,
> -                                              void *context,
> -                                              int error)
> +static struct fscache_io_request_ops nfs_fscache_req_ops = {
> +       .issue_op       = nfs_issue_op,
> +       .done           = nfs_done_io_request,
> +       .get            = nfs_get_io_request,
> +       .put            = nfs_put_io_request,
> +};
> +
> +struct nfs_fscache_req *nfs_alloc_io_request(struct nfs_open_context *ctx,
> +                                           struct address_space *mapping)
>  {
> -       dfprintk(FSCACHE,
> -                "NFS: readpage_from_fscache_complete (0x%p/0x%p/%d)\n",
> -                page, context, error);
> -
> -       /* if the read completes with an error, we just unlock the page and let
> -        * the VM reissue the readpage */
> -       if (!error) {
> -               SetPageUptodate(page);
> -               unlock_page(page);
> -       } else {
> -               error = nfs_readpage_async(context, page->mapping->host, page);
> -               if (error)
> -                       unlock_page(page);
> +       struct nfs_fscache_req *req;
> +       struct inode *inode = mapping->host;
> +
> +       req = kzalloc(sizeof(*req), GFP_KERNEL);
> +       if (req) {
> +               refcount_set(&req->usage, 1);
> +               req->cache.mapping = mapping;
> +               req->desc.ctx = get_nfs_open_context(ctx);
> +
> +               fscache_init_io_request(&req->cache, nfs_i_fscache(inode),
> +                                       &nfs_fscache_req_ops);
> +               req->desc.pgio.pg_fsc_req = req;
>         }
> +
> +       return req;
>  }
>
>  /*
> @@ -403,36 +418,38 @@ static void nfs_readpage_from_fscache_complete(struct page *page,
>  int __nfs_readpage_from_fscache(struct nfs_open_context *ctx,
>                                 struct inode *inode, struct page *page)
>  {
> +       struct nfs_fscache_req *req;
>         int ret;
>
>         dfprintk(FSCACHE,
>                  "NFS: readpage_from_fscache(fsc:%p/p:%p(i:%lx f:%lx)/0x%p)\n",
>                  nfs_i_fscache(inode), page, page->index, page->flags, inode);
>
> -       ret = fscache_read_or_alloc_page(nfs_i_fscache(inode),
> -                                        page,
> -                                        nfs_readpage_from_fscache_complete,
> -                                        ctx,
> -                                        GFP_KERNEL);
> +       req = nfs_alloc_io_request(ctx, page_file_mapping(page));
> +       if (IS_ERR(req))
> +               return PTR_ERR(req);
> +
> +       ret = fscache_read_helper_locked_page(&req->cache, page, ULONG_MAX);
> +
> +       nfs_put_io_request(&req->cache);
>
>         switch (ret) {
> -       case 0: /* read BIO submitted (page in fscache) */
> -               dfprintk(FSCACHE,
> -                        "NFS:    readpage_from_fscache: BIO submitted\n");
> +       case 0: /* read submitted */
> +               dfprintk(FSCACHE, "NFS:    readpage_from_fscache: submitted\n");
>                 nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_OK);
>                 return ret;
>
>         case -ENOBUFS: /* inode not in cache */
>         case -ENODATA: /* page not in cache */
>                 nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_FAIL);
> -               dfprintk(FSCACHE,
> -                        "NFS:    readpage_from_fscache %d\n", ret);
> +               dfprintk(FSCACHE, "NFS:    readpage_from_fscache %d\n", ret);
>                 return 1;
>
>         default:
>                 dfprintk(FSCACHE, "NFS:    readpage_from_fscache %d\n", ret);
>                 nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_FAIL);
>         }
> +
>         return ret;
>  }
>
> @@ -442,75 +459,57 @@ int __nfs_readpage_from_fscache(struct nfs_open_context *ctx,
>  int __nfs_readpages_from_fscache(struct nfs_open_context *ctx,
>                                  struct inode *inode,
>                                  struct address_space *mapping,
> -                                struct list_head *pages,
> -                                unsigned *nr_pages)
> +                                struct list_head *pages)
>  {
> -       unsigned npages = *nr_pages;
> +       struct nfs_fscache_req *req;
>         int ret;
>
> -       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_readpages_from_fscache (0x%p/0x%p)\n",
> +                nfs_i_fscache(inode), inode);
> +
> +       while (!list_empty(pages)) {
> +               req = nfs_alloc_io_request(ctx, mapping);
> +               if (IS_ERR(req))
> +                       return PTR_ERR(req);
> +
> +               ret = fscache_read_helper_page_list(&req->cache, pages,
> +                                                   ULONG_MAX);
> +               nfs_put_io_request(&req->cache);
> +               if (ret < 0)
> +                       break;
> +       }
>
>         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");
> +                        "NFS: nfs_readpages_from_fscache: submitted\n");
>
>                 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);
> +                        "NFS: nfs_readpages_from_fscache: no page: %d\n", ret);
>                 return 1;
>
>         default:
>                 dfprintk(FSCACHE,
> -                        "NFS: nfs_getpages_from_fscache: ret  %d\n", ret);
> +                        "NFS: nfs_readpages_from_fscache: ret  %d\n", ret);
>         }
> -
>         return ret;
>  }
>
>  /*
> - * Store a newly fetched page in fscache
> - * - PG_fscache must be set on the page
> + * Store a newly fetched data in fscache
>   */
> -void __nfs_readpage_to_fscache(struct inode *inode, struct page *page, int sync)
> +void __nfs_read_completion_to_fscache(struct nfs_pgio_header *hdr, unsigned long bytes)
>  {
> -       int ret;
> +       struct nfs_fscache_req *fsc_req = hdr->fsc_req;
>
> -       dfprintk(FSCACHE,
> -                "NFS: readpage_to_fscache(fsc:%p/p:%p(i:%lx f:%lx)/%d)\n",
> -                nfs_i_fscache(inode), page, page->index, page->flags, sync);
> -
> -       ret = fscache_write_page(nfs_i_fscache(inode), page,
> -                                inode->i_size, GFP_KERNEL);
> -       dfprintk(FSCACHE,
> -                "NFS:     readpage_to_fscache: p:%p(i:%lu f:%lx) ret %d\n",
> -                page, page->index, page->flags, ret);
> -
> -       if (ret != 0) {
> -               fscache_uncache_page(nfs_i_fscache(inode), page);
> -               nfs_inc_fscache_stats(inode,
> -                                     NFSIOS_FSCACHE_PAGES_WRITTEN_FAIL);
> -               nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_UNCACHED);
> -       } else {
> -               nfs_inc_fscache_stats(inode,
> -                                     NFSIOS_FSCACHE_PAGES_WRITTEN_OK);
> +       if (fsc_req && fsc_req->cache.io_done) {
> +               fsc_req->cache.transferred = min_t(long long, bytes, fsc_req->cache.len);
> +               set_bit(FSCACHE_IO_DATA_FROM_SERVER, &fsc_req->cache.flags);
> +               fsc_req->cache.io_done(&fsc_req->cache);
> +               nfs_put_io_request(&fsc_req->cache);
>         }
>  }
> diff --git a/fs/nfs/fscache.h b/fs/nfs/fscache.h
> index 6754c8607230..d61721832838 100644
> --- a/fs/nfs/fscache.h
> +++ b/fs/nfs/fscache.h
> @@ -100,8 +100,9 @@ extern int __nfs_readpage_from_fscache(struct nfs_open_context *,
>                                        struct inode *, struct page *);
>  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);
> +                                       struct list_head *);
> +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
> @@ -142,25 +143,22 @@ static inline int nfs_readpage_from_fscache(struct nfs_open_context *ctx,
>  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)
> +                                            struct list_head *pages)
>  {
>         if (NFS_I(inode)->fscache)
> -               return __nfs_readpages_from_fscache(ctx, inode, mapping, pages,
> -                                                   nr_pages);
> +               return __nfs_readpages_from_fscache(ctx, inode, mapping, pages);
>         return -ENOBUFS;
>  }
>
>  /*
> - * Store a page newly fetched from the server in an inode data storage object
> + * Store pages newly fetched from the server in an inode data storage object
>   * in the cache.
>   */
> -static inline void nfs_readpage_to_fscache(struct inode *inode,
> -                                          struct page *page,
> -                                          int sync)
> +static inline void nfs_read_completion_to_fscache(struct nfs_pgio_header *hdr,
> +                                                 unsigned long bytes)
>  {
> -       if (PageFsCache(page))
> -               __nfs_readpage_to_fscache(inode, page, sync);
> +       if (NFS_I(hdr->inode)->fscache)
> +               __nfs_read_completion_to_fscache(hdr, bytes);
>  }
>
>  /*
> @@ -221,14 +219,12 @@ static inline int nfs_readpage_from_fscache(struct nfs_open_context *ctx,
>  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)
> +                                            struct list_head *pages)
>  {
>         return -ENOBUFS;
>  }
> -static inline void nfs_readpage_to_fscache(struct inode *inode,
> -                                          struct page *page, int sync) {}
> -
> +static inline void nfs_read_completion_to_fscache(struct nfs_pgio_header *hdr,
> +                                                 unsigned long bytes) {}
>
>  static inline void nfs_fscache_invalidate(struct inode *inode) {}
>  static inline void nfs_fscache_wait_on_invalidate(struct inode *inode) {}
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index 6ea4cac41e46..c8073b3667d9 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -52,6 +52,7 @@ void nfs_pgheader_init(struct nfs_pageio_descriptor *desc,
>         hdr->good_bytes = mirror->pg_count;
>         hdr->io_completion = desc->pg_io_completion;
>         hdr->dreq = desc->pg_dreq;
> +       hdr->fsc_req = desc->pg_fsc_req;
>         hdr->release = release;
>         hdr->completion_ops = desc->pg_completion_ops;
>         if (hdr->completion_ops->init_hdr)
> diff --git a/fs/nfs/read.c b/fs/nfs/read.c
> index 13266eda8f60..c92862c83a7f 100644
> --- a/fs/nfs/read.c
> +++ b/fs/nfs/read.c
> @@ -124,10 +124,13 @@ static void nfs_readpage_release(struct nfs_page *req, int error)
>                 struct address_space *mapping = page_file_mapping(page);
>
>                 if (PageUptodate(page))
> -                       nfs_readpage_to_fscache(inode, page, 0);
> +                       ; /* FIXME: review fscache page error handling */
>                 else if (!PageError(page) && !PagePrivate(page))
>                         generic_error_remove_page(mapping, page);
> -               unlock_page(page);
> +               if (nfs_i_fscache(inode))
> +                       put_page(page); /* See nfs_issue_op() */
> +               else
> +                       unlock_page(page);
>         }
>         nfs_release_request(req);
>  }
> @@ -181,6 +184,8 @@ static void nfs_read_completion(struct nfs_pgio_header *hdr)
>                 nfs_list_remove_request(req);
>                 nfs_readpage_release(req, error);
>         }
> +       /* FIXME: Review error handling before writing to fscache */
> +       nfs_read_completion_to_fscache(hdr, bytes);
>  out:
>         hdr->release(hdr);
>  }
> @@ -415,8 +420,7 @@ int nfs_readpages(struct file *filp, struct address_space *mapping,
>         /* 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_readpages_from_fscache(desc.ctx, inode, mapping, pages);
>         if (ret == 0)
>                 goto read_complete; /* all pages were read */
>
> diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
> index c32c15216da3..cf4b1a62108e 100644
> --- a/include/linux/nfs_page.h
> +++ b/include/linux/nfs_page.h
> @@ -97,6 +97,7 @@ struct nfs_pageio_descriptor {
>         struct pnfs_layout_segment *pg_lseg;
>         struct nfs_io_completion *pg_io_completion;
>         struct nfs_direct_req   *pg_dreq;
> +       struct nfs_fscache_req  *pg_fsc_req; /* fscache req - may be NULL */
>         unsigned int            pg_bsize;       /* default bsize for mirrors */
>
>         u32                     pg_mirror_count;
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 5fd0a9ef425f..746548676a51 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1535,6 +1535,7 @@ struct nfs_pgio_header {
>         const struct nfs_rw_ops *rw_ops;
>         struct nfs_io_completion *io_completion;
>         struct nfs_direct_req   *dreq;
> +       struct nfs_fscache_req  *fsc_req;  /* fscache req - may be NULL */
>
>         int                     pnfs_error;
>         int                     error;          /* merge with pnfs_error */
> --
> 1.8.3.1
>
> --
> Linux-cachefs mailing list
> Linux-cachefs@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-cachefs
>