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

[Jan Kara]

On Sat 01-08-26 18:00:57, Chao Shi wrote:
> ocfs2_write_block() and ocfs2_write_super_or_backup() detect a failed write
> by looking at BH_Uptodate afterwards.  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.
> 
> Test BH_Write_EIO instead.  Note that ocfs2_write_block()'s test is the
> positive one, so the sense has to be inverted rather than the flag simply
> swapped.
> 
> The comment in ocfs2_write_block()'s error arm needs updating for the same
> reason.  It said the clustered uptodate information did not have to be
> removed because the buffer was not marked locally uptodate; after this
> series it is, so the reason no longer holds.  Not advertising the block to
> the cluster is still the right thing to do - the data is in memory but not
> on disk - so only the justification changes, not the behaviour.
> 
> 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 <coshi036@xxxxxxxxx>

This looks ok to me so feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

but it would be good to get a feedback from ocfs2 maintainers whether the
logic change in ocfs2_write_block() where we now keep the cluster cache
block uptodate after write IO error doesn't break some expectations of
ocfs2.

								Honza

> ---
>  fs/ocfs2/buffer_head_io.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
> index 7bfe377af2df..733ceda79ca1 100644
> --- a/fs/ocfs2/buffer_head_io.c
> +++ b/fs/ocfs2/buffer_head_io.c
> @@ -66,12 +66,14 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh,
>  
>  	wait_on_buffer(bh);
>  
> -	if (buffer_uptodate(bh)) {
> +	if (!buffer_write_io_error(bh)) {
>  		ocfs2_set_buffer_uptodate(ci, bh);
>  	} else {
> -		/* We don't need to remove the clustered uptodate
> -		 * information for this bh as it's not marked locally
> -		 * uptodate. */
> +		/*
> +		 * The buffer still holds what we tried to write, but it did
> +		 * not reach the disk, so don't advertise it to the cluster
> +		 * as up to date.
> +		 */
>  		ret = -EIO;
>  		mlog_errno(ret);
>  	}
> @@ -446,7 +448,7 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb,
>  
>  	wait_on_buffer(bh);
>  
> -	if (!buffer_uptodate(bh)) {
> +	if (buffer_write_io_error(bh)) {
>  		ret = -EIO;
>  		mlog_errno(ret);
>  	}
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR