Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1595379 > unrolled thread

[PATCH v2 0/9] mm/fs: get PG_error out of the writeback reporting business

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/9] mm/fs: get PG_error out of the writeback reporting business Jeff Layton <jlayton@redhat.com> - 2017-03-08 18:00 +0100
    [PATCH v2 9/9] Documentation: document what to do on a writeback error Jeff Layton <jlayton@redhat.com> - 2017-03-08 18:00 +0100
    [PATCH v2 5/9] dax: set error in mapping when writeback fails Jeff Layton <jlayton@redhat.com> - 2017-03-08 18:00 +0100
      Re: [PATCH v2 5/9] dax: set error in mapping when writeback fails Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-03-10 01:30 +0100
    [PATCH v2 6/9] mm: set mapping error when launder_pages fails Jeff Layton <jlayton@redhat.com> - 2017-03-08 18:00 +0100
      Re: [PATCH v2 6/9] mm: set mapping error when launder_pages fails Trond Myklebust <trondmy@primarydata.com> - 2017-03-08 19:10 +0100
        Re: [PATCH v2 6/9] mm: set mapping error when launder_pages fails Jeff Layton <jlayton@redhat.com> - 2017-03-08 19:50 +0100
          Re: [PATCH v2 6/9] mm: set mapping error when launder_pages fails Trond Myklebust <trondmy@primarydata.com> - 2017-03-08 20:30 +0100
        Re: [PATCH v2 6/9] mm: set mapping error when launder_pages fails NeilBrown <neilb@suse.com> - 2017-03-08 23:50 +0100
    [PATCH v2 4/9] nilfs2: set the mapping error when calling SetPageError on writeback Jeff Layton <jlayton@redhat.com> - 2017-03-08 18:00 +0100

#1595379 — [PATCH v2 0/9] mm/fs: get PG_error out of the writeback reporting business

v2:
- still ClearPageError during __filemap_fdatawait_range
- clear AS_* errors when reporting errors during write initiation
- set mapping errors when launder_page fails
- set mapping errors when writeback fails during migration
- set mapping errors when DAX writeback fails
- Documentation patch to give guidance about writeback errors

Here is v2 of this set. The main difference is some new patches to
ensure that mapping errors get set in a few rather obscure places when
writeback fails, and a patch (based on Jan's suggestion) to clear out
the address space errors when initiating writeback fails with -EIO. I
also left the code clearing PG_error in __filemap_fdatawait_range. We
may want to remove that eventually, but we need to ensure that it gets
cleared in some way when writeback fails.

I've done a bit of testing with this (mostly xfstests on xfs), and it
seems to work ok AFAICT.

Original cover letter follows:

------------------------------8<-----------------------------

I recently did some work to wire up -ENOSPC handling in ceph, and found
I could get back -EIO errors in some cases when I should have instead
gotten -ENOSPC. The problem was that the ceph writeback code would set
PG_error on a writeback error, and that error would clobber the mapping
error.

While I fixed that problem by simply not setting that bit on errors,
that led me down a rabbit hole of looking at how PG_error is being
handled in the kernel.

This patch series is a few fixes for things that I 100% noticed by
inspection. I don't have a great way to test these since they involve
error handling. I can certainly doctor up a kernel to inject errors
in this code and test by hand however if these look plausible up front.

Jeff Layton (9):
  mm: fix mapping_set_error call in me_pagecache_dirty
  mm: drop "wait" parameter from write_one_page
  mm: clear any AS_* errors when returning error on any fsync or close
  nilfs2: set the mapping error when calling SetPageError on writeback
  dax: set error in mapping when writeback fails
  mm: set mapping error when launder_pages fails
  mm: ensure that we set mapping error if writeout() fails
  mm: don't TestClearPageError in __filemap_fdatawait_range
  Documentation: document what to do on a writeback error

 Documentation/filesystems/vfs.txt |  7 +++++++
 fs/dax.c                          |  4 +++-
 fs/exofs/dir.c                    |  2 +-
 fs/ext2/dir.c                     |  2 +-
 fs/jfs/jfs_metapage.c             |  4 ++--
 fs/minix/dir.c                    |  2 +-
 fs/nilfs2/segment.c               |  1 +
 fs/sysv/dir.c                     |  2 +-
 fs/ufs/dir.c                      |  2 +-
 include/linux/mm.h                |  2 +-
 mm/filemap.c                      | 40 ++++++++++++++++++++++-----------------
 mm/memory-failure.c               |  2 +-
 mm/migrate.c                      |  6 +++++-
 mm/page-writeback.c               | 14 +++++++-------
 mm/truncate.c                     |  6 +++++-
 15 files changed, 60 insertions(+), 36 deletions(-)

-- 
2.9.3

[toc] | [next] | [standalone]

#1595382 — [PATCH v2 9/9] Documentation: document what to do on a writeback error

There's no real guidance on this for filesystem authors, so add a
paragraph to vfs.txt that explains how this should be handled.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 Documentation/filesystems/vfs.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index 569211703721..527370fbab39 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -577,6 +577,13 @@ should clear PG_Dirty and set PG_Writeback.  It can be actually
 written at any point after PG_Dirty is clear.  Once it is known to be
 safe, PG_Writeback is cleared.
 
+If there is an error during writeback, then the address_space should be
+marked with an AS_EIO or AS_ENOSPC error, in order to ensure that the
+error will be reported to the application at fsync or close.  Most
+writepage callers will do this automatically if writepage returns an
+error, but writepages implementations generally need to ensure this
+themselves.
+
 Writeback makes use of a writeback_control structure...
 
 struct address_space_operations
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1595384 — [PATCH v2 5/9] dax: set error in mapping when writeback fails

In order to get proper error codes from fsync, we must set an error in
the mapping range when writeback fails.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/dax.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/dax.c b/fs/dax.c
index de622d4282a6..a601137286ed 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -892,8 +892,10 @@ int dax_writeback_mapping_range(struct address_space *mapping,
 
 			ret = dax_writeback_one(bdev, mapping, indices[i],
 					pvec.pages[i]);
-			if (ret < 0)
+			if (ret < 0) {
+				mapping_set_error(mapping, ret);
 				return ret;
+			}
 		}
 	}
 	return 0;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1596510 — Re: [PATCH v2 5/9] dax: set error in mapping when writeback fails

On Wed, Mar 08, 2017 at 11:29:30AM -0500, Jeff Layton wrote:
> In order to get proper error codes from fsync, we must set an error in
> the mapping range when writeback fails.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Yep, paired with the changes to filmap_write_and_wait() and
filemap_write_and_wait_range(), this seems fine.

Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>

[toc] | [prev] | [next] | [standalone]

#1595387 — [PATCH v2 6/9] mm: set mapping error when launder_pages fails

If launder_page fails, then we hit a problem writing back some inode
data. Ensure that we communicate that fact in a subsequent fsync since
another task could still have it open for write.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 mm/truncate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/truncate.c b/mm/truncate.c
index 6263affdef88..29ae420a5bf9 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -594,11 +594,15 @@ invalidate_complete_page2(struct address_space *mapping, struct page *page)
 
 static int do_launder_page(struct address_space *mapping, struct page *page)
 {
+	int ret;
+
 	if (!PageDirty(page))
 		return 0;
 	if (page->mapping != mapping || mapping->a_ops->launder_page == NULL)
 		return 0;
-	return mapping->a_ops->launder_page(page);
+	ret = mapping->a_ops->launder_page(page);
+	mapping_set_error(mapping, ret);
+	return ret;
 }
 
 /**
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1595421 — Re: [PATCH v2 6/9] mm: set mapping error when launder_pages fails

On Wed, 2017-03-08 at 11:29 -0500, Jeff Layton wrote:
> If launder_page fails, then we hit a problem writing back some inode
> data. Ensure that we communicate that fact in a subsequent fsync
> since
> another task could still have it open for write.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  mm/truncate.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/truncate.c b/mm/truncate.c
> index 6263affdef88..29ae420a5bf9 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -594,11 +594,15 @@ invalidate_complete_page2(struct address_space
> *mapping, struct page *page)
>  
>  static int do_launder_page(struct address_space *mapping, struct
> page *page)
>  {
> +	int ret;
> +
>  	if (!PageDirty(page))
>  		return 0;
>  	if (page->mapping != mapping || mapping->a_ops->launder_page 
> == NULL)
>  		return 0;
> -	return mapping->a_ops->launder_page(page);
> +	ret = mapping->a_ops->launder_page(page);
> +	mapping_set_error(mapping, ret);
> +	return ret;
>  }
>  
>  /**

No. At that layer, you don't know that this is a page error. In the NFS
case, it could, for instance, just as well be a fatal signal.

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

[toc] | [prev] | [next] | [standalone]

#1595443 — Re: [PATCH v2 6/9] mm: set mapping error when launder_pages fails

On Wed, 2017-03-08 at 18:01 +0000, Trond Myklebust wrote:
> On Wed, 2017-03-08 at 11:29 -0500, Jeff Layton wrote:
> > If launder_page fails, then we hit a problem writing back some inode
> > data. Ensure that we communicate that fact in a subsequent fsync
> > since
> > another task could still have it open for write.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  mm/truncate.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/truncate.c b/mm/truncate.c
> > index 6263affdef88..29ae420a5bf9 100644
> > --- a/mm/truncate.c
> > +++ b/mm/truncate.c
> > @@ -594,11 +594,15 @@ invalidate_complete_page2(struct address_space
> > *mapping, struct page *page)
> >  
> >  static int do_launder_page(struct address_space *mapping, struct
> > page *page)
> >  {
> > +	int ret;
> > +
> >  	if (!PageDirty(page))
> >  		return 0;
> >  	if (page->mapping != mapping || mapping->a_ops->launder_page 
> > == NULL)
> >  		return 0;
> > -	return mapping->a_ops->launder_page(page);
> > +	ret = mapping->a_ops->launder_page(page);
> > +	mapping_set_error(mapping, ret);
> > +	return ret;
> >  }
> >  
> >  /**
> 
> No. At that layer, you don't know that this is a page error. In the NFS
> case, it could, for instance, just as well be a fatal signal.
> 

Ok...don't we have the same problem with writepage then? Most of the
writepage callers will set an error in the mapping if writepage returns
any sort of error? A fatal signal in that codepath could cause the same
problem, it seems. We don't dip into direct reclaim so much anymore, so
maybe signals aren't an issue there?

The alternative here would be to push this down into the callers. I
worry a bit though about getting this right across filesystems though.
It'd be preferable it if we could keep the mapping_set_error call in
generic VFS code instead, but if not then I'll just plan to do that.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1595464 — Re: [PATCH v2 6/9] mm: set mapping error when launder_pages fails

On Wed, 2017-03-08 at 13:38 -0500, Jeff Layton wrote:
> On Wed, 2017-03-08 at 18:01 +0000, Trond Myklebust wrote:
> > On Wed, 2017-03-08 at 11:29 -0500, Jeff Layton wrote:
> > > If launder_page fails, then we hit a problem writing back some
> > > inode
> > > data. Ensure that we communicate that fact in a subsequent fsync
> > > since
> > > another task could still have it open for write.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  mm/truncate.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/truncate.c b/mm/truncate.c
> > > index 6263affdef88..29ae420a5bf9 100644
> > > --- a/mm/truncate.c
> > > +++ b/mm/truncate.c
> > > @@ -594,11 +594,15 @@ invalidate_complete_page2(struct
> > > address_space
> > > *mapping, struct page *page)
> > >  
> > >  static int do_launder_page(struct address_space *mapping, struct
> > > page *page)
> > >  {
> > > +	int ret;
> > > +
> > >  	if (!PageDirty(page))
> > >  		return 0;
> > >  	if (page->mapping != mapping || mapping->a_ops-
> > > >launder_page 
> > > == NULL)
> > >  		return 0;
> > > -	return mapping->a_ops->launder_page(page);
> > > +	ret = mapping->a_ops->launder_page(page);
> > > +	mapping_set_error(mapping, ret);
> > > +	return ret;
> > >  }
> > >  
> > >  /**
> > 
> > No. At that layer, you don't know that this is a page error. In the
> > NFS
> > case, it could, for instance, just as well be a fatal signal.
> > 
> 
> Ok...don't we have the same problem with writepage then? Most of the
> writepage callers will set an error in the mapping if writepage
> returns
> any sort of error? A fatal signal in that codepath could cause the
> same
> problem, it seems. We don't dip into direct reclaim so much anymore,
> so
> maybe signals aren't an issue there?

If writepage() fails due to a signal, then it has the option of marking
the page as dirty and returning AOP_WRITEPAGE_ACTIVATE. That's not
possible for launder_page().

> The alternative here would be to push this down into the callers. I
> worry a bit though about getting this right across filesystems
> though.
> It'd be preferable it if we could keep the mapping_set_error call in
> generic VFS code instead, but if not then I'll just plan to do that.
> 



-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

[toc] | [prev] | [next] | [standalone]

#1595569 — Re: [PATCH v2 6/9] mm: set mapping error when launder_pages fails

[Multipart message — attachments visible in raw view] — view raw

On Thu, Mar 09 2017, Trond Myklebust wrote:

> On Wed, 2017-03-08 at 11:29 -0500, Jeff Layton wrote:
>> If launder_page fails, then we hit a problem writing back some inode
>> data. Ensure that we communicate that fact in a subsequent fsync
>> since
>> another task could still have it open for write.
>> 
>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>> ---
>>  mm/truncate.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/mm/truncate.c b/mm/truncate.c
>> index 6263affdef88..29ae420a5bf9 100644
>> --- a/mm/truncate.c
>> +++ b/mm/truncate.c
>> @@ -594,11 +594,15 @@ invalidate_complete_page2(struct address_space
>> *mapping, struct page *page)
>>  
>>  static int do_launder_page(struct address_space *mapping, struct
>> page *page)
>>  {
>> +	int ret;
>> +
>>  	if (!PageDirty(page))
>>  		return 0;
>>  	if (page->mapping != mapping || mapping->a_ops->launder_page 
>> == NULL)
>>  		return 0;
>> -	return mapping->a_ops->launder_page(page);
>> +	ret = mapping->a_ops->launder_page(page);
>> +	mapping_set_error(mapping, ret);
>> +	return ret;
>>  }
>>  
>>  /**
>
> No. At that layer, you don't know that this is a page error. In the NFS
> case, it could, for instance, just as well be a fatal signal.
>

In that case, would 'ret' be ERESTARTSYS or EAGAIN or similar?
Should mapping_set_error() ignore those?

Thanks,
NeilBrown

> -- 
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> trond.myklebust@primarydata.com

[toc] | [prev] | [next] | [standalone]

#1595389 — [PATCH v2 4/9] nilfs2: set the mapping error when calling SetPageError on writeback

In a later patch, we're going to want to make the fsync codepath not do
a TestClearPageError call as that can override the error set in the
address space. To do that though, we need to ensure that filesystems
that are relying on the PG_error bit for reporting writeback errors
also set an error in the address space.

Ensure that this is set in nilfs2.

Cc: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nilfs2/segment.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index febed1217b3f..612d4b446793 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -1745,6 +1745,7 @@ static void nilfs_end_page_io(struct page *page, int err)
 	} else {
 		__set_page_dirty_nobuffers(page);
 		SetPageError(page);
+		mapping_set_error(page_mapping(page), err);
 	}
 
 	end_page_writeback(page);
-- 
2.9.3

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web