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

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 <[email protected]>
> ---
>  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 <[email protected]>
SUSE Labs, CR