Re: [RFC] gfs2: Do not call iomap_zero_range beyond eof

[Brian Foster <bfoster@redhat.com>]

On Thu, May 08, 2025 at 03:34:27PM +0200, Andreas Gruenbacher wrote:
> Since commit eb65540aa9fc ("iomap: warn on zero range of a post-eof
> folio"), iomap_zero_range() warns when asked to zero a folio beyond eof.
> The warning triggers on the following code path:
> 
>   gfs2_fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)
>     __gfs2_punch_hole()
>       gfs2_block_zero_range()
>         iomap_zero_range()
> 
> So far, gfs2 is just zeroing out partial pages at the beginning and end
> of the range, whether beyond eof or not.  The data beyond eof is already
> expected to be all zeroes, though.  Truncate the range passed to
> iomap_zero_range().
> 
> As an alternative approach, we could also implicitly truncate the range
> inside iomap_zero_range() instead of issuing a warning.  Any thoughts?
> 

Thanks Andreas. The more I think about this the more it seems like
lifting this logic into iomap is a reasonable compromise between just
dropping the warning and forcing individual filesystems to work around
it. The original intent of the warning was to have something to catch
subtle bad behavior since zero range did update i_size for so long.

OTOH I think it's reasonable to argue that we shouldn't need to warn in
situations where we could just enforce correct behavior. Also, I believe
we introduced something similar to avoid post-eof weirdness wrt unshare
range [1], so precedent exists.

I'm interested if others have opinions on the iomap side.. (though as I
write this it looks like hch sits on the side of not tweaking iomap).

Brian

[1] a311a08a4237 ("iomap: constrain the file range passed to iomap_file_unshare")

> Thanks,
> Andreas
> 
> --
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> 
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index b81984def58e..d9a4309cd414 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -1301,6 +1301,10 @@ static int gfs2_block_zero_range(struct inode *inode, loff_t from,
>  				 unsigned int length)
>  {
>  	BUG_ON(current->journal_info);
> +	if (from > inode->i_size)
> +		return 0;
> +	if (from + length > inode->i_size)
> +		length = inode->i_size - from;
>  	return iomap_zero_range(inode, from, length, NULL, &gfs2_iomap_ops,
>  			NULL);
>  }
>