Re: [PATCH 01/14] writeback: don't call mapping_set_error in writepage_cb

[Brian Foster]

On Tue, Feb 13, 2024 at 02:07:13PM +0100, Jan Kara wrote:
> On Mon 12-02-24 08:13:35, Christoph Hellwig wrote:
> > writepage_cb is the iterator callback for write_cache_pages, which
> > already tracks all errors and returns them to the caller.  There is
> > no need to additionally cal mapping_set_error which is intended
>                           ^^^ call
> 
> > for contexts where the error can't be directly returned (e.g. the
> > I/O completion handlers).
> > 
> > Remove the mapping_set_error call in writepage_cb which is not only
> > superfluous but also buggy as it can be called with the error argument
> > set to AOP_WRITEPAGE_ACTIVATE, which is not actually an error but a
> > magic return value asking the caller to unlock the page.
> > 
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> Our error handling in writeback has always been ... spotty. E.g.
> block_write_full_page() and iomap_writepage_map() call mapping_set_error()
> as well so this seems to be a common way to do things, OTOH ext4 calls
> mapping_set_error() only on IO completion. I guess the question is how
> an error in ->writepages from background writeback should propagate to
> eventual fsync(2) caller? Because currently such error propagates all the
> way up to writeback_sb_inodes() where it is silently dropped...
> 

A couple related notes from skimming around:

- Things like iomap might make this call in I/O completion paths, but
  then invoke bio completion paths on submission side errors (i.e.
  iomap_submit_ioend() -> bio_endio()).
- __writeback_single_inode() calls filemap_fdatawait() shortly after
  do_writepages(), which basically looks like it relies on mapping error
  state to propagate error within the writeback path.

The call removed by this path only seems to apply to contexts that don't
define their own .writepages, so it's not clear to me how much this
really matters. It just seems like it's a little hard to quantify
whether this is an undesireable change in behavior or not.

Brian

> 								Honza
> 
> > ---
> >  mm/page-writeback.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index 3f255534986a2f..62901fa905f01e 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -2534,9 +2534,8 @@ static int writepage_cb(struct folio *folio, struct writeback_control *wbc,
> >  		void *data)
> >  {
> >  	struct address_space *mapping = data;
> > -	int ret = mapping->a_ops->writepage(&folio->page, wbc);
> > -	mapping_set_error(mapping, ret);
> > -	return ret;
> > +
> > +	return mapping->a_ops->writepage(&folio->page, wbc);
> >  }
> >  
> >  int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
> > -- 
> > 2.39.2
> > 
> -- 
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
>