Re: [PATCH 16/19] jbd2: report journal write errors with BH_Write_EIO

On Sat 01-08-26 18:01:00, 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 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.
> 
> Signed-off-by: Chao Shi <[email protected]>
> ---
>  fs/jbd2/commit.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 60273cddf434..8912786ebebb 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -39,7 +39,7 @@ static void journal_end_buffer_io_sync(struct bio *bio)
>  	if (uptodate)
>  		set_buffer_uptodate(bh);
>  	else
> -		clear_buffer_uptodate(bh);
> +		mark_buffer_write_io_error(bh);

There's no need to set uptodate flag here at all (all callers set it before
submitting IO). So you can just simplify this to:

	if (!uptodate)
		mark_buffer_write_io_error(bh);

Also I'd rename the local variable from 'uptodate' to 'ioerr' or something
like that.

								Honza

>  	if (orig_bh) {
>  		clear_and_wake_up_bit(BH_Shadow, &orig_bh->b_state);
>  	}
> @@ -169,7 +169,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() */
>  
> @@ -840,7 +840,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++;
> @@ -883,7 +883,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 <[email protected]>
SUSE Labs, CR