| Message ID | 20170424132259.8680-11-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
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 > >
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 > > > >
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>
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>
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
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; }
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(+)