Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails
On Tue, 2017-04-25 at 13:19 +0200, Jan Kara wrote: > 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> > > > > > > > > > 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 > > 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 (cc'ing Jon since I'm proposing a doc update) Here's what I'm thinking for a vfs.txt update after this series. The section on writeback_control could probably be more specific. --8<- diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt index 94dd27ef4a76..aa912b65792a 100644 --- a/Documentation/filesystems/vfs.txt +++ b/Documentation/filesystems/vfs.txt @@ -576,7 +576,23 @@ 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. -Writeback makes use of a writeback_control structure... +Writeback makes use of a writeback_control structure to direct the +operations. This tells the writepage and writepages operations something +about the nature of and reason for the writeback request, and the +constraints under which it is being done. It is also used to track state +between successive writeback requests. + +When there is an error during writeback, then an error should be +reported to fsync on all file descriptors that were open at the time of +the error. This is typically done by setting the wb_err value in the +address_space via mapping_set_error when writeback errors occur. The +vfs-layer fsync code will then report the errors on a per-fd basis. + +Filesystems are free to track errors internally if they choose, but they +should aim to provide the same semantics for error reporting when there +are multiple writers. Filesystems that track their own errors should +avoid calling mapping_set_error in order to ensure that errors stored in +the mapping aren't improperly reported by the generic filesystem code. struct
Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails
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> > > > > > > > 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 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 -- Jan Kara SUSE Labs, CR
Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails
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> > > > > > 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 -- Jeff Layton
Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails
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> > > > 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 -- Jan Kara SUSE Labs, CR
Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails
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> > 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 > > > > -- Jeff Layton
Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails
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 LaytonHum, 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 > > -- Jan Kara SUSE Labs, CR
Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails
Looks fine, Reviewed-by: Christoph Hellwig
[PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails
This ensures that we see errors on fsync when writeback fails. Signed-off-by: Jeff Layton--- 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