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

Done in v2, and 'success' it is - thanks for the follow-up, that is what
fs/buffer.c calls it.

        if (!success)
                   mark_buffer_write_io_error(bh);

Patch 21 adds a clear_buffer_write_io_error() to the success side, so the
final shape has both arms again, but nothing touches BH_Uptodate either way.

Best,
Chao

On Tue, Aug 4, 2026 at 5:10 AM Jan Kara <[email protected]> wrote:
>
> 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