Re: [PATCH 14/19] ocfs2: check for a stale write error before reusing a metadata buffer

On Sat 01-08-26 18:00:58, Chao Shi wrote:
> __ocfs2_journal_access() refuses to journal a buffer whose previous write
> failed, and turns the filesystem read-only rather than risk metadata
> inconsistency.  That check sits inside an if (!buffer_uptodate(bh)) block,
> because until now a failed write also cleared BH_Uptodate.
> 
> This series stops clearing BH_Uptodate on write error, so that outer test
> would never fire again and ocfs2 would silently start reusing buffers whose
> last write failed.  Hoist the check out of the debug block, where it does
> not depend on BH_Uptodate any more, and drop the now dead second half of
> its condition.
> 
> The mlog() pair keeps its own !buffer_uptodate() guard: it is a separate
> "we can safely remove this assertion after testing" debug aid about being
> handed a buffer with no valid contents, which is a different question from
> whether the last write of that buffer failed.
> 
> The unlocked test followed by a locked retest is deliberate.  BH_Write_EIO
> is cleared under the buffer lock when the buffer is submitted for write
> again, so taking the lock and looking a second time avoids turning the
> filesystem read-only over an error that a concurrent rewrite has already
> cleared, while keeping the common case lock-free.
> 
> The code in this patch is Jan's, from the review discussion linked in the
> cover letter.
> 
> Suggested-by: Jan Kara <[email protected]>
> Signed-off-by: Chao Shi <[email protected]>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/ocfs2/journal.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> index d8afbc1a76bb..ea6802d894c2 100644
> --- a/fs/ocfs2/journal.c
> +++ b/fs/ocfs2/journal.c
> @@ -676,19 +676,20 @@ static int __ocfs2_journal_access(handle_t *handle,
>  		mlog(ML_ERROR, "giving me a buffer that's not uptodate!\n");
>  		mlog(ML_ERROR, "b_blocknr=%llu, b_state=0x%lx\n",
>  		     (unsigned long long)bh->b_blocknr, bh->b_state);
> -
> +	}
> +	/*
> +	 * A previous transaction with a couple of buffer heads fail
> +	 * to checkpoint, so all the bhs are marked as BH_Write_EIO.
> +	 * For current transaction, the bh is just among those error
> +	 * bhs which previous transaction handle. We can't just clear
> +	 * its BH_Write_EIO and reuse directly, since other bhs are
> +	 * not written to disk yet and that will cause metadata
> +	 * inconsistency. So we should set fs read-only to avoid
> +	 * further damage.
> +	 */
> +	if (buffer_write_io_error(bh)) {
>  		lock_buffer(bh);
> -		/*
> -		 * A previous transaction with a couple of buffer heads fail
> -		 * to checkpoint, so all the bhs are marked as BH_Write_EIO.
> -		 * For current transaction, the bh is just among those error
> -		 * bhs which previous transaction handle. We can't just clear
> -		 * its BH_Write_EIO and reuse directly, since other bhs are
> -		 * not written to disk yet and that will cause metadata
> -		 * inconsistency. So we should set fs read-only to avoid
> -		 * further damage.
> -		 */
> -		if (buffer_write_io_error(bh) && !buffer_uptodate(bh)) {
> +		if (buffer_write_io_error(bh)) {
>  			unlock_buffer(bh);
>  			return ocfs2_error(osb->sb, "A previous attempt to "
>  					"write this buffer head failed\n");
> -- 
> 2.43.0
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR