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

[Jan Kara]

On Wed 05-08-26 12:52:11, Zhang Yi wrote:
> On 8/2/2026 6:01 AM, 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>
> 
> It looks like after this series, we may need to revisit the necessity of
> ext4_buffer_uptodate() in the ext4 filesystem. IIRC, it was originally
> introduced as a compromise to address this issue. Now that we no longer
> clear the buffer uptodate flag after a failed IO write, this flag will
> still remain valid when getting/reading bh. Therefore, the necessity of
> ext4_buffer_uptodate() is worth reconsidering.

Deninitely :) There are several hacks in ext4 (and other filesystems as
well) that could be removed after this patch set. But let's settle on the
generic changes first.

								Honza

> > ---
> >  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);
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR