nfs: fix nfs_writepage()

I noticed that ->writepage() didn't actually do anything since it doesn't generate a write request for the given page.

The below is an attempt at fixing it, and it works for me. Although I'm not quite sure its the proper fix.

Signed-off-by Peter Zijlstra <[email protected]>

---
 fs/nfs/write.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Index: linux-2.6/fs/nfs/write.c
===================================================================

fs/nfs/write.c +13 -0

@@ -332,10 +332,30 @@ static int nfs_page_async_flush(struct n
 static int nfs_do_writepage(struct page *page, struct writeback_control *wbc, struct nfs_pageio_descriptor *pgio)
 {
 	struct inode *inode = page_file_mapping(page)->host;
+	struct rpc_cred *cred;
+	struct nfs_open_context *ctx;
+	int status;
 
 	nfs_inc_stats(inode, NFSIOS_VFSWRITEPAGE);
 	nfs_add_stats(inode, NFSIOS_WRITEPAGES, 1);
 
+	cred = rpcauth_lookupcred(NFS_CLIENT(inode)->cl_auth, 0);
+	if (IS_ERR(cred))
+		return PTR_ERR(cred);
+
+	ctx = nfs_find_open_context(inode, cred, FMODE_WRITE);
+	if (!ctx)
+		return -EBADF;
+
+	status = nfs_writepage_setup(ctx, page, 0, nfs_page_length(page));
+

Quoted 42 lines · Peter Zijlstra

I noticed that ->writepage() didn't actually do anything since it doesn't generate a write request for the given page.

The below is an attempt at fixing it, and it works for me. Although I'm not quite sure its the proper fix.

Signed-off-by Peter Zijlstra <[email protected]>

---
 fs/nfs/write.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Index: linux-2.6/fs/nfs/write.c
===================================================================

fs/nfs/write.c +19 -0

@@ -332,10 +332,30 @@ static int nfs_page_async_flush(struct n
 static int nfs_do_writepage(struct page *page, struct writeback_control *wbc, struct nfs_pageio_descriptor *pgio)
 {
 	struct inode *inode = page_file_mapping(page)->host;
+	struct rpc_cred *cred;
+	struct nfs_open_context *ctx;
+	int status;
 
 	nfs_inc_stats(inode, NFSIOS_VFSWRITEPAGE);
 	nfs_add_stats(inode, NFSIOS_WRITEPAGES, 1);
 
+	cred = rpcauth_lookupcred(NFS_CLIENT(inode)->cl_auth, 0);
+	if (IS_ERR(cred))
+		return PTR_ERR(cred);
+
+	ctx = nfs_find_open_context(inode, cred, FMODE_WRITE);
+	if (!ctx)
+		return -EBADF;
+
+	status = nfs_writepage_setup(ctx, page, 0, nfs_page_length(page));
+
+	put_nfs_open_context(ctx);
+
+	if (status < 0) {
+		nfs_set_pageerror(page);
+		return status;
+	}

fs/nfs/write.c static int nfs_page_async_flush(struct n · line 361

Quoted 4 lines

fs/nfs/write.c +1 -0

+
 	nfs_pageio_cond_complete(pgio, page_file_index(page));
 	return nfs_page_async_flush(pgio, page);
 }

NACK. writepage()'s job is simply to start writeout on the page. nfs_vm_page_mkwrite(), and nfs_commit_write() actually set up the nfs_page structure before we ever get to writepage().

The thing is, swapper_space just calls ->writepage() and expects the page to be written out. So either the a_ops usage of swapper_space is deviant or NFS' is.

Quoted 3 lines · Peter Zijlstra

The thing is, swapper_space just calls ->writepage() and expects the page to be written out. So either the a_ops usage of swapper_space is deviant or NFS' is.

Could somebody please document WTF writepage() is supposed to do, and WTF page_mkwrite() is for?

I thought that page_mkwrite() was supposed to finally allow us to deal with dirty pages in a clean manner: the caller gets to tell the filesystem that it wants the entire page written out, and then dirties the page. What is the point if the VM then expects to be able to circumvent this?

Put differently:
* _who_ is dirtying the page when the swapper is trying to write the page out?
* why are they not calling either page_mkwrite() or commit_write()?

Quoted 5 lines · Trond Myklebust

Put differently:
* _who_ is dirtying the page when the swapper is trying to write the page out?
* why are they not calling either page_mkwrite() or commit_write()?

Apparently Peter has some broken patches in his tree that cause this. It can for sure not happen in mainline and that's absolutely intentional.

Quoted 2 lines

Apparently Peter has some broken patches in his tree that cause this. It can for sure not happen in mainline and that's absolutely intentional.

Yeah, I got fooled by thinking swapper_space was 'proper' usage.

Quoted 6 lines · Trond Myklebust

Quoted 3 lines · Peter Zijlstra

The thing is, swapper_space just calls ->writepage() and expects the page to be written out. So either the a_ops usage of swapper_space is deviant or NFS' is.

Quoted 8 lines

Could somebody please document WTF writepage() is supposed to do, and WTF page_mkwrite() is for?

I thought that page_mkwrite() was supposed to finally allow us to deal with dirty pages in a clean manner: the caller gets to tell the filesystem that it wants the entire page written out, and then dirties the page. What is the point if the VM then expects to be able to circumvent this?

Put differently:
* _who_ is dirtying the page when the swapper is trying to write the page out?
* why are they not calling either page_mkwrite() or commit_write()?

Quoted 16 lines · Trond Myklebust

Quoted 14 lines · Trond Myklebust

Quoted 3 lines · Peter Zijlstra

The thing is, swapper_space just calls ->writepage() and expects the page to be written out. So either the a_ops usage of swapper_space is deviant or NFS' is.

Could somebody please document WTF writepage() is supposed to do, and WTF page_mkwrite() is for?

I thought that page_mkwrite() was supposed to finally allow us to deal with dirty pages in a clean manner: the caller gets to tell the filesystem that it wants the entire page written out, and then dirties the page. What is the point if the VM then expects to be able to circumvent this?

Quoted 5 lines

Put differently:
* _who_ is dirtying the page when the swapper is trying to write the page out?
* why are they not calling either page_mkwrite() or commit_write()?

I'm writing anonymous pages (I'm the crazy person doing swap over NFS). And anonymous is dirty by default.

Quoted 17 lines · Peter Zijlstra

Quoted 16 lines · Trond Myklebust

Quoted 14 lines · Trond Myklebust

Quoted 3 lines · Peter Zijlstra

The thing is, swapper_space just calls ->writepage() and expects the page to be written out. So either the a_ops usage of swapper_space is deviant or NFS' is.

Could somebody please document WTF writepage() is supposed to do, and WTF page_mkwrite() is for?

I thought that page_mkwrite() was supposed to finally allow us to deal with dirty pages in a clean manner: the caller gets to tell the filesystem that it wants the entire page written out, and then dirties the page. What is the point if the VM then expects to be able to circumvent this?

Quoted 8 lines

Quoted 5 lines

Put differently:
* _who_ is dirtying the page when the swapper is trying to write the page out?
* why are they not calling either page_mkwrite() or commit_write()?

I'm writing anonymous pages (I'm the crazy person doing swap over NFS). And anonymous is dirty by default.

I'd really prefer to _know_ that these writes are coming from the swapper. That makes it possible to give them a correct credential and open context.

Could you perhaps funnel them through a new a_op->swap_out() and default back to ->writepage() for those filesystems that don't define it?

Quoted 27 lines · Trond Myklebust

Quoted 25 lines · Peter Zijlstra

Quoted 16 lines · Trond Myklebust

Quoted 6 lines · Trond Myklebust

Quoted 3 lines · Peter Zijlstra

The thing is, swapper_space just calls ->writepage() and expects the page to be written out. So either the a_ops usage of swapper_space is deviant or NFS' is.

Quoted 8 lines

Could somebody please document WTF writepage() is supposed to do, and WTF page_mkwrite() is for?

I thought that page_mkwrite() was supposed to finally allow us to deal with dirty pages in a clean manner: the caller gets to tell the filesystem that it wants the entire page written out, and then dirties the page. What is the point if the VM then expects to be able to circumvent this?

Quoted 5 lines

Put differently:
* _who_ is dirtying the page when the swapper is trying to write the page out?
* why are they not calling either page_mkwrite() or commit_write()?

I'm writing anonymous pages (I'm the crazy person doing swap over NFS). And anonymous is dirty by default.

Quoted 6 lines

I'd really prefer to _know_ that these writes are coming from the swapper. That makes it possible to give them a correct credential and open context.

Could you perhaps funnel them through a new a_op->swap_out() and default back to ->writepage() for those filesystems that don't define it?

Sure, will do that.

Quoted 32 lines · Peter Zijlstra

Quoted 31 lines · Trond Myklebust

Quoted 17 lines · Peter Zijlstra

Quoted 16 lines · Trond Myklebust

Quoted 14 lines · Trond Myklebust

Quoted 3 lines · Peter Zijlstra

The thing is, swapper_space just calls ->writepage() and expects the page to be written out. So either the a_ops usage of swapper_space is deviant or NFS' is.

Could somebody please document WTF writepage() is supposed to do, and WTF page_mkwrite() is for?

I thought that page_mkwrite() was supposed to finally allow us to deal with dirty pages in a clean manner: the caller gets to tell the filesystem that it wants the entire page written out, and then dirties the page. What is the point if the VM then expects to be able to circumvent this?

Quoted 8 lines

Quoted 5 lines

Put differently:
* _who_ is dirtying the page when the swapper is trying to write the page out?
* why are they not calling either page_mkwrite() or commit_write()?

I'm writing anonymous pages (I'm the crazy person doing swap over NFS). And anonymous is dirty by default.

I'd really prefer to _know_ that these writes are coming from the swapper. That makes it possible to give them a correct credential and open context.

Quoted 4 lines

Quoted 2 lines

Could you perhaps funnel them through a new a_op->swap_out() and default back to ->writepage() for those filesystems that don't define it?

Sure, will do that.

OTOH these pages will have PageSwapCache() and IS_SWAPFILE(inode) will be true, so ->writepage() can already know. Do you still prefer ->swap_out() ?