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

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

On Thu, May 08, 2025 at 10:30:30AM -0400, Brian Foster wrote:
> 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:

Which warning?  This one?

	/* warn about zeroing folios beyond eof that won't write back */
	WARN_ON_ONCE(folio_pos(folio) > iter->inode->i_size);

If so, then why are there folios that start entirely beyond EOF?

> > 
> >   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).

IIRC XFS calls iomap_zero_range during file extending operations to zero
the tail of a folio that spans EOF, so you'd have to allow for that too.

--D

> 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);
> >  }
> > 
> 
>