[v3,10/20] fuse: set mapping error in writepage_locked when it fails
diff mbox

Message ID 20170424132259.8680-11-jlayton@redhat.com
State New
Headers show

Commit Message

Jeff Layton April 24, 2017, 1:22 p.m. UTC
This ensures that we see errors on fsync when writeback fails.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/fuse/file.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Christoph Hellwig April 24, 2017, 3:25 p.m. UTC | #1
Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Jan Kara April 24, 2017, 4:04 p.m. UTC | #2
On Mon 24-04-17 09:22:49, Jeff Layton wrote:
> This ensures that we see errors on fsync when writeback fails.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Hum, but do we really want to clobber mapping errors with temporary stuff
like ENOMEM? Or do you want to handle that in mapping_set_error?

								Honza

> ---
>  fs/fuse/file.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index ec238fb5a584..07d0efcb050c 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1669,6 +1669,7 @@ static int fuse_writepage_locked(struct page *page)
>  err_free:
>  	fuse_request_free(req);
>  err:
> +	mapping_set_error(page->mapping, error);
>  	end_page_writeback(page);
>  	return error;
>  }
> -- 
> 2.9.3
> 
>
Jeff Layton April 24, 2017, 5:14 p.m. UTC | #3
On Mon, 2017-04-24 at 18:04 +0200, Jan Kara wrote:
> On Mon 24-04-17 09:22:49, Jeff Layton wrote:
> > This ensures that we see errors on fsync when writeback fails.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> 
> Hum, but do we really want to clobber mapping errors with temporary stuff
> like ENOMEM? Or do you want to handle that in mapping_set_error?
> 

Right now we don't really have such a thing as temporary errors in the
writeback codepath. If you return an error here, the data doesn't stay
dirty or anything, and I think we want to ensure that that gets reported
via fsync.

I'd like to see us add better handling for retryable errors for stuff
like ENOMEM or EAGAIN. I think this is the first step toward that
though. Once we have more consistent handling of writeback errors in
general, then we can start doing more interesting things with retryable
errors.

So yeah, I this is the right thing to do for now.

> 
> > ---
> >  fs/fuse/file.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index ec238fb5a584..07d0efcb050c 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -1669,6 +1669,7 @@ static int fuse_writepage_locked(struct page *page)
> >  err_free:
> >  	fuse_request_free(req);
> >  err:
> > +	mapping_set_error(page->mapping, error);
> >  	end_page_writeback(page);
> >  	return error;
> >  }
> > -- 
> > 2.9.3
> > 
> >
Jan Kara April 25, 2017, 8:17 a.m. UTC | #4
On Mon 24-04-17 13:14:36, Jeff Layton wrote:
> On Mon, 2017-04-24 at 18:04 +0200, Jan Kara wrote:
> > On Mon 24-04-17 09:22:49, Jeff Layton wrote:
> > > This ensures that we see errors on fsync when writeback fails.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > 
> > Hum, but do we really want to clobber mapping errors with temporary stuff
> > like ENOMEM? Or do you want to handle that in mapping_set_error?
> > 
> 
> Right now we don't really have such a thing as temporary errors in the
> writeback codepath. If you return an error here, the data doesn't stay
> dirty or anything, and I think we want to ensure that that gets reported
> via fsync.
> 
> I'd like to see us add better handling for retryable errors for stuff
> like ENOMEM or EAGAIN. I think this is the first step toward that
> though. Once we have more consistent handling of writeback errors in
> general, then we can start doing more interesting things with retryable
> errors.
> 
> So yeah, I this is the right thing to do for now.

OK, fair enough. And question number 2):

Who is actually responsible for setting the error in the mapping when error
happens inside ->writepage()? Is it the ->writepage() callback or the
caller of ->writepage()? Or something else? Currently it seems to be a
strange mix (e.g. mm/page-writeback.c: __writepage() calls
mapping_set_error() when ->writepage() returns error) so I'd like to
understand what's the plan and have that recorded in the changelogs.

								Honza

> 
> > 
> > > ---
> > >  fs/fuse/file.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > > index ec238fb5a584..07d0efcb050c 100644
> > > --- a/fs/fuse/file.c
> > > +++ b/fs/fuse/file.c
> > > @@ -1669,6 +1669,7 @@ static int fuse_writepage_locked(struct page *page)
> > >  err_free:
> > >  	fuse_request_free(req);
> > >  err:
> > > +	mapping_set_error(page->mapping, error);
> > >  	end_page_writeback(page);
> > >  	return error;
> > >  }
> > > -- 
> > > 2.9.3
> > > 
> > > 
> 
> -- 
> Jeff Layton <jlayton@redhat.com>
Jeff Layton April 25, 2017, 10:35 a.m. UTC | #5
On Tue, 2017-04-25 at 10:17 +0200, Jan Kara wrote:
> On Mon 24-04-17 13:14:36, Jeff Layton wrote:
> > On Mon, 2017-04-24 at 18:04 +0200, Jan Kara wrote:
> > > On Mon 24-04-17 09:22:49, Jeff Layton wrote:
> > > > This ensures that we see errors on fsync when writeback fails.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > 
> > > Hum, but do we really want to clobber mapping errors with temporary stuff
> > > like ENOMEM? Or do you want to handle that in mapping_set_error?
> > > 
> > 
> > Right now we don't really have such a thing as temporary errors in the
> > writeback codepath. If you return an error here, the data doesn't stay
> > dirty or anything, and I think we want to ensure that that gets reported
> > via fsync.
> > 
> > I'd like to see us add better handling for retryable errors for stuff
> > like ENOMEM or EAGAIN. I think this is the first step toward that
> > though. Once we have more consistent handling of writeback errors in
> > general, then we can start doing more interesting things with retryable
> > errors.
> > 
> > So yeah, I this is the right thing to do for now.
> 
> OK, fair enough. And question number 2):
> 
> Who is actually responsible for setting the error in the mapping when error
> happens inside ->writepage()? Is it the ->writepage() callback or the
> caller of ->writepage()? Or something else? Currently it seems to be a
> strange mix (e.g. mm/page-writeback.c: __writepage() calls
> mapping_set_error() when ->writepage() returns error) so I'd like to
> understand what's the plan and have that recorded in the changelogs.
> 

That's an excellent question.

I think we probably want the writepage/launder_page operations to call
mapping_set_error. That makes it possible for filesystems (e.g. NFS) to
handle their own error tracking and reporting without using the new
infrastructure. If they never call mapping_set_error then we'll always
just return whatever their ->fsync operation returns on an fsync.

I'll make another pass through the tree and see whether we have some
mapping_set_error calls that should be removed, and will flesh out
vfs.txt to state this. Maybe that file needs a whole section on
writeback error reporting? Hmmm...

That probably also means that I should drop patch 8 from this series
(mm: ensure that we set mapping error if writeout fails), since that
should be happening in writepage already.

> > 
> > > 
> > > > ---
> > > >  fs/fuse/file.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > > > index ec238fb5a584..07d0efcb050c 100644
> > > > --- a/fs/fuse/file.c
> > > > +++ b/fs/fuse/file.c
> > > > @@ -1669,6 +1669,7 @@ static int fuse_writepage_locked(struct page *page)
> > > >  err_free:
> > > >  	fuse_request_free(req);
> > > >  err:
> > > > +	mapping_set_error(page->mapping, error);
> > > >  	end_page_writeback(page);
> > > >  	return error;
> > > >  }
> > > > -- 
> > > > 2.9.3
> > > > 
> > > > 
> > 
> > -- 
> > Jeff Layton <jlayton@redhat.com>
Jan Kara April 25, 2017, 11:19 a.m. UTC | #6
On Tue 25-04-17 06:35:13, Jeff Layton wrote:
> On Tue, 2017-04-25 at 10:17 +0200, Jan Kara wrote:
> > On Mon 24-04-17 13:14:36, Jeff Layton wrote:
> > > On Mon, 2017-04-24 at 18:04 +0200, Jan Kara wrote:
> > > > On Mon 24-04-17 09:22:49, Jeff Layton wrote:
> > > > > This ensures that we see errors on fsync when writeback fails.
> > > > > 
> > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > 
> > > > Hum, but do we really want to clobber mapping errors with temporary stuff
> > > > like ENOMEM? Or do you want to handle that in mapping_set_error?
> > > > 
> > > 
> > > Right now we don't really have such a thing as temporary errors in the
> > > writeback codepath. If you return an error here, the data doesn't stay
> > > dirty or anything, and I think we want to ensure that that gets reported
> > > via fsync.
> > > 
> > > I'd like to see us add better handling for retryable errors for stuff
> > > like ENOMEM or EAGAIN. I think this is the first step toward that
> > > though. Once we have more consistent handling of writeback errors in
> > > general, then we can start doing more interesting things with retryable
> > > errors.
> > > 
> > > So yeah, I this is the right thing to do for now.
> > 
> > OK, fair enough. And question number 2):
> > 
> > Who is actually responsible for setting the error in the mapping when error
> > happens inside ->writepage()? Is it the ->writepage() callback or the
> > caller of ->writepage()? Or something else? Currently it seems to be a
> > strange mix (e.g. mm/page-writeback.c: __writepage() calls
> > mapping_set_error() when ->writepage() returns error) so I'd like to
> > understand what's the plan and have that recorded in the changelogs.
> > 
> 
> That's an excellent question.
> 
> I think we probably want the writepage/launder_page operations to call
> mapping_set_error. That makes it possible for filesystems (e.g. NFS) to
> handle their own error tracking and reporting without using the new
> infrastructure. If they never call mapping_set_error then we'll always
> just return whatever their ->fsync operation returns on an fsync.

OK, makes sense. It is also in line with what you did for DAX, 9p, or here
for FUSE. So feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

for this patch but please also add a sentense that ->writepage() is
responsible for calling mapping_set_error() if it fails and page is not
redirtied to the changelogs of patches changing writepage handlers.

> I'll make another pass through the tree and see whether we have some
> mapping_set_error calls that should be removed, and will flesh out
> vfs.txt to state this. Maybe that file needs a whole section on
> writeback error reporting? Hmmm...

I think it would be nice to have all the logic described in one place. So
+1 from me.

> That probably also means that I should drop patch 8 from this series
> (mm: ensure that we set mapping error if writeout fails), since that
> should be happening in writepage already.

Yes.

								Honza

Patch
diff mbox

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index ec238fb5a584..07d0efcb050c 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1669,6 +1669,7 @@  static int fuse_writepage_locked(struct page *page)
 err_free:
 	fuse_request_free(req);
 err:
+	mapping_set_error(page->mapping, error);
 	end_page_writeback(page);
 	return error;
 }