diff mbox series

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

Message ID 20241203014407.805916-3-yi.zhang@huaweicloud.com
State New
Headers show
Series jbd2: two straightforward fixes | expand

Commit Message

Zhang Yi Dec. 3, 2024, 1:44 a.m. UTC 
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(-)

Comments

Kemeng Shi Dec. 3, 2024, 6:53 a.m. UTC | #1 
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
Zhang Yi Dec. 3, 2024, 7:24 a.m. UTC | #2 
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.
Jan Kara Dec. 3, 2024, 2:54 p.m. UTC | #3 
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
>
Kemeng Shi Dec. 12, 2024, 12:34 p.m. UTC | #4 
on 12/3/2024 3:24 PM, Zhang Yi wrote:
> 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.
My bad...
Look good to me. Feel free to add:

Reviewed-by: Kemeng Shi <shikemeng@huaweicloud.com>
> 
> Thanks,
> Yi.
> 
>
Zhang Yi Dec. 12, 2024, 2:08 p.m. UTC | #5 
On 2024/12/12 20:34, Kemeng Shi wrote:
> 
> 
> on 12/3/2024 3:24 PM, Zhang Yi wrote:
>> 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.
> My bad...
> Look good to me. Feel free to add:
> 
> Reviewed-by: Kemeng Shi <shikemeng@huaweicloud.com>

It's fine, thanks for your review.

Cheers,
Yi.
diff mbox series

Patch

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);