Re: [PATCH v2 17/21] jbd2: report journal write errors with BH_Write_EIO

[Jan Kara]

On Thu 06-08-26 12:58:40, Chao Shi wrote:
> The journal's own write completion handler,
> journal_end_buffer_io_sync(), reports a failed write by clearing
> BH_Uptodate, and the three places that wait for journal writes look for
> that.  This series is removing that convention: a buffer whose write failed
> still holds the data that was supposed to reach the disk, and saying it is
> not up to date makes callers rewrite, re-read or WARN over data that was
> never wrong.
> 
> Set BH_Write_EIO instead, with mark_buffer_write_io_error(), and test it in
> journal_wait_on_commit_record() and in the two commit-phase waits.
> 
> The handler stops touching BH_Uptodate in either direction.  Setting it on
> success was never needed: every caller marks the buffer up to date before
> submitting the write, because a buffer with no valid data is not something
> you can write out.  The local flag is renamed to match what it now means.
> 
> The two changes have to go together, because commit phase 4 waits on a
> mixed list: descriptor blocks are submitted with
> journal_end_buffer_io_sync(), while revoke blocks go through
> write_dirty_buffer() and land in bh_end_write().  bh_end_write() already
> sets BH_Write_EIO, so converting the consumer alone would keep working for
> revoke blocks and silently stop detecting failed descriptor writes.  With
> the handler converted, both halves of the list report the same way.
> 
> mark_buffer_write_io_error() is safe on all of these buffers.  The shadow
> buffers from jbd2_journal_write_metadata_buffer() have no folio and no
> associated mapping, so it does nothing beyond setting the flag.  Descriptor
> and commit blocks are ordinary buffers on the journal device, and marking
> the journal's mapping with the error is what write_dirty_buffer() already
> does for revoke blocks on the same device.
> 
> 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/jbd2/commit.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 0c85af91f9b2..cd7ef783bd36 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -32,14 +32,12 @@
>  static void journal_end_buffer_io_sync(struct bio *bio)
>  {
>  	struct buffer_head *bh;
> -	bool uptodate = bio_endio_bh(bio, &bh);
> +	bool success = bio_endio_bh(bio, &bh);
>  	struct buffer_head *orig_bh = bh->b_private;
>  
>  	BUFFER_TRACE(bh, "");
> -	if (uptodate)
> -		set_buffer_uptodate(bh);
> -	else
> -		clear_buffer_uptodate(bh);
> +	if (!success)
> +		mark_buffer_write_io_error(bh);
>  	if (orig_bh) {
>  		clear_and_wake_up_bit(BH_Shadow, &orig_bh->b_state);
>  	}
> @@ -169,7 +167,7 @@ static int journal_wait_on_commit_record(journal_t *journal,
>  	clear_buffer_dirty(bh);
>  	wait_on_buffer(bh);
>  
> -	if (unlikely(!buffer_uptodate(bh)))
> +	if (unlikely(buffer_write_io_error(bh)))
>  		ret = -EIO;
>  	put_bh(bh);            /* One for getblk() */
>  
> @@ -834,7 +832,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  		wait_on_buffer(bh);
>  		cond_resched();
>  
> -		if (unlikely(!buffer_uptodate(bh)))
> +		if (unlikely(buffer_write_io_error(bh)))
>  			err = -EIO;
>  		jbd2_unfile_log_bh(bh);
>  		stats.run.rs_blocks_logged++;
> @@ -877,7 +875,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  		wait_on_buffer(bh);
>  		cond_resched();
>  
> -		if (unlikely(!buffer_uptodate(bh)))
> +		if (unlikely(buffer_write_io_error(bh)))
>  			err = -EIO;
>  
>  		BUFFER_TRACE(bh, "ph5: control buffer writeout done: unfile");
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR