| Message ID | 20240923152904.1747117-7-hch@lst.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [01/10] iomap: factor out a iomap_last_written_block helper | expand |
On Mon, Sep 23, 2024 at 05:28:20PM +0200, Christoph Hellwig wrote: > All XFS callers of iomap_zero_range already hold invalidate_lock, so we can't > take it again in iomap_file_buffered_write_punch_delalloc. > > Use the passed in flags argument to detect if we're called from a zeroing > operation and don't take the lock again in this case. Shouldn't this be a part of the previous patch? AFAICT taking the invalidation lock in xfs_file_write_zero_eof is why we need the change to rwsem_assert_held_write here, right? > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_iomap.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c > index 4fa4d66dc37761..0f5fa3de6d3ecc 100644 > --- a/fs/xfs/xfs_iomap.c > +++ b/fs/xfs/xfs_iomap.c > @@ -1239,10 +1239,17 @@ xfs_buffered_write_iomap_end( > if (start_byte >= end_byte) > return 0; > > - filemap_invalidate_lock(inode->i_mapping); > + /* For zeroing operations the callers already hold invalidate_lock. */ > + if (flags & IOMAP_ZERO) > + rwsem_assert_held_write(&inode->i_mapping->invalidate_lock); > + else > + filemap_invalidate_lock(inode->i_mapping); > + > iomap_write_delalloc_release(inode, start_byte, end_byte, flags, iomap, > xfs_buffered_write_delalloc_punch); > - filemap_invalidate_unlock(inode->i_mapping); > + > + if (!(flags & IOMAP_ZERO)) > + filemap_invalidate_unlock(inode->i_mapping); > return 0; > } > > -- > 2.45.2 >
On Mon, Sep 23, 2024 at 09:22:49AM -0700, Darrick J. Wong wrote: > On Mon, Sep 23, 2024 at 05:28:20PM +0200, Christoph Hellwig wrote: > > All XFS callers of iomap_zero_range already hold invalidate_lock, so we can't > > take it again in iomap_file_buffered_write_punch_delalloc. > > > > Use the passed in flags argument to detect if we're called from a zeroing > > operation and don't take the lock again in this case. > > Shouldn't this be a part of the previous patch? AFAICT taking the > invalidation lock in xfs_file_write_zero_eof is why we need the change > to rwsem_assert_held_write here, right? Most callers of zeroing already hold the lock. So I can see arguments for merging the patches now (don't make one case even worse before fixing) or not (this is really two unrelated changes and easier to understand). Or now that the lockig is inside XFS we could even add a private iomap flag to not do the locking for the eof zeroing, but that would create even more special cases.
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c index 4fa4d66dc37761..0f5fa3de6d3ecc 100644 --- a/fs/xfs/xfs_iomap.c +++ b/fs/xfs/xfs_iomap.c @@ -1239,10 +1239,17 @@ xfs_buffered_write_iomap_end( if (start_byte >= end_byte) return 0; - filemap_invalidate_lock(inode->i_mapping); + /* For zeroing operations the callers already hold invalidate_lock. */ + if (flags & IOMAP_ZERO) + rwsem_assert_held_write(&inode->i_mapping->invalidate_lock); + else + filemap_invalidate_lock(inode->i_mapping); + iomap_write_delalloc_release(inode, start_byte, end_byte, flags, iomap, xfs_buffered_write_delalloc_punch); - filemap_invalidate_unlock(inode->i_mapping); + + if (!(flags & IOMAP_ZERO)) + filemap_invalidate_unlock(inode->i_mapping); return 0; }
All XFS callers of iomap_zero_range already hold invalidate_lock, so we can't take it again in iomap_file_buffered_write_punch_delalloc. Use the passed in flags argument to detect if we're called from a zeroing operation and don't take the lock again in this case. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_iomap.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)