question about mapping_set_error when writeback fails?

question about mapping_set_error when writeback fails?

[Darrick J. Wong]

In iomap_finish_page_writeback,

static void
iomap_finish_page_writeback(struct inode *inode, struct page *page,
		int error, unsigned int len)
{
	struct iomap_page *iop = to_iomap_page(page);

	if (error) {
		SetPageError(page);
		mapping_set_error(inode->i_mapping, -EIO);

Why don't we pass error to mapping_set_error here?  If the writeback
completion failed due to insufficient space (e.g. extent mapping btree
expansion hit ENOSPC while trying to perform an unwritten extent
conversion) then we set AS_EIO which causes fsync to return EIO instead
of ENOSPC like you'd expect.

The line in question was lifted from XFS; is this a historical behavior
from before we had AS_ENOSPC?  Or do we always set AS_EIO because that's
the error code that everyone understands (ha) to mean that writeback
failed and now we have no idea what's on disk vs. in the pagecache?

(I tried to figure out what ext4 and btrfs do to handle this, but ...
that was a twisty code maze and I gave up.)

--D

Re: question about mapping_set_error when writeback fails?

[Matthew Wilcox]

On Wed, Jun 02, 2021 at 01:27:56PM -0700, Darrick J. Wong wrote:
> In iomap_finish_page_writeback,
> 
> static void
> iomap_finish_page_writeback(struct inode *inode, struct page *page,
> 		int error, unsigned int len)
> {
> 	struct iomap_page *iop = to_iomap_page(page);
> 
> 	if (error) {
> 		SetPageError(page);
> 		mapping_set_error(inode->i_mapping, -EIO);
> 
> Why don't we pass error to mapping_set_error here?  If the writeback
> completion failed due to insufficient space (e.g. extent mapping btree
> expansion hit ENOSPC while trying to perform an unwritten extent
> conversion) then we set AS_EIO which causes fsync to return EIO instead
> of ENOSPC like you'd expect.

Hah, I noticed the same thing a few weeks ago and didn't get round to
asking about it yet.  I'm pretty sure we should pass the real error to
mapping_set_error().

I also wonder if we shouldn't support more of the errors from
blk_errors, like -ETIMEDOUT or -EILSEQ, but that's a different
conversation.

Re: question about mapping_set_error when writeback fails?

[Jeff Layton]

On Wed, 2021-06-02 at 23:26 +0100, Matthew Wilcox wrote:
> On Wed, Jun 02, 2021 at 01:27:56PM -0700, Darrick J. Wong wrote:
> > In iomap_finish_page_writeback,
> > 
> > static void
> > iomap_finish_page_writeback(struct inode *inode, struct page *page,
> > 		int error, unsigned int len)
> > {
> > 	struct iomap_page *iop = to_iomap_page(page);
> > 
> > 	if (error) {
> > 		SetPageError(page);
> > 		mapping_set_error(inode->i_mapping, -EIO);
> > 
> > Why don't we pass error to mapping_set_error here?  If the writeback
> > completion failed due to insufficient space (e.g. extent mapping btree
> > expansion hit ENOSPC while trying to perform an unwritten extent
> > conversion) then we set AS_EIO which causes fsync to return EIO instead
> > of ENOSPC like you'd expect.
> 
> Hah, I noticed the same thing a few weeks ago and didn't get round to
> asking about it yet.  I'm pretty sure we should pass the real error to
> mapping_set_error().
> 
> I also wonder if we shouldn't support more of the errors from
> blk_errors, like -ETIMEDOUT or -EILSEQ, but that's a different
> conversation.

Note that whatever error you pass there is likely to bubble up to
userland via fsync/msync or whatever. Most file_check_and_advance_wb_err
callers don't vet that return code in any way. That's not a problem,
per-se, but you should be aware of the potential effects.
-- 
Jeff Layton <jlayton@kernel.org>