Re: [PATCH 19/19] buffer: stop clearing BH_Uptodate when a write fails

[Chris S]

Hi Jan, Thanks for your suggestion, changes on v2.

Write completion no longer touches BH_Uptodate in either direction, here
and in the jbd2 and ext4 fast commit handlers.  I instrumented all four
with WARN_ONCE(success && !buffer_uptodate(bh)) and exercised ext4
(data=journal, fast_commit, -o sync), vfat and exfat - creates, rewrites,
hardlinks, renames, deletes.  It never fired.

The BH_Write_EIO change is a separate patch at the end, since it has to
move all five handlers and __bh_submit() together to stay bisectable.
gfs2's handler is in it too; it already marked errors this way and needed
the matching clear.  __bh_submit() keeps set_buffer_req(), and the local
'op' goes with the clear - it had no other user.

Chao

On Tue, Aug 4, 2026 at 5:19 AM Jan Kara <jack@xxxxxxx> wrote:
>
> On Sat 01-08-26 18:01:03, Chao Shi wrote:
> > A buffer whose write failed still holds exactly the data the filesystem
> > asked to be written.  It is the disk that is out of date, not the buffer.
> > Clearing BH_Uptodate says the opposite, and callers act on it:
> >
> >  - mark_buffer_dirty() has a WARN_ON_ONCE(!buffer_uptodate(bh)).  A
> >    filesystem that dirties the buffer again after a failed write - which is
> >    the normal way to retry - trips it.  That is the warning this series
> >    started from.
> >
> >  - a buffer that is not up to date gets re-read from disk, which replaces
> >    the data the filesystem was trying to write with the stale on-disk copy,
> >    silently.
> >
> >  - the window between the write completing and the buffer being marked not
> >    up to date is visible to anyone holding the folio lock, so the state is
> >    not even self consistent while it lasts.
> >
> > BH_Write_EIO already records the failure, and by now every place in the
> > tree that needs to know about it tests that flag instead: the two core
> > helpers in this file, adfs, exfat, ext2, ext4, fat, gfs2, jbd2, ocfs2 and
> > omfs, converted one filesystem at a time in the preceding patches.  The
> > private completion handlers in jbd2 and ext4 fast commit were converted
> > along with their waiters.  Nothing is left that reads BH_Uptodate to find
> > out whether a write failed, so the clears can go.
> >
> > Found by FuzzNvme.
> >
> > Signed-off-by: Chao Shi <coshi036@xxxxxxxxx>
> > ---
> >  fs/buffer.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index ac978d9090c2..0002f0736398 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -207,7 +207,6 @@ void bh_end_write(struct bio *bio)
> >       } else {
> >               buffer_io_error(bh, ", lost sync page write");
> >               mark_buffer_write_io_error(bh);
> > -             clear_buffer_uptodate(bh);
> >       }
> >       unlock_buffer(bh);
> >  }
> > @@ -441,7 +440,6 @@ void bh_end_async_write(struct bio *bio)
> >       } else {
> >               buffer_io_error(bh, ", lost async page write");
> >               mark_buffer_write_io_error(bh);
> > -             clear_buffer_uptodate(bh);
> >       }
> >
> >       first = folio_buffers(folio);
>
> Please remove setting of the uptodate flag from these functions as well.
> Write IO completion shouldn't really touch the uptodate flag at all...
>
> Hum, maybe we should actually clear the BH_Write_EIO flag on successful
> write completion in all the end io handlers. And then we can drop the
> clearing from __bh_submit(). And this will actually also address the
> concern Sahiko raised (mostly harmless but still) that if the user gets to
> check BH_Write_EIO after another process managed to redirty the buffer and
> resubmit it for IO, it can miss detecting IO error that happened.
>
>                                                                 Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR