Re: [PATCH v2 20/21] buffer: stop touching BH_Uptodate on write completion

[Jan Kara]

On Thu 06-08-26 12:58:43, 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.
> 
> Setting BH_Uptodate on success goes too.  A buffer has to be up to date
> before it can be written - you cannot write out data you do not have - so
> the only thing that assignment could do is paper over a caller that got
> that wrong.  Write completion now leaves BH_Uptodate alone in both
> directions.
> 
> What this changes for readers.  A buffer whose write failed stays up to
> date, so the read paths stop replacing it with the on-disk copy:
> __bread_gfp() no longer sends it to __bread_slow(), and
> bh_uptodate_or_lock() reports it as usable.  That is the intent.  ocfs2
> changes the most, because ocfs2_read_blocks() decides whether to go to disk
> on its own cluster uptodate cache and only tests BH_Uptodate after the
> wait, so a block whose write failed makes that read return -EIO today and
> from here it succeeds and hands back the in-memory data.  A caller that
> needs to know the write failed asks BH_Write_EIO.
> 
> Found by FuzzNvme.
> 
> Acked-by: Weidong Zhu <weizhu@xxxxxxx>
> Signed-off-by: Chao Shi <coshi036@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/buffer.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index aebf74abbc49..425fbfe72ad1 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -202,12 +202,9 @@ void bh_end_write(struct bio *bio)
>  	struct buffer_head *bh;
>  	bool success = bio_endio_bh(bio, &bh);
>  
> -	if (success) {
> -		set_buffer_uptodate(bh);
> -	} else {
> +	if (!success) {
>  		buffer_io_error(bh, ", lost sync page write");
>  		mark_buffer_write_io_error(bh);
> -		clear_buffer_uptodate(bh);
>  	}
>  	unlock_buffer(bh);
>  }
> @@ -436,12 +433,9 @@ void bh_end_async_write(struct bio *bio)
>  	BUG_ON(!buffer_async_write(bh));
>  
>  	folio = bh->b_folio;
> -	if (success) {
> -		set_buffer_uptodate(bh);
> -	} else {
> +	if (!success) {
>  		buffer_io_error(bh, ", lost async page write");
>  		mark_buffer_write_io_error(bh);
> -		clear_buffer_uptodate(bh);
>  	}
>  
>  	first = folio_buffers(folio);
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR