Re: [PATCH 16/19] jbd2: report journal write errors with BH_Write_EIO

On Tue 04-08-26 10:54:37, Jan Kara wrote:
> On Sat 01-08-26 18:01:00, Chao Shi wrote:
> > The journal's own write completion handler,
> > journal_end_buffer_io_sync(), reports a failed write by clearing
> > BH_Uptodate, and the three places that wait for journal writes look for
> > that.  This series is removing that convention: a buffer whose write failed
> > still holds the data that was supposed to reach the disk, and saying it is
> > not up to date makes callers rewrite, re-read or WARN over data that was
> > never wrong.
> > 
> > Set BH_Write_EIO instead, with mark_buffer_write_io_error(), and test it in
> > journal_wait_on_commit_record() and in the two commit-phase waits.
> > 
> > The two changes have to go together, because commit phase 4 waits on a
> > mixed list: descriptor blocks are submitted with
> > journal_end_buffer_io_sync(), while revoke blocks go through
> > write_dirty_buffer() and land in bh_end_write().  bh_end_write() already
> > sets BH_Write_EIO, so converting the consumer alone would keep working for
> > revoke blocks and silently stop detecting failed descriptor writes.  With
> > the handler converted, both halves of the list report the same way.
> > 
> > mark_buffer_write_io_error() is safe on all of these buffers.  The shadow
> > buffers from jbd2_journal_write_metadata_buffer() have no folio and no
> > associated mapping, so it does nothing beyond setting the flag.  Descriptor
> > and commit blocks are ordinary buffers on the journal device, and marking
> > the journal's mapping with the error is what write_dirty_buffer() already
> > does for revoke blocks on the same device.
> > 
> > Signed-off-by: Chao Shi <[email protected]>
> > ---
> >  fs/jbd2/commit.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> > index 60273cddf434..8912786ebebb 100644
> > --- a/fs/jbd2/commit.c
> > +++ b/fs/jbd2/commit.c
> > @@ -39,7 +39,7 @@ static void journal_end_buffer_io_sync(struct bio *bio)
> >  	if (uptodate)
> >  		set_buffer_uptodate(bh);
> >  	else
> > -		clear_buffer_uptodate(bh);
> > +		mark_buffer_write_io_error(bh);
> 
> There's no need to set uptodate flag here at all (all callers set it before
> submitting IO). So you can just simplify this to:
> 
> 	if (!uptodate)
> 		mark_buffer_write_io_error(bh);
> 
> Also I'd rename the local variable from 'uptodate' to 'ioerr' or something
> like that.

Sorry, not 'ioerr' but rather 'success'. I can see fs/buffer.c names it like
that.

								Honza
-- 
Jan Kara <[email protected]>
SUSE Labs, CR