Re: [PATCH 12/19] ext4: check for a metadata write error with buffer_write_io_error()

On Sat 01-08-26 18:00:56, Chao Shi wrote:
> Two places detect a failed metadata write by testing !buffer_uptodate()
> after waiting for it.  That relies on the write completion handler clearing
> BH_Uptodate on error, which this series removes: a buffer whose write
> failed still holds the data the filesystem asked to be written, so
> declaring it not up to date is wrong and makes callers re-read it.
> 
> ext4 already does this correctly for the superblock - see
> ext4_commit_super(), which tests buffer_write_io_error() - so this brings
> the other two into line.
> 
> In __ext4_handle_dirty_metadata() the old test also required BH_Req.
> BH_Write_EIO implies it, so the pair collapses into one test.  The new test
> is also strictly stronger than consuming sync_dirty_buffer()'s return
> value, because it still fires when the buffer was written by background
> writeback and that write hit an error, which sync_dirty_buffer() does not
> report.
> 
> Note that BH_Write_EIO stays set until the buffer is written again,
> forgotten or invalidated, so an unrepaired itable block now reports on
> every subsequent sync of that inode rather than only on the write that
> failed.  That is the intended behaviour and matches what ocfs2 has always
> done with this flag.
> 
> No behaviour change today - a failed write sets BH_Write_EIO and clears
> BH_Uptodate together.  It stops being a no-op at the end of the series,
> where the new test is the one that still works.
> 
> Signed-off-by: Chao Shi <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

								Honza

> ---
>  fs/ext4/ext4_jbd2.c | 2 +-
>  fs/ext4/mmp.c       | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
> index 02b066299164..f338d6e3c29f 100644
> --- a/fs/ext4/ext4_jbd2.c
> +++ b/fs/ext4/ext4_jbd2.c
> @@ -413,7 +413,7 @@ int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
>  		}
>  		if (inode && inode_needs_sync(inode)) {
>  			sync_dirty_buffer(bh);
> -			if (buffer_req(bh) && !buffer_uptodate(bh)) {
> +			if (buffer_write_io_error(bh)) {
>  				ext4_error_inode_err(inode, where, line,
>  						     bh->b_blocknr, EIO,
>  					"IO error syncing itable block");
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 7ce361484b38..4b18ddef468d 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -49,7 +49,7 @@ static int write_mmp_block_thawed(struct super_block *sb,
>  	bh_submit(bh, REQ_OP_WRITE | REQ_SYNC | REQ_META | REQ_PRIO,
>  			bh_end_write);
>  	wait_on_buffer(bh);
> -	if (unlikely(!buffer_uptodate(bh)))
> +	if (unlikely(buffer_write_io_error(bh)))
>  		return -EIO;
>  	return 0;
>  }
> -- 
> 2.43.0
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR