On Fri, Mar 03, 2023 at 10:11:50AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Found-by: Brian Foster <bfoster@xxxxxxxxxx>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
Looks good,
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--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.");
