| Message ID | 20240827051028.1751933-10-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/10] iomap: handle a post-direct I/O invalidate race in iomap_write_delalloc_release | expand |
On Tue, Aug 27, 2024 at 07:09:56AM +0200, Christoph Hellwig wrote: > Change to always set xfs_buffered_write_iomap_begin for COW fork > allocations even if they don't overlap existing data fork extents, > which will allow the iomap_end callback to detect if it has to punch > stale delalloc blocks from the COW fork instead of the data fork. It > also means we sample the sequence counter for both the data and the COW > fork when writing to the COW fork, which ensures we properly revalidate > when only COW fork changes happens. > > This is essentially a revert of commit 72a048c1056a ("xfs: only set > IOMAP_F_SHARED when providing a srcmap to a write"). This is fine because > the problem that the commit fixed has now been dealt with in iomap by > only looking at the actual srcmap and not the fallback to the write > iomap. > > Note that the direct I/O path was never changed and has always set > IOMAP_F_SHARED for all COW fork allocations. > > Signed-off-by: Christoph Hellwig <hch@lst.de> That was fun to compare your revert vs. a patch from 3 years ago. $) Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_iomap.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c > index e0dc6393686c01..22e9613a995f12 100644 > --- a/fs/xfs/xfs_iomap.c > +++ b/fs/xfs/xfs_iomap.c > @@ -1186,20 +1186,19 @@ xfs_buffered_write_iomap_begin( > return 0; > > found_cow: > - seq = xfs_iomap_inode_sequence(ip, 0); > if (imap.br_startoff <= offset_fsb) { > - error = xfs_bmbt_to_iomap(ip, srcmap, &imap, flags, 0, seq); > + error = xfs_bmbt_to_iomap(ip, srcmap, &imap, flags, 0, > + xfs_iomap_inode_sequence(ip, 0)); > if (error) > goto out_unlock; > - seq = xfs_iomap_inode_sequence(ip, IOMAP_F_SHARED); > - xfs_iunlock(ip, lockmode); > - return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, > - IOMAP_F_SHARED, seq); > + } else { > + xfs_trim_extent(&cmap, offset_fsb, imap.br_startoff - offset_fsb); > } > > - xfs_trim_extent(&cmap, offset_fsb, imap.br_startoff - offset_fsb); > + iomap_flags = IOMAP_F_SHARED; > + seq = xfs_iomap_inode_sequence(ip, iomap_flags); > xfs_iunlock(ip, lockmode); > - return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, 0, seq); > + return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, iomap_flags, seq); > > out_unlock: > xfs_iunlock(ip, lockmode); > -- > 2.43.0 > >
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c index e0dc6393686c01..22e9613a995f12 100644 --- a/fs/xfs/xfs_iomap.c +++ b/fs/xfs/xfs_iomap.c @@ -1186,20 +1186,19 @@ xfs_buffered_write_iomap_begin( return 0; found_cow: - seq = xfs_iomap_inode_sequence(ip, 0); if (imap.br_startoff <= offset_fsb) { - error = xfs_bmbt_to_iomap(ip, srcmap, &imap, flags, 0, seq); + error = xfs_bmbt_to_iomap(ip, srcmap, &imap, flags, 0, + xfs_iomap_inode_sequence(ip, 0)); if (error) goto out_unlock; - seq = xfs_iomap_inode_sequence(ip, IOMAP_F_SHARED); - xfs_iunlock(ip, lockmode); - return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, - IOMAP_F_SHARED, seq); + } else { + xfs_trim_extent(&cmap, offset_fsb, imap.br_startoff - offset_fsb); } - xfs_trim_extent(&cmap, offset_fsb, imap.br_startoff - offset_fsb); + iomap_flags = IOMAP_F_SHARED; + seq = xfs_iomap_inode_sequence(ip, iomap_flags); xfs_iunlock(ip, lockmode); - return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, 0, seq); + return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, iomap_flags, seq); out_unlock: xfs_iunlock(ip, lockmode);
Change to always set xfs_buffered_write_iomap_begin for COW fork allocations even if they don't overlap existing data fork extents, which will allow the iomap_end callback to detect if it has to punch stale delalloc blocks from the COW fork instead of the data fork. It also means we sample the sequence counter for both the data and the COW fork when writing to the COW fork, which ensures we properly revalidate when only COW fork changes happens. This is essentially a revert of commit 72a048c1056a ("xfs: only set IOMAP_F_SHARED when providing a srcmap to a write"). This is fine because the problem that the commit fixed has now been dealt with in iomap by only looking at the actual srcmap and not the fallback to the write iomap. Note that the direct I/O path was never changed and has always set IOMAP_F_SHARED for all COW fork allocations. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_iomap.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)