[v2] iomap: warn on zero range of a post-eof folio

Commit Message

Brian Foster Nov. 15, 2024, 2:59 p.m. UTC

iomap_zero_range() uses buffered writes for manual zeroing, no
longer updates i_size for such writes, but is still explicitly
called for post-eof ranges. The historical use case for this is
zeroing post-eof speculative preallocation on extending writes from
XFS. However, XFS also recently changed to convert all post-eof
delalloc mappings to unwritten in the iomap_begin() handler, which
means it now never expects manual zeroing of post-eof mappings. In
other words, all post-eof mappings should be reported as holes or
unwritten.

This is a subtle dependency that can be hard to detect if violated
because associated codepaths are likely to update i_size after folio
locks are dropped, but before writeback happens to occur. For
example, if XFS reverts back to some form of manual zeroing of
post-eof blocks on write extension, writeback of those zeroed folios
will now race with the presumed i_size update from the subsequent
buffered write.

Since iomap_zero_range() can't correctly zero post-eof mappings
beyond EOF without updating i_size, warn if this ever occurs. This
serves as minimal indication that if this use case is reintroduced
by a filesystem, iomap_zero_range() might need to reconsider i_size
updates for write extending use cases.

Signed-off-by: Brian Foster <[e-mail]>
Reviewed-by: Christoph Hellwig <[e-mail]>
---

v2:
- Dropped unnecessary local var.
v1: https://lore.kernel.org/linux-fsdevel/[e-mail]/

 fs/iomap/buffered-io.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Darrick J. Wong Nov. 15, 2024, 4:09 p.m. UTC | #1

On Fri, Nov 15, 2024 at 09:59:31AM -0500, Brian Foster wrote:
> iomap_zero_range() uses buffered writes for manual zeroing, no
> longer updates i_size for such writes, but is still explicitly
> called for post-eof ranges. The historical use case for this is
> zeroing post-eof speculative preallocation on extending writes from
> XFS. However, XFS also recently changed to convert all post-eof
> delalloc mappings to unwritten in the iomap_begin() handler, which
> means it now never expects manual zeroing of post-eof mappings. In
> other words, all post-eof mappings should be reported as holes or
> unwritten.
> 
> This is a subtle dependency that can be hard to detect if violated
> because associated codepaths are likely to update i_size after folio
> locks are dropped, but before writeback happens to occur. For
> example, if XFS reverts back to some form of manual zeroing of
> post-eof blocks on write extension, writeback of those zeroed folios
> will now race with the presumed i_size update from the subsequent
> buffered write.
> 
> Since iomap_zero_range() can't correctly zero post-eof mappings
> beyond EOF without updating i_size, warn if this ever occurs. This
> serves as minimal indication that if this use case is reintroduced
> by a filesystem, iomap_zero_range() might need to reconsider i_size
> updates for write extending use cases.
> 
> Signed-off-by: Brian Foster <[e-mail]>
> Reviewed-by: Christoph Hellwig <[e-mail]>

Looks fine to me now,
Reviewed-by: "Darrick J. Wong" <[e-mail]>

--D

> ---
> 
> v2:
> - Dropped unnecessary local var.
> v1: https://lore.kernel.org/linux-fsdevel/[e-mail]/
> 
>  fs/iomap/buffered-io.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index af2f59817779..25fbb541032a 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -1369,6 +1369,8 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  		if (iter->iomap.flags & IOMAP_F_STALE)
>  			break;
>  
> +		/* warn about zeroing folios beyond eof that won't write back */
> +		WARN_ON_ONCE(folio_pos(folio) > iter->inode->i_size);
>  		offset = offset_in_folio(folio, pos);
>  		if (bytes > folio_size(folio) - offset)
>  			bytes = folio_size(folio) - offset;
> -- 
> 2.47.0
> 
>

Christian Brauner Nov. 20, 2024, 8:24 a.m. UTC | #2

On Fri, 15 Nov 2024 09:59:31 -0500, Brian Foster wrote:
> iomap_zero_range() uses buffered writes for manual zeroing, no
> longer updates i_size for such writes, but is still explicitly
> called for post-eof ranges. The historical use case for this is
> zeroing post-eof speculative preallocation on extending writes from
> XFS. However, XFS also recently changed to convert all post-eof
> delalloc mappings to unwritten in the iomap_begin() handler, which
> means it now never expects manual zeroing of post-eof mappings. In
> other words, all post-eof mappings should be reported as holes or
> unwritten.
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] iomap: warn on zero range of a post-eof folio
      https://git.kernel.org/vfs/vfs/c/2eba5b6013a5

Patch

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index af2f59817779..25fbb541032a 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1369,6 +1369,8 @@  static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 		if (iter->iomap.flags & IOMAP_F_STALE)
 			break;
 
+		/* warn about zeroing folios beyond eof that won't write back */
+		WARN_ON_ONCE(folio_pos(folio) > iter->inode->i_size);
 		offset = offset_in_folio(folio, pos);
 		if (bytes > folio_size(folio) - offset)
 			bytes = folio_size(folio) - offset;