[PATCH] ext4:Fix WARNING in ext4_journalled_invalidate_folio

[PATCH] ext4:Fix WARNING in ext4_journalled_invalidate_folio

[shuo chen]

when the bh's transaction is committing and the bh is part of the folio,
journal_unmap_buffer returns -EBUSY and triggers a warning.

Reported-by: Farhad Alemi <farhad.alemi@berkeley.edu>
Closes:https://lore.kernel.org/linux-ext4/CA+0ovCgxFN3TLpdS_O_Z48sARuQ-WR-JLDCqQWEmd9ELtkzddA@mail.gmail.com/T/
Signed-off-by: shuo chen <1289151713@qq.com>
---
 fs/ext4/inode.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index c2c2d6ac7f3d..6bb94afd5100 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3429,7 +3429,27 @@ static void ext4_journalled_invalidate_folio(struct folio *folio,
 					   size_t offset,
 					   size_t length)
 {
-	WARN_ON(__ext4_journalled_invalidate_folio(folio, offset, length) < 0);
+	tid_t commit_tid;
+	int ret;
+	bool has_transaction;
+	journal_t *journal = EXT4_JOURNAL(folio->mapping->host);
+
+	while (1) {
+		ret = __ext4_journalled_invalidate_folio(folio, offset, length);
+		if (ret != -EBUSY) {
+			WARN_ON(ret < 0);
+			return;
+		}
+		has_transaction = false;
+		read_lock(&journal->j_state_lock);
+		if (journal->j_committing_transaction) {
+			commit_tid = journal->j_committing_transaction->t_tid;
+			has_transaction = true;
+		}
+		read_unlock(&journal->j_state_lock);
+		if (has_transaction)
+			jbd2_log_wait_commit(journal, commit_tid);
+	}
 }
 
 static bool ext4_release_folio(struct folio *folio, gfp_t wait)
-- 
2.55.0

Re: [PATCH] ext4:Fix WARNING in ext4_journalled_invalidate_folio

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ABBA deadlock between truncating thread and journal commit thread due to holding the folio lock while waiting for a transaction commit.
--

commit 0b3a826d64ad8fde652eef131d6bd13542e8206b
Author: shuo chen <1289151713@qq.com>

ext4:Fix WARNING in ext4_journalled_invalidate_folio

This commit modifies ext4_journalled_invalidate_folio() to handle -EBUSY
from __ext4_journalled_invalidate_folio() by waiting for the current
transaction to commit before retrying.

Closes: https://lore.kernel.org/linux-ext4/CA+0ovCgxFN3TLpdS_O_Z48sARuQ-WR-JLDCqQWEmd9ELtkzddA@mail.gmail.com/T/

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 26f0f9714f03c..e5e78d5551474 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3419,7 +3419,27 @@ static void ext4_journalled_invalidate_folio(struct folio *folio,
>  					   size_t offset,
>  					   size_t length)
>  {
> -	WARN_ON(__ext4_journalled_invalidate_folio(folio, offset, length) < 0);
> +	tid_t commit_tid;
> +	int ret;
> +	bool has_transaction;
> +	journal_t *journal = EXT4_JOURNAL(folio->mapping->host);
> +
> +	while (1) {
> +		ret = __ext4_journalled_invalidate_folio(folio, offset, length);
> +		if (ret != -EBUSY) {
> +			WARN_ON(ret < 0);
> +			return;
> +		}
> +		has_transaction = false;
> +		read_lock(&journal->j_state_lock);
> +		if (journal->j_committing_transaction) {
> +			commit_tid = journal->j_committing_transaction->t_tid;
> +			has_transaction = true;
> +		}
> +		read_unlock(&journal->j_state_lock);
> +		if (has_transaction)
> +			jbd2_log_wait_commit(journal, commit_tid);

[Severity: High]
Does waiting for a journal commit while holding the folio lock cause an ABBA
deadlock with the JBD2 commit thread?

The truncation thread enters this path with the folio locked. If it sleeps
here, it blocks while holding the folio lock.

Meanwhile, the main JBD2 commit thread loops over dirty folios during commit:

fs/ext4/super.c:ext4_journalled_submit_inode_data_buffers() {
    ...
	while ((folio = writeback_iter(mapping, &wbc, folio, &error))) {
		if (ext4_journalled_writepage_needs_redirty(jinode, folio))
			folio_redirty_for_writepage(&wbc, folio);
		folio_unlock(folio);
	}
    ...
}

The writeback_iter() function internally calls folio_lock(). If it reaches
the folio held by the truncation thread, the commit thread will block
waiting for the folio lock, while the truncation thread is blocked waiting
for the commit thread to finish, leading to a complete filesystem stall.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_D74F4B8A5FDC1BCFC23B8750428019635609@qq.com?part=1

Re: [PATCH] ext4:Fix WARNING in ext4_journalled_invalidate_folio

[Jan Kara]

On Mon 14-09-26 10:19:20, shuo chen wrote:
> when the bh's transaction is committing and the bh is part of the folio,
> journal_unmap_buffer returns -EBUSY and triggers a warning.
> 
> Reported-by: Farhad Alemi <farhad.alemi@berkeley.edu>
> Closes:https://lore.kernel.org/linux-ext4/CA+0ovCgxFN3TLpdS_O_Z48sARuQ-WR-JLDCqQWEmd9ELtkzddA@mail.gmail.com/T/
> Signed-off-by: shuo chen <1289151713@qq.com>

This is a wrong way to fix that warning and would introduce deadlocks into
the folio truncation in data=journal mode since we'd be waiting for
transaction commit with folio lock held.

The proper fix is to use ext4_wait_for_tail_page_commit() helper before
calling ext4_truncate_failed_write() from ext4_journalled_write_end().

								Honza

> ---
>  fs/ext4/inode.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index c2c2d6ac7f3d..6bb94afd5100 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3429,7 +3429,27 @@ static void ext4_journalled_invalidate_folio(struct folio *folio,
>  					   size_t offset,
>  					   size_t length)
>  {
> -	WARN_ON(__ext4_journalled_invalidate_folio(folio, offset, length) < 0);
> +	tid_t commit_tid;
> +	int ret;
> +	bool has_transaction;
> +	journal_t *journal = EXT4_JOURNAL(folio->mapping->host);
> +
> +	while (1) {
> +		ret = __ext4_journalled_invalidate_folio(folio, offset, length);
> +		if (ret != -EBUSY) {
> +			WARN_ON(ret < 0);
> +			return;
> +		}
> +		has_transaction = false;
> +		read_lock(&journal->j_state_lock);
> +		if (journal->j_committing_transaction) {
> +			commit_tid = journal->j_committing_transaction->t_tid;
> +			has_transaction = true;
> +		}
> +		read_unlock(&journal->j_state_lock);
> +		if (has_transaction)
> +			jbd2_log_wait_commit(journal, commit_tid);
> +	}
>  }
>  
>  static bool ext4_release_folio(struct folio *folio, gfp_t wait)
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2] ext4:Fix WARNING in ext4_journalled_invalidate_folio

[shuo chen]

Thanks to Jack's suggestion.
Call the ext4_wait_for_tail_page_commit() helper before
ext4_truncate_failed_write() from ext4_journalled_write_end(),
which avoids invalidating the folio while it is being committed.

Suggested-by: Jan Kara <jack@suse.cz>
Reported-by: Farhad Alemi <farhad.alemi@berkeley.edu>
Closes:https://lore.kernel.org/linux-ext4/CA+0ovCgxFN3TLpdS_O_Z48sARuQ-WR-JLDCqQWEmd9ELtkzddA@mail.gmail.com/T/
Signed-off-by: shuo chen <1289151713@qq.com>
---
 fs/ext4/inode.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index c2c2d6ac7f3d..10a89bbb994a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -56,6 +56,7 @@ static void ext4_journalled_zero_new_buffers(handle_t *handle,
 					    struct inode *inode,
 					    struct folio *folio,
 					    unsigned from, unsigned to);
+static void ext4_wait_for_tail_page_commit(struct inode *inode);
 
 static __u32 ext4_inode_csum(struct inode *inode, struct ext4_inode *raw,
 			      struct ext4_inode_info *ei)
@@ -1605,6 +1606,7 @@ static int ext4_journalled_write_end(const struct kiocb *iocb,
 	if (!ret)
 		ret = ret2;
 	if (pos + len > inode->i_size && !verity) {
+		ext4_wait_for_tail_page_commit(inode);
 		ext4_truncate_failed_write(inode);
 		/*
 		 * If truncate failed early the inode might still be
-- 
2.55.0

Re: [PATCH v2] ext4:Fix WARNING in ext4_journalled_invalidate_folio

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The WARNING in ext4_journalled_invalidate_folio can still be triggered from ext4_write_begin on error paths.
--

commit 83d8cc4138093c0b6b19db042eb3a85e72ca5b68
Author: shuo chen <1289151713@qq.com>

    ext4:Fix WARNING in ext4_journalled_invalidate_folio

This commit fixes a warning in ext4_journalled_invalidate_folio() by
calling ext4_wait_for_tail_page_commit() before truncating a failed write
in ext4_journalled_write_end(). This prevents invalidating the folio while
it is being committed.

Suggested-by: Jan Kara <jack@suse.cz>
Reported-by: Farhad Alemi <farhad.alemi@berkeley.edu>
Closes:https://lore.kernel.org/linux-ext4/CA+0ovCgxFN3TLpdS_O_Z48sARuQ-WR-JLDCqQWEmd9ELtkzddA@mail.gmail.com/T/
Signed-off-by: shuo chen <1289151713@qq.com>

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 26f0f9714f03c..960a049a67bd2 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c

[ ... ]

> @@ -1597,6 +1598,7 @@ static int ext4_journalled_write_end(const struct kiocb *iocb,
>  	if (!ret)
>  		ret = ret2;
>  	if (pos + len > inode->i_size && !verity) {
> +		ext4_wait_for_tail_page_commit(inode);
>  		ext4_truncate_failed_write(inode);
>  		/*
>  		 * If truncate failed early the inode might still be

[Severity: High]
This isn't a bug introduced by this patch, but it appears the same issue
might still exist on the error paths in ext4_write_begin().

When a write fails (e.g., due to ENOSPC or block allocation failure) and
extends i_size on a data=journal file, does ext4_write_begin() also need
to wait for the tail page commit?

fs/ext4/inode.c:ext4_write_begin() {
	...
		ext4_journal_stop(handle);
		if (extended) {
			ext4_truncate_failed_write(inode);
	...
}

If ext4_truncate_failed_write() attempts to invalidate the folio without
waiting, jbd2_journal_invalidate_folio() could return -EBUSY because the
buffer is still committing. Could this leave the page cache in an
inconsistent state and trigger the WARN_ON in
ext4_journalled_invalidate_folio()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_87F5A404ED65B0B1349C60E830C485716705@qq.com?part=1

Re: [PATCH v2] ext4:Fix WARNING in ext4_journalled_invalidate_folio

[Jan Kara]

On Tue 15-09-26 00:41:44, shuo chen wrote:
> Thanks to Jack's suggestion.
> Call the ext4_wait_for_tail_page_commit() helper before
> ext4_truncate_failed_write() from ext4_journalled_write_end(),
> which avoids invalidating the folio while it is being committed.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Reported-by: Farhad Alemi <farhad.alemi@berkeley.edu>
> Closes:https://lore.kernel.org/linux-ext4/CA+0ovCgxFN3TLpdS_O_Z48sARuQ-WR-JLDCqQWEmd9ELtkzddA@mail.gmail.com/T/
> Signed-off-by: shuo chen <1289151713@qq.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/ext4/inode.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index c2c2d6ac7f3d..10a89bbb994a 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -56,6 +56,7 @@ static void ext4_journalled_zero_new_buffers(handle_t *handle,
>  					    struct inode *inode,
>  					    struct folio *folio,
>  					    unsigned from, unsigned to);
> +static void ext4_wait_for_tail_page_commit(struct inode *inode);
>  
>  static __u32 ext4_inode_csum(struct inode *inode, struct ext4_inode *raw,
>  			      struct ext4_inode_info *ei)
> @@ -1605,6 +1606,7 @@ static int ext4_journalled_write_end(const struct kiocb *iocb,
>  	if (!ret)
>  		ret = ret2;
>  	if (pos + len > inode->i_size && !verity) {
> +		ext4_wait_for_tail_page_commit(inode);
>  		ext4_truncate_failed_write(inode);
>  		/*
>  		 * If truncate failed early the inode might still be
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR