[PATCH] nfs: fix nfs_writepage()

[PATCH] nfs: fix nfs_writepage()

[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 <a.p.zijlstra@chello.nl>
---
 fs/nfs/write.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Index: linux-2.6/fs/nfs/write.c
===================================================================
--- linux-2.6.orig/fs/nfs/write.c
+++ linux-2.6/fs/nfs/write.c
@@ -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;
+	}
+
 	nfs_pageio_cond_complete(pgio, page_file_index(page));
 	return nfs_page_async_flush(pgio, page);
 }

Re: [PATCH] nfs: fix nfs_writepage()

[Trond Myklebust]

On Wed, 2007-10-17 at 15:40 +0200, Peter Zijlstra wrote:
> 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 <a.p.zijlstra@chello.nl>
> ---
>  fs/nfs/write.c |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> Index: linux-2.6/fs/nfs/write.c
> ===================================================================
> --- linux-2.6.orig/fs/nfs/write.c
> +++ linux-2.6/fs/nfs/write.c
> @@ -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;
> +	}
> +
>  	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().

Cheers
  Trond

Re: [PATCH] nfs: fix nfs_writepage()

[Peter Zijlstra]

On Wed, 2007-10-17 at 11:25 -0400, Trond Myklebust wrote:
> On Wed, 2007-10-17 at 15:40 +0200, Peter Zijlstra wrote:
> > 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 <a.p.zijlstra@chello.nl>
> > ---
> >  fs/nfs/write.c |   20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > Index: linux-2.6/fs/nfs/write.c
> > ===================================================================
> > --- linux-2.6.orig/fs/nfs/write.c
> > +++ linux-2.6/fs/nfs/write.c
> > @@ -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;
> > +	}
> > +
> >  	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().

Do all a_ops require this? From what I can gather other address_spaces
do not require this, but I might be wrong.

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.

Re: [PATCH] nfs: fix nfs_writepage()

[Trond Myklebust]

On Wed, 2007-10-17 at 17:30 +0200, Peter Zijlstra wrote:

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

Trond

Re: [PATCH] nfs: fix nfs_writepage()

[Trond Myklebust]

On Wed, 2007-10-17 at 11:45 -0400, Trond Myklebust wrote:
> On Wed, 2007-10-17 at 17:30 +0200, Peter Zijlstra wrote:
> 
> > 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()?

Trond

Re: [PATCH] nfs: fix nfs_writepage()

[Christoph Hellwig]

On Wed, Oct 17, 2007 at 11:47:54AM -0400, Trond Myklebust wrote:
> 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.

Re: [PATCH] nfs: fix nfs_writepage()

[Peter Zijlstra]

On Wed, 2007-10-17 at 11:47 -0400, Trond Myklebust wrote:
> On Wed, 2007-10-17 at 11:45 -0400, Trond Myklebust wrote:
> > On Wed, 2007-10-17 at 17:30 +0200, Peter Zijlstra wrote:
> > 
> > > 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()?

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

Re: [PATCH] nfs: fix nfs_writepage()

[Peter Zijlstra]

On Wed, 2007-10-17 at 16:55 +0100, Christoph Hellwig wrote:
> On Wed, Oct 17, 2007 at 11:47:54AM -0400, Trond Myklebust wrote:
> > 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.

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

Re: [PATCH] nfs: fix nfs_writepage()

[Christoph Hellwig]

On Wed, Oct 17, 2007 at 06:02:45PM +0200, Peter Zijlstra wrote:
> > 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.

The swapper space is and always has been special because it's not owned
by a filesystem but rather the VM, so it can be sloppy about some things.

Re: [PATCH] nfs: fix nfs_writepage()

[Trond Myklebust]

On Wed, 2007-10-17 at 17:57 +0200, Peter Zijlstra wrote:
> On Wed, 2007-10-17 at 11:47 -0400, Trond Myklebust wrote:
> > On Wed, 2007-10-17 at 11:45 -0400, Trond Myklebust wrote:
> > > On Wed, 2007-10-17 at 17:30 +0200, Peter Zijlstra wrote:
> > > 
> > > > 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()?
> 
> 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?

Re: [PATCH] nfs: fix nfs_writepage()

[Peter Zijlstra]

On Wed, 2007-10-17 at 12:24 -0400, Trond Myklebust wrote:
> On Wed, 2007-10-17 at 17:57 +0200, Peter Zijlstra wrote:
> > On Wed, 2007-10-17 at 11:47 -0400, Trond Myklebust wrote:
> > > On Wed, 2007-10-17 at 11:45 -0400, Trond Myklebust wrote:
> > > > On Wed, 2007-10-17 at 17:30 +0200, Peter Zijlstra wrote:
> > > > 
> > > > > 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()?
> > 
> > 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?

Sure, will do that.

Re: [PATCH] nfs: fix nfs_writepage()

[Peter Zijlstra]

On Wed, 2007-10-17 at 18:26 +0200, Peter Zijlstra wrote:
> On Wed, 2007-10-17 at 12:24 -0400, Trond Myklebust wrote:
> > On Wed, 2007-10-17 at 17:57 +0200, Peter Zijlstra wrote:
> > > On Wed, 2007-10-17 at 11:47 -0400, Trond Myklebust wrote:
> > > > On Wed, 2007-10-17 at 11:45 -0400, Trond Myklebust wrote:
> > > > > On Wed, 2007-10-17 at 17:30 +0200, Peter Zijlstra wrote:
> > > > > 
> > > > > > 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()?
> > > 
> > > 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?
> 
> 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() ?

Re: [PATCH] nfs: fix nfs_writepage()

[Trond Myklebust]

On Wed, 2007-10-17 at 19:36 +0200, Peter Zijlstra wrote:
> On Wed, 2007-10-17 at 18:26 +0200, Peter Zijlstra wrote:
> > On Wed, 2007-10-17 at 12:24 -0400, Trond Myklebust wrote:
> > > On Wed, 2007-10-17 at 17:57 +0200, Peter Zijlstra wrote:
> > > > On Wed, 2007-10-17 at 11:47 -0400, Trond Myklebust wrote:
> > > > > On Wed, 2007-10-17 at 11:45 -0400, Trond Myklebust wrote:
> > > > > > On Wed, 2007-10-17 at 17:30 +0200, Peter Zijlstra wrote:
> > > > > > 
> > > > > > > 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()?
> > > > 
> > > > 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?
> > 
> > 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() ?

I think it is cleaner. PageSwapCache() is currently internal to the mm
layer, and doesn't need to be exported here. Then there is the issue of
overloading ->writepage(), which I don't like at all: it muddies the
semantics.

As for IS_SWAPFILE(), it would be nice to get rid of that one day...

Cheers
  Trond