Re: [PATCH V2] xfs: fix off-by-one-block in xfs_discard_folio()

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Mar 03, 2023 at 10:11:50AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The recent writeback corruption fixes changed the code in
> xfs_discard_folio() to calculate a byte range to for punching
> delalloc extents. A mistake was made in using round_up(pos) for the
> end offset, because when pos points at the first byte of a block, it
> does not get rounded up to point to the end byte of the block. hence
> the punch range is short, and this leads to unexpected behaviour in
> certain cases in xfs_bmap_punch_delalloc_range.
> 
> e.g. pos = 0 means we call xfs_bmap_punch_delalloc_range(0,0), so
> there is no previous extent and it rounds up the punch to the end of
> the delalloc extent it found at offset 0, not the end of the range
> given to xfs_bmap_punch_delalloc_range().
> 
> Fix this by handling the zero block offset case correctly.
> 
> Fixes: 7348b322332d ("xfs: xfs_bmap_punch_delalloc_range() should take a byte range")
> Reported-by: Pengfei Xu <pengfei.xu@intel.com>
> Found-by: Brian Foster <bfoster@redhat.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
> 
> Version 2
> - update xfs_discard_folio() comment to reflect reality
> - simplify the end offset calculation and comment to reflect the
>   fact the punch range always ends at the end of the supplied folio
>   regardless of the position we start the punch from.
> 
>  fs/xfs/xfs_aops.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 41734202796f..2ef78aa1d3f6 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -449,15 +449,17 @@ xfs_prepare_ioend(
>  }
>  
>  /*
> - * If the page has delalloc blocks on it, we need to punch them out before we
> - * invalidate the page.  If we don't, we leave a stale delalloc mapping on the
> - * inode that can trip up a later direct I/O read operation on the same region.
> + * If the folio has delalloc blocks on it, the caller is asking us to punch them
> + * out. If we don't, we can leave a stale delalloc mapping covered by a clean
> + * page that needs to be dirtied again before the delalloc mapping can be
> + * converted. This stale delalloc mapping can trip up a later direct I/O read
> + * operation on the same region.
>   *
> - * We prevent this by truncating away the delalloc regions on the page.  Because
> + * We prevent this by truncating away the delalloc regions on the folio. Because
>   * they are delalloc, we can do this without needing a transaction. Indeed - if
>   * we get ENOSPC errors, we have to be able to do this truncation without a
> - * transaction as there is no space left for block reservation (typically why we
> - * see a ENOSPC in writeback).
> + * transaction as there is no space left for block reservation (typically why
> + * we see a ENOSPC in writeback).
>   */
>  static void
>  xfs_discard_folio(
> @@ -475,8 +477,13 @@ xfs_discard_folio(
>  		"page discard on page "PTR_FMT", inode 0x%llx, pos %llu.",
>  			folio, ip->i_ino, pos);
>  
> +	/*
> +	 * The end of the punch range is always the offset of the the first
> +	 * byte of the next folio. Hence the end offset is only dependent on the
> +	 * folio itself and not the start offset that is passed in.
> +	 */
>  	error = xfs_bmap_punch_delalloc_range(ip, pos,
> -			round_up(pos, folio_size(folio)));
> +				folio_pos(folio) + folio_size(folio));
>  
>  	if (error && !xfs_is_shutdown(mp))
>  		xfs_alert(mp, "page discard unable to remove delalloc mapping.");