Re: question about mapping_set_error when writeback fails?

[Jeff Layton <jlayton@kernel.org>]

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>