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

[sashiko-bot@kernel.org]

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