Re: [Lsf-pc] [LSF/MM TOPIC] do we really need PG_error at all?

[NeilBrown]

On Mon, Feb 27 2017, Jeff Layton wrote:

> On Tue, 2017-02-28 at 10:32 +1100, NeilBrown wrote:
>> On Mon, Feb 27 2017, Andreas Dilger wrote:
>> 
>> > 
>> > My thought is that PG_error is definitely useful for applications to get
>> > correct errors back when doing write()/sync_file_range() so that they know
>> > there is an error in the data that _they_ wrote, rather than receiving an
>> > error for data that may have been written by another thread, and in turn
>> > clearing the error from another thread so it *doesn't* know it had a write
>> > error.
>> 
>> It might be useful in that way, but it is not currently used that way.
>> Such usage would be a change in visible behaviour.
>> 
>> sync_file_range() calls filemap_fdatawait_range(), which calls
>> filemap_check_errors().
>> If there have been any errors in the file recently, inside or outside
>> the range, the latter will return an error which will propagate up.
>> 
>> > 
>> > As for stray sync() clearing PG_error from underneath an application, that
>> > shouldn't happen since filemap_fdatawait_keep_errors() doesn't clear errors
>> > and is used by device flushing code (fdatawait_one_bdev(), wait_sb_inodes()).
>> 
>> filemap_fdatawait_keep_errors() calls __filemap_fdatawait_range() which
>> clears PG_error on every page.
>> What it doesn't do is call filemap_check_errors(), and so doesn't clear
>> AS_ENOSPC or AS_EIO.
>> 
>> 
>
> I think it's helpful to get a clear idea of what happens now in the face
> of errors and what we expect to happen, and I don't quite have that yet:
>
> --------------------------8<-----------------------------
> void page_endio(struct page *page, bool is_write, int err)
> {
> ÂÂÂÂÂÂÂÂif (!is_write) {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!err) {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂSetPageUptodate(page);
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ} else {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂClearPageUptodate(page);
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂSetPageError(page);
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunlock_page(page);
> ÂÂÂÂÂÂÂÂ} else {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (err) {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂSetPageError(page);
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (page->mapping)
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmapping_set_error(page->mapping, err);
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂend_page_writeback(page);
> ÂÂÂÂÂÂÂÂ}
> }
> --------------------------8<-----------------------------
>
> ...not everything uses page_endio, but it's a good place to look since
> we have both flavors of error handling in one place.
>
> On a write error, we SetPageError and set the error in the mapping.
>
> What I'm not clear on is:
>
> 1) what happens to the page at that point when we get a writeback error?
> Does it just remain in-core and is allowed to service reads (assuming
> that it was uptodate before)?

Yes, it remains in core and can service reads.  It is no different from
a page on which a write recent succeeded, except that the write didn't
succeed so the contents of backing store might be different from the
contents of the page.

>
> Can I redirty it and have it retry the write? Is there standard behavior
> for this or is it just up to the whim of the filesystem?

Everything is at the whim of the filesystem, but I doubt if many differ
from the above.

NeilBrown

>
> I'll probably have questions about the read side as well, but for now it
> looks like it's mostly used in an ad-hoc way to communicate errors
> across subsystems (block to fs layer, for instance).
> --
> Jeff Layton <jlayton@xxxxxxxxxx>

Attachment: signature.asc
Description: PGP signature