diff mbox series

[1/7] xfs: write page faults in iomap are not buffered writes

Message ID 20221101003412.3842572-2-david@fromorbit.com (mailing list archive)
State New, archived
Headers show
Series [1/7] xfs: write page faults in iomap are not buffered writes | expand

Commit Message

Dave Chinner Nov. 1, 2022, 12:34 a.m. UTC 
From: Dave Chinner <dchinner@redhat.com>

When we reserve a delalloc region in xfs_buffered_write_iomap_begin,
we mark the iomap as IOMAP_F_NEW so that the the write context
understands that it allocated the delalloc region.

If we then fail that buffered write, xfs_buffered_write_iomap_end()
checks for the IOMAP_F_NEW flag and if it is set, it punches out
the unused delalloc region that was allocated for the write.

The assumption this code makes is that all buffered write operations
that can allocate space are run under an exclusive lock (i_rwsem).
This is an invalid assumption: page faults in mmap()d regions call
through this same function pair to map the file range being faulted
and this runs only holding the inode->i_mapping->invalidate_lock in
shared mode.

IOWs, we can have races between page faults and write() calls that
fail the nested page cache write operation that result in data loss.
That is, the failing iomap_end call will punch out the data that
the other racing iomap iteration brought into the page cache. This
can be reproduced with generic/34[46] if we arbitrarily fail page
cache copy-in operations from write() syscalls.

Code analysis tells us that the iomap_page_mkwrite() function holds
the already instantiated and uptodate folio locked across the iomap
mapping iterations. Hence the folio cannot be removed from memory
whilst we are mapping the range it covers, and as such we do not
care if the mapping changes state underneath the iomap iteration
loop:

1. if the folio is not already dirty, there is no writeback races
   possible.
2. if we allocated the mapping (delalloc or unwritten), the folio
   cannot already be dirty. See #1.
3. If the folio is already dirty, it must be up to date. As we hold
   it locked, it cannot be reclaimed from memory. Hence we always
   have valid data in the page cache while iterating the mapping.
4. Valid data in the page cache can exist when the underlying
   mapping is DELALLOC, UNWRITTEN or WRITTEN. Having the mapping
   change from DELALLOC->UNWRITTEN or UNWRITTEN->WRITTEN does not
   change the data in the page - it only affects actions if we are
   initialising a new page. Hence #3 applies  and we don't care
   about these extent map transitions racing with
   iomap_page_mkwrite().
5. iomap_page_mkwrite() checks for page invalidation races
   (truncate, hole punch, etc) after it locks the folio. We also
   hold the mapping->invalidation_lock here, and hence the mapping
   cannot change due to extent removal operations while we are
   iterating the folio.

As such, filesystems that don't use bufferheads will never fail
the iomap_folio_mkwrite_iter() operation on the current mapping,
regardless of whether the iomap should be considered stale.

Further, the range we are asked to iterate is limited to the range
inside EOF that the folio spans. Hence, for XFS, we will only map
the exact range we are asked for, and we will only do speculative
preallocation with delalloc if we are mapping a hole at the EOF
page. The iterator will consume the entire range of the folio that
is within EOF, and anything beyond the EOF block cannot be accessed.
We never need to truncate this post-EOF speculative prealloc away in
the context of the iomap_page_mkwrite() iterator because if it
remains unused we'll remove it when the last reference to the inode
goes away.

Hence we don't actually need an .iomap_end() cleanup/error handling
path at all for iomap_page_mkwrite() for XFS. This means we can
separate the page fault processing from the complexity of the
.iomap_end() processing in the buffered write path. This also means
that the buffered write path will also be able to take the
mapping->invalidate_lock as necessary.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_file.c  | 2 +-
 fs/xfs/xfs_iomap.c | 9 +++++++++
 fs/xfs/xfs_iomap.h | 1 +
 3 files changed, 11 insertions(+), 1 deletion(-)

Comments

Christoph Hellwig Nov. 2, 2022, 7:17 a.m. UTC | #1 
The analsysi and fix look correct, but I wonder if it wouldn't be better
if we don't expand the number of iomap ops even further and just check
for IOMAP_FAULT to not be set in flags in xfs_buffered_write_iomap_end.

Either way:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Nov. 2, 2022, 4:12 p.m. UTC | #2 
On Tue, Nov 01, 2022 at 11:34:06AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When we reserve a delalloc region in xfs_buffered_write_iomap_begin,
> we mark the iomap as IOMAP_F_NEW so that the the write context
> understands that it allocated the delalloc region.
> 
> If we then fail that buffered write, xfs_buffered_write_iomap_end()
> checks for the IOMAP_F_NEW flag and if it is set, it punches out
> the unused delalloc region that was allocated for the write.
> 
> The assumption this code makes is that all buffered write operations
> that can allocate space are run under an exclusive lock (i_rwsem).
> This is an invalid assumption: page faults in mmap()d regions call
> through this same function pair to map the file range being faulted
> and this runs only holding the inode->i_mapping->invalidate_lock in
> shared mode.
> 
> IOWs, we can have races between page faults and write() calls that
> fail the nested page cache write operation that result in data loss.
> That is, the failing iomap_end call will punch out the data that
> the other racing iomap iteration brought into the page cache. This
> can be reproduced with generic/34[46] if we arbitrarily fail page
> cache copy-in operations from write() syscalls.
> 
> Code analysis tells us that the iomap_page_mkwrite() function holds
> the already instantiated and uptodate folio locked across the iomap
> mapping iterations. Hence the folio cannot be removed from memory
> whilst we are mapping the range it covers, and as such we do not
> care if the mapping changes state underneath the iomap iteration
> loop:
> 
> 1. if the folio is not already dirty, there is no writeback races
>    possible.
> 2. if we allocated the mapping (delalloc or unwritten), the folio
>    cannot already be dirty. See #1.
> 3. If the folio is already dirty, it must be up to date. As we hold
>    it locked, it cannot be reclaimed from memory. Hence we always
>    have valid data in the page cache while iterating the mapping.
> 4. Valid data in the page cache can exist when the underlying
>    mapping is DELALLOC, UNWRITTEN or WRITTEN. Having the mapping
>    change from DELALLOC->UNWRITTEN or UNWRITTEN->WRITTEN does not
>    change the data in the page - it only affects actions if we are
>    initialising a new page. Hence #3 applies  and we don't care
>    about these extent map transitions racing with
>    iomap_page_mkwrite().
> 5. iomap_page_mkwrite() checks for page invalidation races
>    (truncate, hole punch, etc) after it locks the folio. We also
>    hold the mapping->invalidation_lock here, and hence the mapping
>    cannot change due to extent removal operations while we are
>    iterating the folio.
> 
> As such, filesystems that don't use bufferheads will never fail
> the iomap_folio_mkwrite_iter() operation on the current mapping,
> regardless of whether the iomap should be considered stale.
> 
> Further, the range we are asked to iterate is limited to the range
> inside EOF that the folio spans. Hence, for XFS, we will only map
> the exact range we are asked for, and we will only do speculative
> preallocation with delalloc if we are mapping a hole at the EOF
> page. The iterator will consume the entire range of the folio that
> is within EOF, and anything beyond the EOF block cannot be accessed.
> We never need to truncate this post-EOF speculative prealloc away in
> the context of the iomap_page_mkwrite() iterator because if it
> remains unused we'll remove it when the last reference to the inode
> goes away.

Why /do/ we need to trim the delalloc reservations after a failed
write(), anyway?  I gather it's because we don't want to end up with a
clean page backed by a delalloc reservation because writeback will never
get run to convert that reservation into real space, which means we've
leaked the reservation until someone dirties the page?

Ah.  Inode eviction also complains about inodes with delalloc
reservations.  The blockgc code only touches cow fork mappings and
post-eof blocks, which means it doesn't look for these dead/orphaned
delalloc reservations either.

But you're right that (non-bh) page_mkwrite won't ever fail, so
->iomap_end isn't necessary.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> Hence we don't actually need an .iomap_end() cleanup/error handling
> path at all for iomap_page_mkwrite() for XFS. This means we can
> separate the page fault processing from the complexity of the
> .iomap_end() processing in the buffered write path. This also means
> that the buffered write path will also be able to take the
> mapping->invalidate_lock as necessary.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_file.c  | 2 +-
>  fs/xfs/xfs_iomap.c | 9 +++++++++
>  fs/xfs/xfs_iomap.h | 1 +
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index c6c80265c0b2..fee471ca9737 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1324,7 +1324,7 @@ __xfs_filemap_fault(
>  		if (write_fault) {
>  			xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
>  			ret = iomap_page_mkwrite(vmf,
> -					&xfs_buffered_write_iomap_ops);
> +					&xfs_page_mkwrite_iomap_ops);
>  			xfs_iunlock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
>  		} else {
>  			ret = filemap_fault(vmf);
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 07da03976ec1..5cea069a38b4 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -1187,6 +1187,15 @@ const struct iomap_ops xfs_buffered_write_iomap_ops = {
>  	.iomap_end		= xfs_buffered_write_iomap_end,
>  };
>  
> +/*
> + * iomap_page_mkwrite() will never fail in a way that requires delalloc extents
> + * that it allocated to be revoked. Hence we do not need an .iomap_end method
> + * for this operation.
> + */
> +const struct iomap_ops xfs_page_mkwrite_iomap_ops = {
> +	.iomap_begin		= xfs_buffered_write_iomap_begin,
> +};
> +
>  static int
>  xfs_read_iomap_begin(
>  	struct inode		*inode,
> diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
> index c782e8c0479c..0f62ab633040 100644
> --- a/fs/xfs/xfs_iomap.h
> +++ b/fs/xfs/xfs_iomap.h
> @@ -47,6 +47,7 @@ xfs_aligned_fsb_count(
>  }
>  
>  extern const struct iomap_ops xfs_buffered_write_iomap_ops;
> +extern const struct iomap_ops xfs_page_mkwrite_iomap_ops;
>  extern const struct iomap_ops xfs_direct_write_iomap_ops;
>  extern const struct iomap_ops xfs_read_iomap_ops;
>  extern const struct iomap_ops xfs_seek_iomap_ops;
> -- 
> 2.37.2
>
Dave Chinner Nov. 2, 2022, 9:11 p.m. UTC | #3 
On Wed, Nov 02, 2022 at 09:12:05AM -0700, Darrick J. Wong wrote:
> On Tue, Nov 01, 2022 at 11:34:06AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > When we reserve a delalloc region in xfs_buffered_write_iomap_begin,
> > we mark the iomap as IOMAP_F_NEW so that the the write context
> > understands that it allocated the delalloc region.
> > 
> > If we then fail that buffered write, xfs_buffered_write_iomap_end()
> > checks for the IOMAP_F_NEW flag and if it is set, it punches out
> > the unused delalloc region that was allocated for the write.
> > 
> > The assumption this code makes is that all buffered write operations
> > that can allocate space are run under an exclusive lock (i_rwsem).
> > This is an invalid assumption: page faults in mmap()d regions call
> > through this same function pair to map the file range being faulted
> > and this runs only holding the inode->i_mapping->invalidate_lock in
> > shared mode.
> > 
> > IOWs, we can have races between page faults and write() calls that
> > fail the nested page cache write operation that result in data loss.
> > That is, the failing iomap_end call will punch out the data that
> > the other racing iomap iteration brought into the page cache. This
> > can be reproduced with generic/34[46] if we arbitrarily fail page
> > cache copy-in operations from write() syscalls.
> > 
> > Code analysis tells us that the iomap_page_mkwrite() function holds
> > the already instantiated and uptodate folio locked across the iomap
> > mapping iterations. Hence the folio cannot be removed from memory
> > whilst we are mapping the range it covers, and as such we do not
> > care if the mapping changes state underneath the iomap iteration
> > loop:
> > 
> > 1. if the folio is not already dirty, there is no writeback races
> >    possible.
> > 2. if we allocated the mapping (delalloc or unwritten), the folio
> >    cannot already be dirty. See #1.
> > 3. If the folio is already dirty, it must be up to date. As we hold
> >    it locked, it cannot be reclaimed from memory. Hence we always
> >    have valid data in the page cache while iterating the mapping.
> > 4. Valid data in the page cache can exist when the underlying
> >    mapping is DELALLOC, UNWRITTEN or WRITTEN. Having the mapping
> >    change from DELALLOC->UNWRITTEN or UNWRITTEN->WRITTEN does not
> >    change the data in the page - it only affects actions if we are
> >    initialising a new page. Hence #3 applies  and we don't care
> >    about these extent map transitions racing with
> >    iomap_page_mkwrite().
> > 5. iomap_page_mkwrite() checks for page invalidation races
> >    (truncate, hole punch, etc) after it locks the folio. We also
> >    hold the mapping->invalidation_lock here, and hence the mapping
> >    cannot change due to extent removal operations while we are
> >    iterating the folio.
> > 
> > As such, filesystems that don't use bufferheads will never fail
> > the iomap_folio_mkwrite_iter() operation on the current mapping,
> > regardless of whether the iomap should be considered stale.
> > 
> > Further, the range we are asked to iterate is limited to the range
> > inside EOF that the folio spans. Hence, for XFS, we will only map
> > the exact range we are asked for, and we will only do speculative
> > preallocation with delalloc if we are mapping a hole at the EOF
> > page. The iterator will consume the entire range of the folio that
> > is within EOF, and anything beyond the EOF block cannot be accessed.
> > We never need to truncate this post-EOF speculative prealloc away in
> > the context of the iomap_page_mkwrite() iterator because if it
> > remains unused we'll remove it when the last reference to the inode
> > goes away.
> 
> Why /do/ we need to trim the delalloc reservations after a failed
> write(), anyway?  I gather it's because we don't want to end up with a
> clean page backed by a delalloc reservation because writeback will never
> get run to convert that reservation into real space, which means we've
> leaked the reservation until someone dirties the page?

Yup, we leak the delalloc reservation and extent until the inode is
evicted from cache. What happens when reads/page faults happen over
that page from that point is now an interesting question - it might
all work correctly, but then again it might not...

> Ah.  Inode eviction also complains about inodes with delalloc
> reservations.  The blockgc code only touches cow fork mappings and
> post-eof blocks, which means it doesn't look for these dead/orphaned
> delalloc reservations either.

Yup, the inode eviction code that detects leaked delalloc blocks is
a useful canary - if it fires, it tends to indicate a bug in the
allocation/free code somewhere. If we allowed leaking delalloc
extents, we lose that canary.

And, yes, we'd need to make xfs_free_eofblocks() now search for
leaked delalloc blocks within EOF and punch them out, but then it
would have to either do the "search page cache for dirty pages" walk
or force writeback first.

None of these things feel like an improvement to me.

-Dave.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index c6c80265c0b2..fee471ca9737 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1324,7 +1324,7 @@  __xfs_filemap_fault(
 		if (write_fault) {
 			xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
 			ret = iomap_page_mkwrite(vmf,
-					&xfs_buffered_write_iomap_ops);
+					&xfs_page_mkwrite_iomap_ops);
 			xfs_iunlock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
 		} else {
 			ret = filemap_fault(vmf);
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 07da03976ec1..5cea069a38b4 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1187,6 +1187,15 @@  const struct iomap_ops xfs_buffered_write_iomap_ops = {
 	.iomap_end		= xfs_buffered_write_iomap_end,
 };
 
+/*
+ * iomap_page_mkwrite() will never fail in a way that requires delalloc extents
+ * that it allocated to be revoked. Hence we do not need an .iomap_end method
+ * for this operation.
+ */
+const struct iomap_ops xfs_page_mkwrite_iomap_ops = {
+	.iomap_begin		= xfs_buffered_write_iomap_begin,
+};
+
 static int
 xfs_read_iomap_begin(
 	struct inode		*inode,
diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
index c782e8c0479c..0f62ab633040 100644
--- a/fs/xfs/xfs_iomap.h
+++ b/fs/xfs/xfs_iomap.h
@@ -47,6 +47,7 @@  xfs_aligned_fsb_count(
 }
 
 extern const struct iomap_ops xfs_buffered_write_iomap_ops;
+extern const struct iomap_ops xfs_page_mkwrite_iomap_ops;
 extern const struct iomap_ops xfs_direct_write_iomap_ops;
 extern const struct iomap_ops xfs_read_iomap_ops;
 extern const struct iomap_ops xfs_seek_iomap_ops;