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@xxxxxxxxxx>
