Re: [PATCH v3 03/10] ext4: fix stale data if it bail out of the extents mapping loop

[Jan Kara]

On Tue 01-07-25 21:06:28, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 
> During the process of writing back folios, if
> mpage_map_and_submit_extent() exits the extent mapping loop due to an
> ENOSPC or ENOMEM error, it may result in stale data or filesystem
> inconsistency in environments where the block size is smaller than the
> folio size.
> 
> When mapping a discontinuous folio in mpage_map_and_submit_extent(),
> some buffers may have already be mapped. If we exit the mapping loop
> prematurely, the folio data within the mapped range will not be written
> back, and the file's disk size will not be updated. Once the transaction
> that includes this range of extents is committed, this can lead to stale
> data or filesystem inconsistency.
> 
> Fix this by submitting the current processing partially mapped folio.
> 
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

Just one naming suggestion below:

> +/*
> + * This is used to submit mapped buffers in a single folio that is not fully
> + * mapped for various reasons, such as insufficient space or journal credits.
> + */
> +static int mpage_submit_buffers(struct mpage_da_data *mpd)

mpage_submit_buffers() sounds somewhat generic. How about
mpage_submit_partial_folio()?

								Honza

> +{
> +	struct inode *inode = mpd->inode;
> +	struct folio *folio;
> +	loff_t pos;
> +	int ret;
> +
> +	folio = filemap_get_folio(inode->i_mapping,
> +				  mpd->start_pos >> PAGE_SHIFT);
> +	if (IS_ERR(folio))
> +		return PTR_ERR(folio);
> +	/*
> +	 * The mapped position should be within the current processing folio
> +	 * but must not be the folio start position.
> +	 */
> +	pos = mpd->map.m_lblk << inode->i_blkbits;
> +	if (WARN_ON_ONCE((folio_pos(folio) == pos) ||
> +			 !folio_contains(folio, pos >> PAGE_SHIFT)))
> +		return -EINVAL;
> +
> +	ret = mpage_submit_folio(mpd, folio);
> +	if (ret)
> +		goto out;
> +	/*
> +	 * Update start_pos to prevent this folio from being released in
> +	 * mpage_release_unused_pages(), it will be reset to the aligned folio
> +	 * pos when this folio is written again in the next round. Additionally,
> +	 * do not update wbc->nr_to_write here, as it will be updated once the
> +	 * entire folio has finished processing.
> +	 */
> +	mpd->start_pos = pos;
> +out:
> +	folio_unlock(folio);
> +	folio_put(folio);
> +	return ret;
> +}
> +
>  /*
>   * mpage_map_and_submit_extent - map extent starting at mpd->lblk of length
>   *				 mpd->len and submit pages underlying it for IO
> @@ -2411,8 +2452,16 @@ static int mpage_map_and_submit_extent(handle_t *handle,
>  			 */
>  			if ((err == -ENOMEM) ||
>  			    (err == -ENOSPC && ext4_count_free_clusters(sb))) {
> -				if (progress)
> +				/*
> +				 * We may have already allocated extents for
> +				 * some bhs inside the folio, issue the
> +				 * corresponding data to prevent stale data.
> +				 */
> +				if (progress) {
> +					if (mpage_submit_buffers(mpd))
> +						goto invalidate_dirty_pages;
>  					goto update_disksize;
> +				}
>  				return err;
>  			}
>  			ext4_msg(sb, KERN_CRIT,
> -- 
> 2.46.1
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR