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

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

On Wed, Mar 01, 2023 at 11:17:06AM +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>
> ---
>  fs/xfs/xfs_aops.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 41734202796f..429f63cfd7d4 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -466,6 +466,7 @@ xfs_discard_folio(
>  {
>  	struct xfs_inode	*ip = XFS_I(folio->mapping->host);
>  	struct xfs_mount	*mp = ip->i_mount;
> +	xfs_off_t		end_off;
>  	int			error;
>  
>  	if (xfs_is_shutdown(mp))
> @@ -475,8 +476,17 @@ xfs_discard_folio(
>  		"page discard on page "PTR_FMT", inode 0x%llx, pos %llu.",
>  			folio, ip->i_ino, pos);
>  
> -	error = xfs_bmap_punch_delalloc_range(ip, pos,
> -			round_up(pos, folio_size(folio)));
> +	/*
> +	 * Need to be careful with the case where the pos passed in points to
> +	 * the first byte of the folio - rounding up won't change the value,
> +	 * but in all cases here we need to end offset to point to the start
> +	 * of the next folio.
> +	 */
> +	if (pos == folio_pos(folio))
> +		end_off = pos + folio_size(folio);
> +	else
> +		end_off = round_up(pos, folio_size(folio));

Can this construct be simplified to:

	end_off = round_up(pos + 1, folio_size(folio));

If pos is the first byte of the folio, it'll round end_off to the start
of the next folio.  If pos is (somehow) the last byte of the folio, the
first argument to round_up is already the first byte of the next folio,
and rounding won't change it.

I'll run this through fstests since it's getting late already.

--D

> +	error = xfs_bmap_punch_delalloc_range(ip, pos, end_off);
>  
>  	if (error && !xfs_is_shutdown(mp))
>  		xfs_alert(mp, "page discard unable to remove delalloc mapping.");
> -- 
> 2.39.2
>