Re: [PATCH v2 21/21] buffer: clear BH_Write_EIO when a write succeeds, not when one starts

[Jan Kara]

On Thu 06-08-26 12:58:44, Chao Shi wrote:
> BH_Write_EIO is cleared in __bh_submit(), when a buffer that has been
> written before is submitted for write again.  That is early: it says the
> error is gone at the moment we start trying to fix it, rather than when we
> have.
> 
> It also loses errors.  A task whose write fails sets the flag and then goes
> to look at it; if another task redirties the buffer and resubmits it in
> between, the submission clears the flag and the first task sees no error at
> all.  Neither of them is doing anything wrong.
> 
> Clear it on successful write completion instead, in the end io handlers -
> the same four the rest of this series has been converting, plus gfs2's,
> which already marked errors this way.  Then the flag means what it says:
> the last write of this buffer that finished, failed.  A resubmission no
> longer hides an error that has not been fixed yet, and one that has been
> fixed clears the flag when the data reaches the disk.
> 
> __bh_submit() keeps setting BH_Req, which is what the rest of the tree
> reads it for.
> 
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Acked-by: Weidong Zhu <weizhu@xxxxxxx>
> Signed-off-by: Chao Shi <coshi036@xxxxxxxxx>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/buffer.c           | 15 +++++++--------
>  fs/ext4/fast_commit.c |  1 +
>  fs/gfs2/lops.c        |  2 ++
>  fs/jbd2/commit.c      |  4 +++-
>  4 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 425fbfe72ad1..68ea0ef8470e 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -202,7 +202,9 @@ void bh_end_write(struct bio *bio)
>  	struct buffer_head *bh;
>  	bool success = bio_endio_bh(bio, &bh);
>  
> -	if (!success) {
> +	if (success) {
> +		clear_buffer_write_io_error(bh);
> +	} else {
>  		buffer_io_error(bh, ", lost sync page write");
>  		mark_buffer_write_io_error(bh);
>  	}
> @@ -433,7 +435,9 @@ void bh_end_async_write(struct bio *bio)
>  	BUG_ON(!buffer_async_write(bh));
>  
>  	folio = bh->b_folio;
> -	if (!success) {
> +	if (success) {
> +		clear_buffer_write_io_error(bh);
> +	} else {
>  		buffer_io_error(bh, ", lost async page write");
>  		mark_buffer_write_io_error(bh);
>  	}
> @@ -1114,7 +1118,6 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
>  		enum rw_hint write_hint, struct writeback_control *wbc,
>  		bio_end_io_t end_bio)
>  {
> -	const enum req_op op = opf & REQ_OP_MASK;
>  	struct bio *bio;
>  
>  	BUG_ON(!buffer_locked(bh));
> @@ -1122,11 +1125,7 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
>  	BUG_ON(buffer_delay(bh));
>  	BUG_ON(buffer_unwritten(bh));
>  
> -	/*
> -	 * Only clear out a write error when rewriting
> -	 */
> -	if (test_set_buffer_req(bh) && (op == REQ_OP_WRITE))
> -		clear_buffer_write_io_error(bh);
> +	set_buffer_req(bh);
>  
>  	if (buffer_meta(bh))
>  		opf |= REQ_META;
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index a2028fbd4540..95998827ff01 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -209,6 +209,7 @@ static void ext4_end_buffer_io_sync(struct bio *bio)
>  	if (success) {
>  		ext4_debug("%s: Block %lld written",
>  			   __func__, bh->b_blocknr);
> +		clear_buffer_write_io_error(bh);
>  	} else {
>  		ext4_debug("%s: Block %lld write failed",
>  			   __func__, bh->b_blocknr);
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 3df6e4b7e8b9..7440e5b72f8a 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -179,6 +179,8 @@ static void gfs2_end_log_write_bh(struct gfs2_sbd *sdp, struct folio *folio,
>  	do {
>  		if (error)
>  			mark_buffer_write_io_error(bh);
> +		else
> +			clear_buffer_write_io_error(bh);
>  		unlock_buffer(bh);
>  		next = bh->b_this_page;
>  		size -= bh->b_size;
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index cd7ef783bd36..ebf6ba58ff4d 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -36,7 +36,9 @@ static void journal_end_buffer_io_sync(struct bio *bio)
>  	struct buffer_head *orig_bh = bh->b_private;
>  
>  	BUFFER_TRACE(bh, "");
> -	if (!success)
> +	if (success)
> +		clear_buffer_write_io_error(bh);
> +	else
>  		mark_buffer_write_io_error(bh);
>  	if (orig_bh) {
>  		clear_and_wake_up_bit(BH_Shadow, &orig_bh->b_state);
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR