2019-02-06 11:09:46

by Benjamin Coddington

[permalink] [raw]
Subject: [PATCH v2] NFS: Don't use page_file_mapping after removing the page

If nfs_page_async_flush() removes the page from the mapping, then we can't
use page_file_mapping() on it as nfs_updatepate() is wont to do when
receiving an error. Instead, push the mapping to the stack before the page
is possibly truncated.

Fixes: 8fc75bed96bb ("NFS: Fix up return value on fatal errors in nfs_page_async_flush()")
Signed-off-by: Benjamin Coddington <[email protected]>
---
fs/nfs/write.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index f12cb31a41e5..d09c9f878141 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -238,9 +238,9 @@ static void nfs_grow_file(struct page *page, unsigned int offset, unsigned int c
}

/* A writeback failed: mark the page as bad, and invalidate the page cache */
-static void nfs_set_pageerror(struct page *page)
+static void nfs_set_pageerror(struct address_space *mapping)
{
- nfs_zap_mapping(page_file_mapping(page)->host, page_file_mapping(page));
+ nfs_zap_mapping(mapping->host, mapping);
}

/*
@@ -994,7 +994,7 @@ static void nfs_write_completion(struct nfs_pgio_header *hdr)
nfs_list_remove_request(req);
if (test_bit(NFS_IOHDR_ERROR, &hdr->flags) &&
(hdr->good_bytes < bytes)) {
- nfs_set_pageerror(req->wb_page);
+ nfs_set_pageerror(page_file_mapping(req->wb_page));
nfs_context_set_write_error(req->wb_context, hdr->error);
goto remove_req;
}
@@ -1348,7 +1348,8 @@ int nfs_updatepage(struct file *file, struct page *page,
unsigned int offset, unsigned int count)
{
struct nfs_open_context *ctx = nfs_file_open_context(file);
- struct inode *inode = page_file_mapping(page)->host;
+ struct address_space *mapping = page_file_mapping(page);
+ struct inode *inode = mapping->host;
int status = 0;

nfs_inc_stats(inode, NFSIOS_VFSUPDATEPAGE);
@@ -1366,7 +1367,7 @@ int nfs_updatepage(struct file *file, struct page *page,

status = nfs_writepage_setup(ctx, page, offset, count);
if (status < 0)
- nfs_set_pageerror(page);
+ nfs_set_pageerror(mapping);
else
__set_page_dirty_nobuffers(page);
out:
--
2.14.3



2019-02-12 18:52:40

by Benjamin Coddington

[permalink] [raw]
Subject: Re: [PATCH v2] NFS: Don't use page_file_mapping after removing the page

Hi Trond and Anna,

I want to bump this. The problem we created is easier to hit now than
the problem we were fixing, so something should be done for 5.0 if
possible.

Ben

On 6 Feb 2019, at 6:09, Benjamin Coddington wrote:

> If nfs_page_async_flush() removes the page from the mapping, then we
> can't
> use page_file_mapping() on it as nfs_updatepate() is wont to do when
> receiving an error. Instead, push the mapping to the stack before the
> page
> is possibly truncated.
>
> Fixes: 8fc75bed96bb ("NFS: Fix up return value on fatal errors in
> nfs_page_async_flush()")
> Signed-off-by: Benjamin Coddington <[email protected]>
> ---
> fs/nfs/write.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index f12cb31a41e5..d09c9f878141 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -238,9 +238,9 @@ static void nfs_grow_file(struct page *page,
> unsigned int offset, unsigned int c
> }
>
> /* A writeback failed: mark the page as bad, and invalidate the page
> cache */
> -static void nfs_set_pageerror(struct page *page)
> +static void nfs_set_pageerror(struct address_space *mapping)
> {
> - nfs_zap_mapping(page_file_mapping(page)->host,
> page_file_mapping(page));
> + nfs_zap_mapping(mapping->host, mapping);
> }
>
> /*
> @@ -994,7 +994,7 @@ static void nfs_write_completion(struct
> nfs_pgio_header *hdr)
> nfs_list_remove_request(req);
> if (test_bit(NFS_IOHDR_ERROR, &hdr->flags) &&
> (hdr->good_bytes < bytes)) {
> - nfs_set_pageerror(req->wb_page);
> + nfs_set_pageerror(page_file_mapping(req->wb_page));
> nfs_context_set_write_error(req->wb_context, hdr->error);
> goto remove_req;
> }
> @@ -1348,7 +1348,8 @@ int nfs_updatepage(struct file *file, struct
> page *page,
> unsigned int offset, unsigned int count)
> {
> struct nfs_open_context *ctx = nfs_file_open_context(file);
> - struct inode *inode = page_file_mapping(page)->host;
> + struct address_space *mapping = page_file_mapping(page);
> + struct inode *inode = mapping->host;
> int status = 0;
>
> nfs_inc_stats(inode, NFSIOS_VFSUPDATEPAGE);
> @@ -1366,7 +1367,7 @@ int nfs_updatepage(struct file *file, struct
> page *page,
>
> status = nfs_writepage_setup(ctx, page, offset, count);
> if (status < 0)
> - nfs_set_pageerror(page);
> + nfs_set_pageerror(mapping);
> else
> __set_page_dirty_nobuffers(page);
> out:
> --
> 2.14.3

2019-02-12 21:28:32

by Anna Schumaker

[permalink] [raw]
Subject: Re: [PATCH v2] NFS: Don't use page_file_mapping after removing the page

On Tue, 2019-02-12 at 13:52 -0500, Benjamin Coddington wrote:
> Hi Trond and Anna,
>
> I want to bump this. The problem we created is easier to hit now than
> the problem we were fixing, so something should be done for 5.0 if
> possible.

Sounds good. I'll send it in a pull request this week.

Thanks,
Anna

>
> Ben
>
> On 6 Feb 2019, at 6:09, Benjamin Coddington wrote:
>
> > If nfs_page_async_flush() removes the page from the mapping, then we
> > can't
> > use page_file_mapping() on it as nfs_updatepate() is wont to do when
> > receiving an error. Instead, push the mapping to the stack before the
> > page
> > is possibly truncated.
> >
> > Fixes: 8fc75bed96bb ("NFS: Fix up return value on fatal errors in
> > nfs_page_async_flush()")
> > Signed-off-by: Benjamin Coddington <[email protected]>
> > ---
> > fs/nfs/write.c | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> > index f12cb31a41e5..d09c9f878141 100644
> > --- a/fs/nfs/write.c
> > +++ b/fs/nfs/write.c
> > @@ -238,9 +238,9 @@ static void nfs_grow_file(struct page *page,
> > unsigned int offset, unsigned int c
> > }
> >
> > /* A writeback failed: mark the page as bad, and invalidate the page
> > cache */
> > -static void nfs_set_pageerror(struct page *page)
> > +static void nfs_set_pageerror(struct address_space *mapping)
> > {
> > - nfs_zap_mapping(page_file_mapping(page)->host,
> > page_file_mapping(page));
> > + nfs_zap_mapping(mapping->host, mapping);
> > }
> >
> > /*
> > @@ -994,7 +994,7 @@ static void nfs_write_completion(struct
> > nfs_pgio_header *hdr)
> > nfs_list_remove_request(req);
> > if (test_bit(NFS_IOHDR_ERROR, &hdr->flags) &&
> > (hdr->good_bytes < bytes)) {
> > - nfs_set_pageerror(req->wb_page);
> > + nfs_set_pageerror(page_file_mapping(req->wb_page));
> > nfs_context_set_write_error(req->wb_context, hdr-
> > >error);
> > goto remove_req;
> > }
> > @@ -1348,7 +1348,8 @@ int nfs_updatepage(struct file *file, struct
> > page *page,
> > unsigned int offset, unsigned int count)
> > {
> > struct nfs_open_context *ctx = nfs_file_open_context(file);
> > - struct inode *inode = page_file_mapping(page)->host;
> > + struct address_space *mapping = page_file_mapping(page);
> > + struct inode *inode = mapping->host;
> > int status = 0;
> >
> > nfs_inc_stats(inode, NFSIOS_VFSUPDATEPAGE);
> > @@ -1366,7 +1367,7 @@ int nfs_updatepage(struct file *file, struct
> > page *page,
> >
> > status = nfs_writepage_setup(ctx, page, offset, count);
> > if (status < 0)
> > - nfs_set_pageerror(page);
> > + nfs_set_pageerror(mapping);
> > else
> > __set_page_dirty_nobuffers(page);
> > out:
> > --
> > 2.14.3