Re: [PATCH 1/3] jbd2: check return value of blkdev_issue_flush() when checkpointing

[Jan Kara]

On Mon 12-10-26 11:47:11, guolingxing wrote:
> jbd2_cleanup_journal_tail() calls blkdev_issue_flush() on the file
> system device to make sure all checkpointed data are on permanent
> storage before it updates the log tail in the superblock.  However
> the return value of blkdev_issue_flush() is ignored so if the flush
> fails, we still advance the log tail.  After a crash the journal
> recovery will consider the checkpointed transactions as committed
> and skip the replay while their data may have never reached the
> disk, which may lead to a filesystem corruption.
> 
> Abort the journal when the flush fails, similar to how we abort
> when detecting writeback errors of the file system device.
> 
> Fixes: 79feb521a447 ("jbd2: issue cache flush after checkpointing even with internal journal")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: guolingxing <guolingxing@xxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/jbd2/checkpoint.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 513273712010..9bbce4c8cadb 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -346,8 +346,14 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
>  	 * need this to guarantee correctness.  Fortunately
>  	 * jbd2_cleanup_journal_tail() doesn't get called all that often.
>  	 */
> -	if (journal->j_flags & JBD2_BARRIER)
> -		blkdev_issue_flush(journal->j_fs_dev);
> +	if (journal->j_flags & JBD2_BARRIER) {
> +		int err = blkdev_issue_flush(journal->j_fs_dev);
> +
> +		if (err) {
> +			jbd2_journal_abort(journal, err);
> +			return err;
> +		}
> +	}
>  
>  	return __jbd2_update_log_tail(journal, first_tid, blocknr);
>  }
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR