Re: [PATCH v2 16/21] gfs2: check for a metadata write error with buffer_write_io_error()

[Jan Kara]

On Thu 06-08-26 12:58:39, Chao Shi wrote:
> gfs2_ail1_start_one() and gfs2_ail1_empty_one() decide whether a buffer on
> the ail reached the disk by looking at BH_Uptodate once it is no longer
> busy.  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.  In
> gfs2_ail1_start_one() the test is the positive one, so the sense has to be
> inverted rather than the flag simply swapped.
> 
> This is not a pure conversion for gfs2, because gfs2 already has a private
> write completion handler that behaves the way this series is heading:
> gfs2_end_log_write_bh() calls mark_buffer_write_io_error() and leaves
> BH_Uptodate alone.  Buffers completed through it are therefore invisible to
> both tests today, and start being caught once they look at BH_Write_EIO.
> That is a real behaviour change, and it is the one gfs2 wanted: a failed
> log write now withdraws the filesystem instead of passing silently.
> 
> gfs2_pin() is a different case and gets a different treatment.  Its
> !buffer_uptodate() test is not only a proxy for a failed write - a buffer
> with no valid contents at all is equally a reason to withdraw before
> pinning it into a transaction - so the write error test is added to it
> rather than replacing it.
> 
> Left alone deliberately: the BUG_ON(!buffer_uptodate(bh)) in gfs2_unpin()
> and the two WARN_ON()s in fs/gfs2/rgrp.c.  After this series they simply
> stop firing for write errors, which is correct; turning them into
> BUG_ON(buffer_write_io_error(bh)) would newly panic on an I/O error.
> 
> Acked-by: Weidong Zhu <weizhu@xxxxxxx>
> Signed-off-by: Chao Shi <coshi036@xxxxxxxxx>

It would be good to get ack from GFS2 developers here but the changes look
good to me so feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/gfs2/log.c  | 4 ++--
>  fs/gfs2/lops.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index 78bba8cc10b8..e3e0dcb1f567 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -107,7 +107,7 @@ __acquires(&sdp->sd_ail_lock)
>  		gfs2_assert(sdp, bd->bd_tr == tr);
>  
>  		if (!buffer_busy(bh)) {
> -			if (buffer_uptodate(bh)) {
> +			if (!buffer_write_io_error(bh)) {
>  				list_move(&bd->bd_ail_st_list,
>  					  &tr->tr_ail2_list);
>  				continue;
> @@ -321,7 +321,7 @@ static int gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr,
>  			active_count++;
>  			continue;
>  		}
> -		if (!buffer_uptodate(bh) &&
> +		if (buffer_write_io_error(bh) &&
>  		    !cmpxchg(&sdp->sd_log_error, 0, -EIO))
>  			gfs2_io_error_bh(sdp, bh);
>  		/*
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 6dabe73ad790..3df6e4b7e8b9 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -48,7 +48,7 @@ void gfs2_pin(struct gfs2_sbd *sdp, struct buffer_head *bh)
>  	clear_buffer_dirty(bh);
>  	if (test_set_buffer_pinned(bh))
>  		gfs2_assert_withdraw(sdp, 0);
> -	if (!buffer_uptodate(bh))
> +	if (!buffer_uptodate(bh) || buffer_write_io_error(bh))
>  		gfs2_io_error_bh(sdp, bh);
>  	bd = bh->b_private;
>  	/* If this buffer is in the AIL and it has already been written
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR