[PATCH] ext4: record writeback error when discarding pages on failed delalloc

[PATCH] ext4: record writeback error when discarding pages on failed delalloc

[guolingxing]

When mpage_map_and_submit_extent() gives up on a failed block mapping,
it sets give_up_on_write so that mpage_release_unused_pages() discards
the remaining dirty folios. However, the error is not recorded in the
mapping's writeback error state.

If this happens during background writeback, a later fsync() may
report success even though the dirty data was discarded and never
written. Returning the error from ext4_writepages() is not sufficient
to notify a later fsync(). In addition, ext4_do_writepages() may clear
-ENOSPC when committing the journal and retrying, even after the
affected dirty folios have been discarded.

Call mapping_set_error() when deciding to give up on the write,
before the dirty folios are invalidated, so that fsync() can report
the writeback failure through the mapping's error state.

Fixes: c4a0c46ec92c ("ext4: invalidate pages if delalloc block allocation fails.")
Cc: stable@vger.kernel.org
Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
---
 fs/ext4/inode.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 26f0f9714f03..dba903d188e2 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2505,6 +2505,12 @@ static int mpage_map_and_submit_extent(handle_t *handle,
 				ext4_print_free_blocks(inode);
 		invalidate_dirty_pages:
 			*give_up_on_write = true;
+			/*
+			 * Record the error so that a subsequent fsync()
+			 * / errseq check can observe the data loss that
+			 * discarding these dirty pages implies.
+			 */
+			mapping_set_error(inode->i_mapping, err);
 			return err;
 		}
 		progress = 1;
-- 
2.34.1

Re: [PATCH] ext4: record writeback error when discarding pages on failed delalloc

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: data loss due to leaked give_up_on_write flag across writepages loop iterations
--

commit b95d276ed2e575cb01a2eaee0f04e704d2371b9a
Author: guolingxing <guolingxing@tju.edu.cn>

ext4: record writeback error when discarding pages on failed delalloc

When mpage_map_and_submit_extent() gives up on a failed block mapping,
it sets give_up_on_write so that mpage_release_unused_pages() discards
the remaining dirty folios. This commit calls mapping_set_error() when
deciding to give up on the write so that fsync() can report the
writeback failure.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6ede3644a738b..27640409aa97b 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2637,6 +2637,12 @@ static int mpage_map_and_submit_extent(handle_t *handle,
>  				ext4_print_free_blocks(inode);
>  		invalidate_dirty_pages:
>  			*give_up_on_write = true;

[Severity: High]
Does ext4_do_writepages() leak this true value across loop iterations?

If this hits an -ENOSPC error, give_up_on_write is set to true. In
ext4_do_writepages(), it catches the -ENOSPC, forces a journal commit,
and executes continue to retry writing further pages:

fs/ext4/inode.c:ext4_do_writepages() {
    bool give_up_on_write = false;
    ...
    while (!mpd->scanned_until_end && wbc->nr_to_write > 0) {
        ...
        ret = mpage_map_and_submit_extent(handle, mpd,
                &give_up_on_write);
        ...
        mpage_release_unused_pages(mpd, give_up_on_write);
        ...
        if (ret == -ENOSPC && sbi->s_journal) {
            /*
             * Commit the transaction which would
             * free blocks released in the transaction
             * and try again
             */
            jbd2_journal_force_commit_nested(sbi->s_journal);
            ret = 0;
            continue;
        }
        ...
    }
}

Since give_up_on_write is initialized outside the loop, it isn't reset to
false. In the next iteration, if a transient error occurs (like -EAGAIN),
the leaked true value causes mpage_release_unused_pages() to aggressively
invalidate innocent unwritten folios, causing silent data loss.

Should give_up_on_write be reset per-iteration?

> +			/*
> +			 * Record the error so that a subsequent fsync()
> +			 * / errseq check can observe the data loss that
> +			 * discarding these dirty pages implies.
> +			 */
> +			mapping_set_error(inode->i_mapping, err);
>  			return err;
>  		}
>  		progress = 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016101605.4039217-1-guolingxing@tju.edu.cn?part=1

[PATCH v2] ext4: record writeback error when discarding pages on failed delalloc

[guolingxing]

When mpage_map_and_submit_extent() gives up on a failed block mapping,
it sets give_up_on_write so that mpage_release_unused_pages() discards
the remaining dirty folios. However, the error is not recorded in the
mapping's writeback error state.

If this happens during background writeback, a later fsync() may
report success even though the dirty data was discarded and never
written. Returning the error from ext4_writepages() is not sufficient
to notify a later fsync(). In addition, ext4_do_writepages() may clear
-ENOSPC when committing the journal and retrying, even after the
affected dirty folios have been discarded.

Call mapping_set_error() when deciding to give up on the write,
before the dirty folios are invalidated, so that fsync() can report
the writeback failure through the mapping's error state.

The give_up_on_write flag also persists across writeback loop
iterations. After a fatal -ENOSPC failure sets it, ext4_do_writepages()
may commit the journal and retry without clearing the flag. If a
subsequent iteration encounters a transient error and leaves dirty
folios pending, mpage_release_unused_pages() invalidates them instead
of preserving them for retry. Initialize give_up_on_write inside the
loop so that only a fatal failure in the current iteration causes
the remaining dirty folios to be discarded.

Fixes: c4a0c46ec92c ("ext4: invalidate pages if delalloc block allocation fails.")
Fixes: cb530541182b ("ext4: fix up error handling for mpage_map_and_submit_extent()")
Cc: stable@vger.kernel.org
Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
---
Changes since v1:
- Initialize give_up_on_write in each writeback iteration to prevent
  a stale value from invalidating dirty folios after an ENOSPC retry.
- Update the commit message and add the corresponding Fixes tag.

 fs/ext4/inode.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 26f0f9714f03..ae54212e0a7d 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2505,6 +2505,12 @@ static int mpage_map_and_submit_extent(handle_t *handle,
 				ext4_print_free_blocks(inode);
 		invalidate_dirty_pages:
 			*give_up_on_write = true;
+			/*
+			 * Record the error so that a subsequent fsync()
+			 * / errseq check can observe the data loss that
+			 * discarding these dirty pages implies.
+			 */
+			mapping_set_error(inode->i_mapping, err);
 			return err;
 		}
 		progress = 1;
@@ -2774,7 +2780,6 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 	int needed_blocks, rsv_blocks = 0, ret = 0;
 	struct ext4_sb_info *sbi = EXT4_SB(mapping->host->i_sb);
 	struct blk_plug plug;
-	bool give_up_on_write = false;
 
 	trace_ext4_writepages(inode, wbc);
 
@@ -2892,6 +2897,8 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 		goto unplug;
 
 	while (!mpd->scanned_until_end && wbc->nr_to_write > 0) {
+		bool give_up_on_write = false;
+
 		/* For each extent of pages we use new io_end */
 		mpd->io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
 		if (!mpd->io_submit.io_end) {
-- 
2.34.1

Re: [PATCH v2] ext4: record writeback error when discarding pages on failed delalloc

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ext4: folio reference leak on error path in mpage_submit_partial_folio()
--

commit 828f6b6c9cdd25b981807944a30cd22b2612d38e
Author: guolingxing <guolingxing@tju.edu.cn>
Subject: ext4: record writeback error when discarding pages on failed delalloc

This commit records writeback errors when block mapping fails and dirty
folios are discarded. It also scopes the give_up_on_write flag to each
writeback loop iteration to prevent transient errors from invalidating
dirty folios after a fatal ENOSPC retry.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6ede3644a738b..b3b9650b32116 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2637,6 +2637,12 @@ static int mpage_map_and_submit_extent(handle_t *handle,
>  				ext4_print_free_blocks(inode);
>  		invalidate_dirty_pages:
>  			*give_up_on_write = true;
> +			/*
> +			 * Record the error so that a subsequent fsync()
> +			 * / errseq check can observe the data loss that
> +			 * discarding these dirty pages implies.
> +			 */
> +			mapping_set_error(inode->i_mapping, err);

[Severity: Medium]
Is there a memory leak in the error path of mpage_submit_partial_folio()?

When called from mpage_map_and_submit_extent() and jumping to
invalidate_dirty_pages:

fs/ext4/inode.c:mpage_map_and_submit_extent() {
    ...
				if (progress) {
					if (mpage_submit_partial_folio(mpd))
						goto invalidate_dirty_pages;
    ...
}

The call to mpage_submit_partial_folio() can return an error without
releasing the folio reference:

fs/ext4/inode.c:mpage_submit_partial_folio() {
    ...
	folio = filemap_get_folio(inode->i_mapping,
				  mpd->start_pos >> PAGE_SHIFT);
	if (IS_ERR(folio))
		return PTR_ERR(folio);
    ...
	pos = ((loff_t)mpd->map.m_lblk) << inode->i_blkbits;
	if (WARN_ON_ONCE((folio_pos(folio) == pos) ||
			 !folio_contains(folio, pos >> PAGE_SHIFT)))
		return -EINVAL;
    ...
}

If the WARN_ON_ONCE bounds check fails, it returns -EINVAL early without
calling folio_put(folio). Can this prevent the folio from being freed,
potentially causing hangs during unmount, inode eviction, or memory
reclaim?

>  			return err;
>  		}
>  		progress = 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016110311.71391-1-guolingxing@tju.edu.cn?part=1