help converting btrfs to new writeback error tracking?

I've been working on set of patches to clean up how writeback errors are tracked and handled in the kernel:

http://marc.info/?l=linux-fsdevel&m=149304074111261&w=2

The basic idea is that rather than having a set of flags that are cleared whenever they are checked, we have a sequence counter and error that are tracked on a per-mapping basis, and can then use that sequence counter to tell whether the error should be reported.

This changes the way that things like filemap_write_and_wait work. Rather than having to ensure that AS_EIO/AS_ENOSPC are not cleared inappropriately (and thus losing errors that should be reported), you can now tell whether there has been a writeback error since a certain point in time, irrespective of whether anyone else is checking for errors.

I've been doing some conversions of the existing code to the new scheme, but btrfs has _really_ complicated error handling. I think it could probably be simplified with this new scheme, but I could use some help here.

What I think we probably want to do is to sample the error sequence in the mapping at well-defined points in time (probably when starting a transaction?) and then use that to determine whether writeback errors have occurred since then. Is there anyone in the btrfs community who could help me here?

Hi Jeff,

Quoted 22 lines · Jeff Layton

I've been working on set of patches to clean up how writeback errors are tracked and handled in the kernel:

http://marc.info/?l=linux-fsdevel&m=149304074111261&w=2

The basic idea is that rather than having a set of flags that are cleared whenever they are checked, we have a sequence counter and error that are tracked on a per-mapping basis, and can then use that sequence counter to tell whether the error should be reported.

This changes the way that things like filemap_write_and_wait work. Rather than having to ensure that AS_EIO/AS_ENOSPC are not cleared inappropriately (and thus losing errors that should be reported), you can now tell whether there has been a writeback error since a certain point in time, irrespective of whether anyone else is checking for errors.

I've been doing some conversions of the existing code to the new scheme, but btrfs has _really_ complicated error handling. I think it could probably be simplified with this new scheme, but I could use some help here.

Quoted 6 lines

What I think we probably want to do is to sample the error sequence in the mapping at well-defined points in time (probably when starting a transaction?) and then use that to determine whether writeback errors have occurred since then. Is there anyone in the btrfs community who could help me here?

I went through the patch set and reviewed the btrfs part particular in [PATCH v3 14/20] fs: retrofit old error reporting API onto new infrastructure

It looks good to me.

In btrfs ->writepage(), it sets PG_error whenever an error (-EIO/-ENOSPC/-ENOMEM) occurs and it sets mapping's error as well in end_extent_writepage(). And the special case is the compression code, where it only sets mapping's error when there is any error during processing compression bytes.

Similar to ext4, btrfs tracks the IO error by setting mapping's error in writepage_endio and other places (eg. compression code), and around tree-log.c it's checking BTRFS_ORDERED_IOERR from ordered_extent->flags, which is usually set in writepage_endio and sometimes in some error handling code where it couldn't call endio.

So the conversion in btrfs's fsync() seems to be good enough, did I miss anything?

Quoted 43 lines · Liu Bo

Hi Jeff,

Quoted 22 lines · Jeff Layton

I've been working on set of patches to clean up how writeback errors are tracked and handled in the kernel:

http://marc.info/?l=linux-fsdevel&m=149304074111261&w=2

The basic idea is that rather than having a set of flags that are cleared whenever they are checked, we have a sequence counter and error that are tracked on a per-mapping basis, and can then use that sequence counter to tell whether the error should be reported.

This changes the way that things like filemap_write_and_wait work. Rather than having to ensure that AS_EIO/AS_ENOSPC are not cleared inappropriately (and thus losing errors that should be reported), you can now tell whether there has been a writeback error since a certain point in time, irrespective of whether anyone else is checking for errors.

I've been doing some conversions of the existing code to the new scheme, but btrfs has _really_ complicated error handling. I think it could probably be simplified with this new scheme, but I could use some help here.

Quoted 6 lines

What I think we probably want to do is to sample the error sequence in the mapping at well-defined points in time (probably when starting a transaction?) and then use that to determine whether writeback errors have occurred since then. Is there anyone in the btrfs community who could help me here?

I went through the patch set and reviewed the btrfs part particular in [PATCH v3 14/20] fs: retrofit old error reporting API onto new infrastructure

It looks good to me.

In btrfs ->writepage(), it sets PG_error whenever an error (-EIO/-ENOSPC/-ENOMEM) occurs and it sets mapping's error as well in end_extent_writepage(). And the special case is the compression code, where it only sets mapping's error when there is any error during processing compression bytes.

Quoted 9 lines

Similar to ext4, btrfs tracks the IO error by setting mapping's error in writepage_endio and other places (eg. compression code), and around tree-log.c it's checking BTRFS_ORDERED_IOERR from ordered_extent->flags, which is usually set in writepage_endio and sometimes in some error handling code where it couldn't call endio.

So the conversion in btrfs's fsync() seems to be good enough, did I miss anything?

Many thanks for taking a look:

There are a number of calls in btrfs to filemap_fdatawait_range that check the return code. That function will wait for writeback on all of the pages in the mapping range and return an error if there has been one. Note too that there are also some places that ignore the return code.

These patches change how filemap_fdatawait_range (and some similar functions) work. Before this set, you'd get an error if one had occurred since anyone last checked it. Now, you only get an error there if one occurred since you started waiting. If the failed writeback occurred before that function was called, you won't get an error back.

For fsync, it shouldn't matter. You'll get an error back there if one occurred since the last fsync since you're setting it in the mapping. The bigger question is whether other callers in this code do anything with that error return.

If they do, then the next question is: from what point do you want to detect errors that have occurred?

What sort of makes sense to me (in a handwavy way) would be to sample the errseq_t in the mapping when you start a transaction, and then check vs. that for errors. Then, even if you have parallel transactions going on the same inode (is that even possible?) then you can tell whether they all succeded or not.

Thoughts?

Hi Jeff,

Quoted 62 lines · Jeff Layton

Quoted 43 lines · Liu Bo

Hi Jeff,

Quoted 22 lines · Jeff Layton

I've been working on set of patches to clean up how writeback errors are tracked and handled in the kernel:

http://marc.info/?l=linux-fsdevel&m=149304074111261&w=2

The basic idea is that rather than having a set of flags that are cleared whenever they are checked, we have a sequence counter and error that are tracked on a per-mapping basis, and can then use that sequence counter to tell whether the error should be reported.

This changes the way that things like filemap_write_and_wait work. Rather than having to ensure that AS_EIO/AS_ENOSPC are not cleared inappropriately (and thus losing errors that should be reported), you can now tell whether there has been a writeback error since a certain point in time, irrespective of whether anyone else is checking for errors.

I've been doing some conversions of the existing code to the new scheme, but btrfs has _really_ complicated error handling. I think it could probably be simplified with this new scheme, but I could use some help here.

Quoted 6 lines

What I think we probably want to do is to sample the error sequence in the mapping at well-defined points in time (probably when starting a transaction?) and then use that to determine whether writeback errors have occurred since then. Is there anyone in the btrfs community who could help me here?

I went through the patch set and reviewed the btrfs part particular in [PATCH v3 14/20] fs: retrofit old error reporting API onto new infrastructure

It looks good to me.

In btrfs ->writepage(), it sets PG_error whenever an error (-EIO/-ENOSPC/-ENOMEM) occurs and it sets mapping's error as well in end_extent_writepage(). And the special case is the compression code, where it only sets mapping's error when there is any error during processing compression bytes.

Quoted 9 lines

Similar to ext4, btrfs tracks the IO error by setting mapping's error in writepage_endio and other places (eg. compression code), and around tree-log.c it's checking BTRFS_ORDERED_IOERR from ordered_extent->flags, which is usually set in writepage_endio and sometimes in some error handling code where it couldn't call endio.

So the conversion in btrfs's fsync() seems to be good enough, did I miss anything?

Many thanks for taking a look:

There are a number of calls in btrfs to filemap_fdatawait_range that check the return code. That function will wait for writeback on all of the pages in the mapping range and return an error if there has been one. Note too that there are also some places that ignore the return code.

Quoted 6 lines

These patches change how filemap_fdatawait_range (and some similar functions) work. Before this set, you'd get an error if one had occurred since anyone last checked it. Now, you only get an error there if one occurred since you started waiting. If the failed writeback occurred before that function was called, you won't get an error back.

Since all filemap_fdatawait_range() called in btrfs checked the return value, it is supposed to catch any errors that are occured from filemap_fdatawrite_range() which is called twice by btrfs_fdatawrite_range()[1], so with this set, it's possible to fail to detect errors if only calling filemap_fdatawait_range().

[1]: filemap_fdatawrite_range() needs to be called twice to make sure compressed data is flushed.

Quoted 14 lines

For fsync, it shouldn't matter. You'll get an error back there if one occurred since the last fsync since you're setting it in the mapping. The bigger question is whether other callers in this code do anything with that error return.

If they do, then the next question is: from what point do you want to detect errors that have occurred?

What sort of makes sense to me (in a handwavy way) would be to sample the errseq_t in the mapping when you start a transaction, and then check vs. that for errors. Then, even if you have parallel transactions going on the same inode (is that even possible?) then you can tell whether they all succeded or not.

Quoted 1 line

Thoughts?

That may not work, a transaction here is kind of unrelated to mapping and writeback of a mapping can be processed accross two transactions.

Is it possible to provide another version of filemap_fdatawait_range() to take @since as an argument? If so, we can sample wb_err before calling filemap_fdatawrite_range() (or btrfs_fdatawrite_range()) and collect any wb_err after filemap_fdatawait_range().