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

[Jan Kara]

On Mon 12-10-26 11:47:12, guolingxing wrote:
> For journals on a separate device, jbd2_commit_transaction() must
> flush the file system device before submitting the commit record,
> otherwise the transaction may get committed while the ordered data
> are still in the device cache.  However the return value of
> blkdev_issue_flush() is ignored so if the flush fails, the commit
> record is still submitted and fsync() reports success although the
> ordered data may have never reached the disk.  After a crash the
> journal recovery will consider such transactions committed and data
> written by fsync() may be lost.
> 
> Abort the journal when the flush fails.  The commit record is then
> not submitted anymore since journal_submit_commit_record() checks
> for an aborted journal.
> 
> Fixes: cc3e1bea5d87 ("ext4, jbd2: Add barriers for file systems with exernal journals")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: guolingxing <guolingxing@xxxxxxxxxx>
...
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 3029cb6f6d64..99477e9c78c7 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -800,8 +800,12 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  	 */
>  	if ((commit_transaction->t_need_data_flush || update_tail) &&
>  	    (journal->j_fs_dev != journal->j_dev) &&
> -	    (journal->j_flags & JBD2_BARRIER))
> -		blkdev_issue_flush(journal->j_fs_dev);
> +	    (journal->j_flags & JBD2_BARRIER)) {
> +		int err = blkdev_issue_flush(journal->j_fs_dev);

This shadows an 'err' definition of already existing variable. Just use
that variable instead. Otherwise the patch looks good.

								Honza

> +
> +		if (err)
> +			jbd2_journal_abort(journal, err);
> +	}
>  
>  	/* Done it all: now write the commit record asynchronously. */
>  	if (jbd2_has_feature_async_commit(journal)) {
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR