jbd2: two straightforward fixes

[PATCH 2/2] jbd2: flush filesystem device before updating tail sequence

Posted by Zhang Yi 1 day, 17 hours ago

From: Zhang Yi <yi.zhang@huawei.com>

When committing transaction in jbd2_journal_commit_transaction(), the
disk caches for the filesystem device should be flushed before updating
the journal tail sequence. However, this step is missed if the journal
is not located on the filesystem device. As a result, the filesystem may
become inconsistent following a power failure or system crash. Fix it by
ensuring that the filesystem device is flushed appropriately.

Fixes: 3339578f0578 ("jbd2: cleanup journal tail after transaction commit")
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/jbd2/commit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 4305a1ac808a..f95cf272a1b5 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -776,9 +776,9 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	/*
 	 * If the journal is not located on the file system device,
 	 * then we must flush the file system device before we issue
-	 * the commit record
+	 * the commit record and update the journal tail sequence.
 	 */
-	if (commit_transaction->t_need_data_flush &&
+	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);
-- 
2.46.1

Re: [PATCH 2/2] jbd2: flush filesystem device before updating tail sequence

Posted by Jan Kara 1 day, 4 hours ago

On Tue 03-12-24 09:44:07, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> When committing transaction in jbd2_journal_commit_transaction(), the
> disk caches for the filesystem device should be flushed before updating
> the journal tail sequence. However, this step is missed if the journal
> is not located on the filesystem device. As a result, the filesystem may
> become inconsistent following a power failure or system crash. Fix it by
> ensuring that the filesystem device is flushed appropriately.
> 
> Fixes: 3339578f0578 ("jbd2: cleanup journal tail after transaction commit")
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Ah, good catch. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/jbd2/commit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 4305a1ac808a..f95cf272a1b5 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -776,9 +776,9 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  	/*
>  	 * If the journal is not located on the file system device,
>  	 * then we must flush the file system device before we issue
> -	 * the commit record
> +	 * the commit record and update the journal tail sequence.
>  	 */
> -	if (commit_transaction->t_need_data_flush &&
> +	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);
> -- 
> 2.46.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] jbd2: flush filesystem device before updating tail sequence

Posted by Kemeng Shi 1 day, 12 hours ago


on 12/3/2024 9:44 AM, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> When committing transaction in jbd2_journal_commit_transaction(), the
> disk caches for the filesystem device should be flushed before updating
> the journal tail sequence. However, this step is missed if the journal
> is not located on the filesystem device. As a result, the filesystem may
> become inconsistent following a power failure or system crash. Fix it by
> ensuring that the filesystem device is flushed appropriately.
> 
> Fixes: 3339578f0578 ("jbd2: cleanup journal tail after transaction commit")
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> ---
>  fs/jbd2/commit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 4305a1ac808a..f95cf272a1b5 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -776,9 +776,9 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  	/*
>  	 * If the journal is not located on the file system device,
>  	 * then we must flush the file system device before we issue
> -	 * the commit record
> +	 * the commit record and update the journal tail sequence.
>  	 */
> -	if (commit_transaction->t_need_data_flush &&
> +	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);
> 
In journal_submit_commit_record(), we will submit commit block with REQ_PREFLUSH
which is supposed to ensure disk cache is flushed before writing commit block.
So I think the current code is fine.
Please correct me if I miss anything.

Thanks,
Kemeng

Re: [PATCH 2/2] jbd2: flush filesystem device before updating tail sequence

Posted by Zhang Yi 1 day, 11 hours ago

On 2024/12/3 14:53, Kemeng Shi wrote:
> 
> 
> on 12/3/2024 9:44 AM, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> When committing transaction in jbd2_journal_commit_transaction(), the
>> disk caches for the filesystem device should be flushed before updating
>> the journal tail sequence. However, this step is missed if the journal
>> is not located on the filesystem device. As a result, the filesystem may
>> become inconsistent following a power failure or system crash. Fix it by
>> ensuring that the filesystem device is flushed appropriately.
>>
>> Fixes: 3339578f0578 ("jbd2: cleanup journal tail after transaction commit")
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> ---
>>  fs/jbd2/commit.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
>> index 4305a1ac808a..f95cf272a1b5 100644
>> --- a/fs/jbd2/commit.c
>> +++ b/fs/jbd2/commit.c
>> @@ -776,9 +776,9 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>>  	/*
>>  	 * If the journal is not located on the file system device,
>>  	 * then we must flush the file system device before we issue
>> -	 * the commit record
>> +	 * the commit record and update the journal tail sequence.
>>  	 */
>> -	if (commit_transaction->t_need_data_flush &&
>> +	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);
>>
> In journal_submit_commit_record(), we will submit commit block with REQ_PREFLUSH
> which is supposed to ensure disk cache is flushed before writing commit block.
> So I think the current code is fine.
> Please correct me if I miss anything.
> 

The commit I/O with REQ_PREFLUSH only flushes 'journal->j_dev', not
'journal->j_fs_dev'. We need to flush journal->j_fs_dev to ensure that all
written metadata has been persisted to the filesystem disk, Until then, we
cannot update the tail sequence.

Thanks,
Yi.