diff mbox series

[06/10] xfs: zeroing already holds invalidate_lock

Message ID 20240923152904.1747117-7-hch@lst.de (mailing list archive)
State New
Headers show
Series [01/10] iomap: factor out a iomap_last_written_block helper | expand

Commit Message

Christoph Hellwig Sept. 23, 2024, 3:28 p.m. UTC
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(-)

Comments

Darrick J. Wong Sept. 23, 2024, 4:22 p.m. UTC | #1
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
>
Christoph Hellwig Sept. 24, 2024, 5:44 a.m. UTC | #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 mbox series

Patch

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