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

[Jan Kara]

On Fri 16-10-26 11:03:11, guolingxing wrote:
> 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@xxxxxxxxxxxxxxx
> Signed-off-by: guolingxing <guolingxing@xxxxxxxxxx>

I agree this is a bug but the kernel will complain very loudly to the log
(as this generally means filesystem corruption) so the practical impact of
this is very low. With that reservation feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> 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
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR