improve sub-block size direct I/O concurrency

improve sub-block size direct I/O concurrency

[Christoph Hellwig]

Hi all,

this series avoids taking the iolock exclusively for direct I/O
writes that are not file system block size aligned, but also do
not require allocations or unwritten extent conversion.

[PATCH 1/3] xfs: factor out a xfs_ilock_iocb helper

[Christoph Hellwig]

Add a helper to factor out the nowait locking logical for the read/write
helpers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c | 55 +++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index ba02780dee6439..aa3fe89628f0f1 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -217,6 +217,23 @@ xfs_file_fsync(
 	return error;
 }
 
+static int
+xfs_ilock_iocb(
+	struct kiocb		*iocb,
+	unsigned int		lock_mode)
+{
+	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
+
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!xfs_ilock_nowait(ip, lock_mode))
+			return -EAGAIN;
+	} else {
+		xfs_ilock(ip, lock_mode);
+	}
+
+	return 0;
+}
+
 STATIC ssize_t
 xfs_file_dio_aio_read(
 	struct kiocb		*iocb,
@@ -233,12 +250,9 @@ xfs_file_dio_aio_read(
 
 	file_accessed(iocb->ki_filp);
 
-	if (iocb->ki_flags & IOCB_NOWAIT) {
-		if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED))
-			return -EAGAIN;
-	} else {
-		xfs_ilock(ip, XFS_IOLOCK_SHARED);
-	}
+	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
+	if (ret)
+		return ret;
 	ret = iomap_dio_rw(iocb, to, &xfs_read_iomap_ops, NULL,
 			is_sync_kiocb(iocb));
 	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
@@ -260,13 +274,9 @@ xfs_file_dax_read(
 	if (!count)
 		return 0; /* skip atime */
 
-	if (iocb->ki_flags & IOCB_NOWAIT) {
-		if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED))
-			return -EAGAIN;
-	} else {
-		xfs_ilock(ip, XFS_IOLOCK_SHARED);
-	}
-
+	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
+	if (ret)
+		return ret;
 	ret = dax_iomap_rw(iocb, to, &xfs_read_iomap_ops);
 	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
 
@@ -284,12 +294,9 @@ xfs_file_buffered_aio_read(
 
 	trace_xfs_file_buffered_read(ip, iov_iter_count(to), iocb->ki_pos);
 
-	if (iocb->ki_flags & IOCB_NOWAIT) {
-		if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED))
-			return -EAGAIN;
-	} else {
-		xfs_ilock(ip, XFS_IOLOCK_SHARED);
-	}
+	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
+	if (ret)
+		return ret;
 	ret = generic_file_read_iter(iocb, to);
 	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
 
@@ -628,13 +635,9 @@ xfs_file_dax_write(
 	size_t			count;
 	loff_t			pos;
 
-	if (iocb->ki_flags & IOCB_NOWAIT) {
-		if (!xfs_ilock_nowait(ip, iolock))
-			return -EAGAIN;
-	} else {
-		xfs_ilock(ip, iolock);
-	}
-
+	ret = xfs_ilock_iocb(iocb, iolock);
+	if (ret)
+		return ret;
 	ret = xfs_file_aio_write_checks(iocb, from, &iolock);
 	if (ret)
 		goto out;
-- 
2.29.2

[PATCH 2/3] xfs: make xfs_file_aio_write_checks IOCB_NOWAIT-aware

[Christoph Hellwig]

Ensure we don't block on the iolock, or waiting for I/O in
xfs_file_aio_write_checks if the caller asked to avoid that.

Fixes: 29a5d29ec181 ("xfs: nowait aio support")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index aa3fe89628f0f1..1470fc4f2e0255 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -355,7 +355,14 @@ xfs_file_aio_write_checks(
 	if (error <= 0)
 		return error;
 
-	error = xfs_break_layouts(inode, iolock, BREAK_WRITE);
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		error = break_layout(inode, false);
+		if (error == -EWOULDBLOCK)
+			error = -EAGAIN;
+	} else {
+		error = xfs_break_layouts(inode, iolock, BREAK_WRITE);
+	}
+
 	if (error)
 		return error;
 
@@ -366,7 +373,11 @@ xfs_file_aio_write_checks(
 	if (*iolock == XFS_IOLOCK_SHARED && !IS_NOSEC(inode)) {
 		xfs_iunlock(ip, *iolock);
 		*iolock = XFS_IOLOCK_EXCL;
-		xfs_ilock(ip, *iolock);
+		error = xfs_ilock_iocb(iocb, *iolock);
+		if (error) {
+			*iolock = 0;
+			return error;
+		}
 		goto restart;
 	}
 	/*
@@ -388,6 +399,10 @@ xfs_file_aio_write_checks(
 	isize = i_size_read(inode);
 	if (iocb->ki_pos > isize) {
 		spin_unlock(&ip->i_flags_lock);
+
+		if (iocb->ki_flags & IOCB_NOWAIT)
+			return -EAGAIN;
+
 		if (!drained_dio) {
 			if (*iolock == XFS_IOLOCK_SHARED) {
 				xfs_iunlock(ip, *iolock);
@@ -613,7 +628,8 @@ xfs_file_dio_aio_write(
 			   &xfs_dio_write_ops,
 			   is_sync_kiocb(iocb) || unaligned_io);
 out:
-	xfs_iunlock(ip, iolock);
+	if (iolock)
+		xfs_iunlock(ip, iolock);
 
 	/*
 	 * No fallback to buffered IO after short writes for XFS, direct I/O
@@ -652,7 +668,8 @@ xfs_file_dax_write(
 		error = xfs_setfilesize(ip, pos, ret);
 	}
 out:
-	xfs_iunlock(ip, iolock);
+	if (iolock)
+		xfs_iunlock(ip, iolock);
 	if (error)
 		return error;
 
-- 
2.29.2

[PATCH 3/3] xfs: try to avoid the iolock exclusive for non-aligned direct writes

[Christoph Hellwig]

We only need the exclusive iolock for direct writes to protect sub-block
zeroing after an allocation or conversion of unwritten extents, and the
synchronous execution of these writes is also only needed because the
iolock is dropped early for the dodgy i_dio_count synchronisation.

Always start out with the shared iolock in xfs_file_dio_aio_write for
non-appending writes and only upgrade it to exclusive if the start and
end of the write range are not already allocated and in written
state.  This means one or two extra lookups in the in-core extent tree,
but with our btree data structure those lookups are very cheap and do
not show up in profiles on NVMe hardware for me.  On the other hand
avoiding the lock allows for a high concurrency using aio or io_uring.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Avi Kivity <avi@scylladb.com>
Suggested-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_file.c | 127 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 96 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 1470fc4f2e0255..59d4c6e90f06c1 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -521,6 +521,57 @@ static const struct iomap_dio_ops xfs_dio_write_ops = {
 	.end_io		= xfs_dio_write_end_io,
 };
 
+static int
+xfs_dio_write_exclusive(
+	struct kiocb		*iocb,
+	size_t			count,
+	bool			*exclusive_io)
+{
+	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_ifork	*ifp = &ip->i_df;
+	loff_t			offset = iocb->ki_pos;
+	loff_t			end = offset + count;
+	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
+	xfs_fileoff_t		end_fsb = XFS_B_TO_FSB(mp, end);
+	struct xfs_bmbt_irec	got = { };
+	struct xfs_iext_cursor	icur;
+	int			ret;
+
+	*exclusive_io = true;
+
+	/*
+	 * Bmap information not read in yet or no blocks allocated at all?
+	 */
+	if (!(ifp->if_flags & XFS_IFEXTENTS) || !ip->i_d.di_nblocks)
+		return 0;
+
+	ret = xfs_ilock_iocb(iocb, XFS_ILOCK_SHARED);
+	if (ret)
+		return ret;
+
+	if (offset & mp->m_blockmask) {
+		if (!xfs_iext_lookup_extent(ip, ifp, offset_fsb, &icur, &got) ||
+		    got.br_startoff > offset_fsb ||
+		    got.br_state == XFS_EXT_UNWRITTEN)
+		    	goto out_unlock;
+	}
+
+	if ((end & mp->m_blockmask) &&
+	    got.br_startoff + got.br_blockcount <= end_fsb) {
+		if (!xfs_iext_lookup_extent(ip, ifp, end_fsb, &icur, &got) ||
+		    got.br_startoff > end_fsb ||
+		    got.br_state == XFS_EXT_UNWRITTEN)
+		    	goto out_unlock;
+	}
+
+	*exclusive_io = false;
+
+out_unlock:
+	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+	return ret;
+}
+
 /*
  * xfs_file_dio_aio_write - handle direct IO writes
  *
@@ -557,8 +608,9 @@ xfs_file_dio_aio_write(
 	struct xfs_inode	*ip = XFS_I(inode);
 	struct xfs_mount	*mp = ip->i_mount;
 	ssize_t			ret = 0;
-	int			unaligned_io = 0;
-	int			iolock;
+	int			iolock = XFS_IOLOCK_SHARED;
+	bool			subblock_io = false;
+	bool			exclusive_io = false;
 	size_t			count = iov_iter_count(from);
 	struct xfs_buftarg      *target = xfs_inode_buftarg(ip);
 
@@ -566,45 +618,58 @@ xfs_file_dio_aio_write(
 	if ((iocb->ki_pos | count) & target->bt_logical_sectormask)
 		return -EINVAL;
 
-	/*
-	 * Don't take the exclusive iolock here unless the I/O is unaligned to
-	 * the file system block size.  We don't need to consider the EOF
-	 * extension case here because xfs_file_aio_write_checks() will relock
-	 * the inode as necessary for EOF zeroing cases and fill out the new
-	 * inode size as appropriate.
-	 */
+	/* I/O that is not aligned to the fsblock size will need special care */
 	if ((iocb->ki_pos & mp->m_blockmask) ||
-	    ((iocb->ki_pos + count) & mp->m_blockmask)) {
-		unaligned_io = 1;
+	    ((iocb->ki_pos + count) & mp->m_blockmask))
+		subblock_io = true;
 
-		/*
-		 * We can't properly handle unaligned direct I/O to reflink
-		 * files yet, as we can't unshare a partial block.
-		 */
-		if (xfs_is_cow_inode(ip)) {
-			trace_xfs_reflink_bounce_dio_write(ip, iocb->ki_pos, count);
-			return -ENOTBLK;
-		}
-		iolock = XFS_IOLOCK_EXCL;
-	} else {
-		iolock = XFS_IOLOCK_SHARED;
+	/*
+	 * We can't properly handle unaligned direct I/O to reflink files yet,
+	 * as we can't unshare a partial block.
+	 */
+	if (subblock_io && xfs_is_cow_inode(ip)) {
+		trace_xfs_reflink_bounce_dio_write(ip, iocb->ki_pos, count);
+		return -ENOTBLK;
 	}
 
-	if (iocb->ki_flags & IOCB_NOWAIT) {
-		/* unaligned dio always waits, bail */
-		if (unaligned_io)
-			return -EAGAIN;
-		if (!xfs_ilock_nowait(ip, iolock))
+	/*
+	 * Racy shortcut for obvious appends to avoid too much relocking:
+	 */
+	if (iocb->ki_pos > i_size_read(inode)) {
+		if (iocb->ki_flags & IOCB_NOWAIT)
 			return -EAGAIN;
-	} else {
-		xfs_ilock(ip, iolock);
+		iolock = XFS_IOLOCK_EXCL;
 	}
 
+relock:
+	ret = xfs_ilock_iocb(iocb, iolock);
+	if (ret)
+		return ret;
 	ret = xfs_file_aio_write_checks(iocb, from, &iolock);
 	if (ret)
 		goto out;
 	count = iov_iter_count(from);
 
+	/*
+	 * Upgrade to an exclusive lock and force synchronous completion if the
+	 * I/O will require partial block zeroing.
+	 * We don't need to consider the EOF extension case here because
+	 * xfs_file_aio_write_checks() will relock the inode as necessary for
+	 * EOF zeroing cases and fill out the new inode size as appropriate.
+	 */
+	if (iolock != XFS_IOLOCK_EXCL && subblock_io) {
+		ret = xfs_dio_write_exclusive(iocb, count, &exclusive_io);
+		if (ret)
+			goto out;
+		if (exclusive_io) {
+			xfs_iunlock(ip, iolock);
+			if (iocb->ki_flags & IOCB_NOWAIT)
+				return -EAGAIN;
+			iolock = XFS_IOLOCK_EXCL;
+			goto relock;
+		}
+	}
+
 	/*
 	 * If we are doing unaligned IO, we can't allow any other overlapping IO
 	 * in-flight at the same time or we risk data corruption. Wait for all
@@ -612,7 +677,7 @@ xfs_file_dio_aio_write(
 	 * iolock if we had to take the exclusive lock in
 	 * xfs_file_aio_write_checks() for other reasons.
 	 */
-	if (unaligned_io) {
+	if (exclusive_io) {
 		inode_dio_wait(inode);
 	} else if (iolock == XFS_IOLOCK_EXCL) {
 		xfs_ilock_demote(ip, XFS_IOLOCK_EXCL);
@@ -626,7 +691,7 @@ xfs_file_dio_aio_write(
 	 */
 	ret = iomap_dio_rw(iocb, from, &xfs_direct_write_iomap_ops,
 			   &xfs_dio_write_ops,
-			   is_sync_kiocb(iocb) || unaligned_io);
+			   is_sync_kiocb(iocb) || exclusive_io);
 out:
 	if (iolock)
 		xfs_iunlock(ip, iolock);
-- 
2.29.2

Re: [PATCH 1/3] xfs: factor out a xfs_ilock_iocb helper

[Brian Foster]

On Mon, Jan 11, 2021 at 05:12:10PM +0100, Christoph Hellwig wrote:
> Add a helper to factor out the nowait locking logical for the read/write
> helpers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_file.c | 55 +++++++++++++++++++++++++----------------------
>  1 file changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index ba02780dee6439..aa3fe89628f0f1 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -217,6 +217,23 @@ xfs_file_fsync(
>  	return error;
>  }
>  
> +static int
> +xfs_ilock_iocb(
> +	struct kiocb		*iocb,
> +	unsigned int		lock_mode)
> +{
> +	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
> +
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		if (!xfs_ilock_nowait(ip, lock_mode))
> +			return -EAGAIN;
> +	} else {
> +		xfs_ilock(ip, lock_mode);
> +	}
> +
> +	return 0;
> +}
> +
>  STATIC ssize_t
>  xfs_file_dio_aio_read(
>  	struct kiocb		*iocb,
> @@ -233,12 +250,9 @@ xfs_file_dio_aio_read(
>  
>  	file_accessed(iocb->ki_filp);
>  
> -	if (iocb->ki_flags & IOCB_NOWAIT) {
> -		if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED))
> -			return -EAGAIN;
> -	} else {
> -		xfs_ilock(ip, XFS_IOLOCK_SHARED);
> -	}
> +	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
> +	if (ret)
> +		return ret;
>  	ret = iomap_dio_rw(iocb, to, &xfs_read_iomap_ops, NULL,
>  			is_sync_kiocb(iocb));
>  	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> @@ -260,13 +274,9 @@ xfs_file_dax_read(
>  	if (!count)
>  		return 0; /* skip atime */
>  
> -	if (iocb->ki_flags & IOCB_NOWAIT) {
> -		if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED))
> -			return -EAGAIN;
> -	} else {
> -		xfs_ilock(ip, XFS_IOLOCK_SHARED);
> -	}
> -
> +	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
> +	if (ret)
> +		return ret;
>  	ret = dax_iomap_rw(iocb, to, &xfs_read_iomap_ops);
>  	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>  
> @@ -284,12 +294,9 @@ xfs_file_buffered_aio_read(
>  
>  	trace_xfs_file_buffered_read(ip, iov_iter_count(to), iocb->ki_pos);
>  
> -	if (iocb->ki_flags & IOCB_NOWAIT) {
> -		if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED))
> -			return -EAGAIN;
> -	} else {
> -		xfs_ilock(ip, XFS_IOLOCK_SHARED);
> -	}
> +	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
> +	if (ret)
> +		return ret;
>  	ret = generic_file_read_iter(iocb, to);
>  	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>  
> @@ -628,13 +635,9 @@ xfs_file_dax_write(
>  	size_t			count;
>  	loff_t			pos;
>  
> -	if (iocb->ki_flags & IOCB_NOWAIT) {
> -		if (!xfs_ilock_nowait(ip, iolock))
> -			return -EAGAIN;
> -	} else {
> -		xfs_ilock(ip, iolock);
> -	}
> -
> +	ret = xfs_ilock_iocb(iocb, iolock);
> +	if (ret)
> +		return ret;
>  	ret = xfs_file_aio_write_checks(iocb, from, &iolock);
>  	if (ret)
>  		goto out;
> -- 
> 2.29.2
> 

Re: [PATCH 2/3] xfs: make xfs_file_aio_write_checks IOCB_NOWAIT-aware

[Brian Foster]

On Mon, Jan 11, 2021 at 05:12:11PM +0100, Christoph Hellwig wrote:
> Ensure we don't block on the iolock, or waiting for I/O in
> xfs_file_aio_write_checks if the caller asked to avoid that.
> 
> Fixes: 29a5d29ec181 ("xfs: nowait aio support")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_file.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index aa3fe89628f0f1..1470fc4f2e0255 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -355,7 +355,14 @@ xfs_file_aio_write_checks(
>  	if (error <= 0)
>  		return error;
>  
> -	error = xfs_break_layouts(inode, iolock, BREAK_WRITE);
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		error = break_layout(inode, false);
> +		if (error == -EWOULDBLOCK)
> +			error = -EAGAIN;
> +	} else {
> +		error = xfs_break_layouts(inode, iolock, BREAK_WRITE);
> +	}
> +
>  	if (error)
>  		return error;
>  
> @@ -366,7 +373,11 @@ xfs_file_aio_write_checks(
>  	if (*iolock == XFS_IOLOCK_SHARED && !IS_NOSEC(inode)) {
>  		xfs_iunlock(ip, *iolock);
>  		*iolock = XFS_IOLOCK_EXCL;
> -		xfs_ilock(ip, *iolock);
> +		error = xfs_ilock_iocb(iocb, *iolock);
> +		if (error) {
> +			*iolock = 0;
> +			return error;
> +		}
>  		goto restart;
>  	}
>  	/*
> @@ -388,6 +399,10 @@ xfs_file_aio_write_checks(
>  	isize = i_size_read(inode);
>  	if (iocb->ki_pos > isize) {
>  		spin_unlock(&ip->i_flags_lock);
> +
> +		if (iocb->ki_flags & IOCB_NOWAIT)
> +			return -EAGAIN;
> +
>  		if (!drained_dio) {
>  			if (*iolock == XFS_IOLOCK_SHARED) {
>  				xfs_iunlock(ip, *iolock);
> @@ -613,7 +628,8 @@ xfs_file_dio_aio_write(
>  			   &xfs_dio_write_ops,
>  			   is_sync_kiocb(iocb) || unaligned_io);
>  out:
> -	xfs_iunlock(ip, iolock);
> +	if (iolock)
> +		xfs_iunlock(ip, iolock);
>  
>  	/*
>  	 * No fallback to buffered IO after short writes for XFS, direct I/O
> @@ -652,7 +668,8 @@ xfs_file_dax_write(
>  		error = xfs_setfilesize(ip, pos, ret);
>  	}
>  out:
> -	xfs_iunlock(ip, iolock);
> +	if (iolock)
> +		xfs_iunlock(ip, iolock);
>  	if (error)
>  		return error;
>  
> -- 
> 2.29.2
> 

Re: [PATCH 3/3] xfs: try to avoid the iolock exclusive for non-aligned direct writes

[Brian Foster]

On Mon, Jan 11, 2021 at 05:12:12PM +0100, Christoph Hellwig wrote:
> We only need the exclusive iolock for direct writes to protect sub-block
> zeroing after an allocation or conversion of unwritten extents, and the
> synchronous execution of these writes is also only needed because the
> iolock is dropped early for the dodgy i_dio_count synchronisation.
> 
> Always start out with the shared iolock in xfs_file_dio_aio_write for
> non-appending writes and only upgrade it to exclusive if the start and
> end of the write range are not already allocated and in written
> state.  This means one or two extra lookups in the in-core extent tree,
> but with our btree data structure those lookups are very cheap and do
> not show up in profiles on NVMe hardware for me.  On the other hand
> avoiding the lock allows for a high concurrency using aio or io_uring.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Avi Kivity <avi@scylladb.com>
> Suggested-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_file.c | 127 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 96 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 1470fc4f2e0255..59d4c6e90f06c1 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -521,6 +521,57 @@ static const struct iomap_dio_ops xfs_dio_write_ops = {
>  	.end_io		= xfs_dio_write_end_io,
>  };
>  
> +static int
> +xfs_dio_write_exclusive(
> +	struct kiocb		*iocb,
> +	size_t			count,
> +	bool			*exclusive_io)
> +{
> +	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_ifork	*ifp = &ip->i_df;
> +	loff_t			offset = iocb->ki_pos;
> +	loff_t			end = offset + count;
> +	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
> +	xfs_fileoff_t		end_fsb = XFS_B_TO_FSB(mp, end);
> +	struct xfs_bmbt_irec	got = { };
> +	struct xfs_iext_cursor	icur;
> +	int			ret;
> +
> +	*exclusive_io = true;
> +
> +	/*
> +	 * Bmap information not read in yet or no blocks allocated at all?
> +	 */
> +	if (!(ifp->if_flags & XFS_IFEXTENTS) || !ip->i_d.di_nblocks)
> +		return 0;
> +
> +	ret = xfs_ilock_iocb(iocb, XFS_ILOCK_SHARED);
> +	if (ret)
> +		return ret;

It looks like this helper is only called with ILOCK_SHARED already held.

> +
> +	if (offset & mp->m_blockmask) {
> +		if (!xfs_iext_lookup_extent(ip, ifp, offset_fsb, &icur, &got) ||
> +		    got.br_startoff > offset_fsb ||
> +		    got.br_state == XFS_EXT_UNWRITTEN)
> +		    	goto out_unlock;
> +	}
> +
> +	if ((end & mp->m_blockmask) &&
> +	    got.br_startoff + got.br_blockcount <= end_fsb) {
> +		if (!xfs_iext_lookup_extent(ip, ifp, end_fsb, &icur, &got) ||
> +		    got.br_startoff > end_fsb ||
> +		    got.br_state == XFS_EXT_UNWRITTEN)
> +		    	goto out_unlock;

This line and the same goto in the previous block are both whitespace
damaged (tabs before spaces).

> +	}
> +
> +	*exclusive_io = false;
> +
> +out_unlock:
> +	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +	return ret;
> +}
> +
>  /*
>   * xfs_file_dio_aio_write - handle direct IO writes
>   *
> @@ -557,8 +608,9 @@ xfs_file_dio_aio_write(
>  	struct xfs_inode	*ip = XFS_I(inode);
>  	struct xfs_mount	*mp = ip->i_mount;
>  	ssize_t			ret = 0;
> -	int			unaligned_io = 0;
> -	int			iolock;
> +	int			iolock = XFS_IOLOCK_SHARED;
> +	bool			subblock_io = false;
> +	bool			exclusive_io = false;
>  	size_t			count = iov_iter_count(from);
>  	struct xfs_buftarg      *target = xfs_inode_buftarg(ip);
>  
> @@ -566,45 +618,58 @@ xfs_file_dio_aio_write(
>  	if ((iocb->ki_pos | count) & target->bt_logical_sectormask)
>  		return -EINVAL;
>  
> -	/*
> -	 * Don't take the exclusive iolock here unless the I/O is unaligned to
> -	 * the file system block size.  We don't need to consider the EOF
> -	 * extension case here because xfs_file_aio_write_checks() will relock
> -	 * the inode as necessary for EOF zeroing cases and fill out the new
> -	 * inode size as appropriate.
> -	 */
> +	/* I/O that is not aligned to the fsblock size will need special care */
>  	if ((iocb->ki_pos & mp->m_blockmask) ||
> -	    ((iocb->ki_pos + count) & mp->m_blockmask)) {
> -		unaligned_io = 1;
> +	    ((iocb->ki_pos + count) & mp->m_blockmask))
> +		subblock_io = true;
>  
> -		/*
> -		 * We can't properly handle unaligned direct I/O to reflink
> -		 * files yet, as we can't unshare a partial block.
> -		 */
> -		if (xfs_is_cow_inode(ip)) {
> -			trace_xfs_reflink_bounce_dio_write(ip, iocb->ki_pos, count);
> -			return -ENOTBLK;
> -		}
> -		iolock = XFS_IOLOCK_EXCL;
> -	} else {
> -		iolock = XFS_IOLOCK_SHARED;
> +	/*
> +	 * We can't properly handle unaligned direct I/O to reflink files yet,
> +	 * as we can't unshare a partial block.
> +	 */
> +	if (subblock_io && xfs_is_cow_inode(ip)) {
> +		trace_xfs_reflink_bounce_dio_write(ip, iocb->ki_pos, count);
> +		return -ENOTBLK;
>  	}
>  
> -	if (iocb->ki_flags & IOCB_NOWAIT) {
> -		/* unaligned dio always waits, bail */
> -		if (unaligned_io)
> -			return -EAGAIN;
> -		if (!xfs_ilock_nowait(ip, iolock))
> +	/*
> +	 * Racy shortcut for obvious appends to avoid too much relocking:

s/:/./

> +	 */
> +	if (iocb->ki_pos > i_size_read(inode)) {
> +		if (iocb->ki_flags & IOCB_NOWAIT)
>  			return -EAGAIN;

Not sure why we need this check here if we'll eventually fall into the
serialized check. It seems safer to me to just do 'iolock =
XFS_IOLOCK_EXCL;' here and carry on.

> -	} else {
> -		xfs_ilock(ip, iolock);
> +		iolock = XFS_IOLOCK_EXCL;
>  	}
>  
> +relock:
> +	ret = xfs_ilock_iocb(iocb, iolock);
> +	if (ret)
> +		return ret;
>  	ret = xfs_file_aio_write_checks(iocb, from, &iolock);
>  	if (ret)
>  		goto out;
>  	count = iov_iter_count(from);
>  
> +	/*
> +	 * Upgrade to an exclusive lock and force synchronous completion if the
> +	 * I/O will require partial block zeroing.
> +	 * We don't need to consider the EOF extension case here because
> +	 * xfs_file_aio_write_checks() will relock the inode as necessary for
> +	 * EOF zeroing cases and fill out the new inode size as appropriate.
> +	 */
> +	if (iolock != XFS_IOLOCK_EXCL && subblock_io) {
> +		ret = xfs_dio_write_exclusive(iocb, count, &exclusive_io);
> +		if (ret)
> +			goto out;
> +		if (exclusive_io) {
> +			xfs_iunlock(ip, iolock);
> +			if (iocb->ki_flags & IOCB_NOWAIT)
> +				return -EAGAIN;
> +			iolock = XFS_IOLOCK_EXCL;
> +			goto relock;
> +		}
> +	}
> +
>  	/*
>  	 * If we are doing unaligned IO, we can't allow any other overlapping IO
>  	 * in-flight at the same time or we risk data corruption. Wait for all
> @@ -612,7 +677,7 @@ xfs_file_dio_aio_write(
>  	 * iolock if we had to take the exclusive lock in
>  	 * xfs_file_aio_write_checks() for other reasons.
>  	 */
> -	if (unaligned_io) {
> +	if (exclusive_io) {

Hmm.. so if we hold or upgrade to ILOCK_EXCL from the start for whatever
reason, we'd never actually check whether the I/O is "exclusive" or not.
Then we fall into here, demote the lock and the iomap layer may very
well end up doing subblock zeroing. I suspect if we wanted to maintain
this logic, the exclusive I/O check should occur for any subblock_io
regardless of how the lock is held.

Also, the comment above this check could use an update since it still refers
to "unaligned IO."

Brian

>  		inode_dio_wait(inode);
>  	} else if (iolock == XFS_IOLOCK_EXCL) {
>  		xfs_ilock_demote(ip, XFS_IOLOCK_EXCL);
> @@ -626,7 +691,7 @@ xfs_file_dio_aio_write(
>  	 */
>  	ret = iomap_dio_rw(iocb, from, &xfs_direct_write_iomap_ops,
>  			   &xfs_dio_write_ops,
> -			   is_sync_kiocb(iocb) || unaligned_io);
> +			   is_sync_kiocb(iocb) || exclusive_io);
>  out:
>  	if (iolock)
>  		xfs_iunlock(ip, iolock);
> -- 
> 2.29.2
> 

Re: [PATCH 3/3] xfs: try to avoid the iolock exclusive for non-aligned direct writes

[Christoph Hellwig]

On Mon, Jan 11, 2021 at 01:59:20PM -0500, Brian Foster wrote:
> > +	/*
> > +	 * Bmap information not read in yet or no blocks allocated at all?
> > +	 */
> > +	if (!(ifp->if_flags & XFS_IFEXTENTS) || !ip->i_d.di_nblocks)
> > +		return 0;
> > +
> > +	ret = xfs_ilock_iocb(iocb, XFS_ILOCK_SHARED);
> > +	if (ret)
> > +		return ret;
> 
> It looks like this helper is only called with ILOCK_SHARED already held.

xfs_dio_write_exclusive is called with the iolock held shared, but not
the ilock.


> > +	if (iocb->ki_pos > i_size_read(inode)) {
> > +		if (iocb->ki_flags & IOCB_NOWAIT)
> >  			return -EAGAIN;
> 
> Not sure why we need this check here if we'll eventually fall into the
> serialized check. It seems safer to me to just do 'iolock =
> XFS_IOLOCK_EXCL;' here and carry on.

It seems a little pointless to first acquire the lock for that.  But
in the end this is not what the patch is about, so I'm happy to drop it
if that is preferred.

> > -	if (unaligned_io) {
> > +	if (exclusive_io) {
> 
> Hmm.. so if we hold or upgrade to ILOCK_EXCL from the start for whatever
> reason, we'd never actually check whether the I/O is "exclusive" or not.
> Then we fall into here, demote the lock and the iomap layer may very
> well end up doing subblock zeroing. I suspect if we wanted to maintain
> this logic, the exclusive I/O check should occur for any subblock_io
> regardless of how the lock is held.

True.

Re: [PATCH 3/3] xfs: try to avoid the iolock exclusive for non-aligned direct writes

[Brian Foster]

On Mon, Jan 11, 2021 at 08:14:12PM +0100, Christoph Hellwig wrote:
> On Mon, Jan 11, 2021 at 01:59:20PM -0500, Brian Foster wrote:
> > > +	/*
> > > +	 * Bmap information not read in yet or no blocks allocated at all?
> > > +	 */
> > > +	if (!(ifp->if_flags & XFS_IFEXTENTS) || !ip->i_d.di_nblocks)
> > > +		return 0;
> > > +
> > > +	ret = xfs_ilock_iocb(iocb, XFS_ILOCK_SHARED);
> > > +	if (ret)
> > > +		return ret;
> > 
> > It looks like this helper is only called with ILOCK_SHARED already held.
> 
> xfs_dio_write_exclusive is called with the iolock held shared, but not
> the ilock.
> 

Oops, thinko.

> 
> > > +	if (iocb->ki_pos > i_size_read(inode)) {
> > > +		if (iocb->ki_flags & IOCB_NOWAIT)
> > >  			return -EAGAIN;
> > 
> > Not sure why we need this check here if we'll eventually fall into the
> > serialized check. It seems safer to me to just do 'iolock =
> > XFS_IOLOCK_EXCL;' here and carry on.
> 
> It seems a little pointless to first acquire the lock for that.  But
> in the end this is not what the patch is about, so I'm happy to drop it
> if that is preferred.
> 

It may be fine, but I think the codepath is easier to grok without
creating duplicate/racy branches for logic that potentially impacts
behavior (whereas the lock upgrade only impacts performance).
Particularly in this case where if NOWAIT is set, we'll already bail at
the first appropriate point anyways.

Brian

> > > -	if (unaligned_io) {
> > > +	if (exclusive_io) {
> > 
> > Hmm.. so if we hold or upgrade to ILOCK_EXCL from the start for whatever
> > reason, we'd never actually check whether the I/O is "exclusive" or not.
> > Then we fall into here, demote the lock and the iomap layer may very
> > well end up doing subblock zeroing. I suspect if we wanted to maintain
> > this logic, the exclusive I/O check should occur for any subblock_io
> > regardless of how the lock is held.
> 
> True.
> 

Re: improve sub-block size direct I/O concurrency

[Dave Chinner]

On Mon, Jan 11, 2021 at 05:12:09PM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series avoids taking the iolock exclusively for direct I/O
> writes that are not file system block size aligned, but also do
> not require allocations or unwritten extent conversion.

I wrote patches to do this yesterday, and ran it through testing
overnight. I made a note that I need to do what your first two
patches do, and this morning I was going to completely separate the
DIO unaligned write path from block aligned write path because
there's almost nothing shared between the two cases.

I already commented that I don't like the approach brian suggested
because of the requirement to cycle the ILOCK and duplicate all the
checks that the IOMAP_NOWAIT case already does in the XFS DIO write
submission code. i.e.  I lift ithe trigger for IOMAP_NOWAIT to the
iomap_dio_rw() caller so that we can use IOMAP_NOWAIT for unaligned
IO, and only if that returns -EAGAIN do we run an exclusive IO. THis
also means that we can run sub-block dio as AIO and under RWF_NOWAIT
conditions without explicitly having to check for these things...

Let me rework what I've done on the top of your first two patches
and test it and I'll send it out later today for comparison.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] xfs: try to avoid the iolock exclusive for non-aligned direct writes

[Dave Chinner]

On Mon, Jan 11, 2021 at 05:12:12PM +0100, Christoph Hellwig wrote:
> We only need the exclusive iolock for direct writes to protect sub-block
> zeroing after an allocation or conversion of unwritten extents, and the
> synchronous execution of these writes is also only needed because the
> iolock is dropped early for the dodgy i_dio_count synchronisation.
> 
> Always start out with the shared iolock in xfs_file_dio_aio_write for
> non-appending writes and only upgrade it to exclusive if the start and
> end of the write range are not already allocated and in written
> state.  This means one or two extra lookups in the in-core extent tree,
> but with our btree data structure those lookups are very cheap and do
> not show up in profiles on NVMe hardware for me.  On the other hand
> avoiding the lock allows for a high concurrency using aio or io_uring.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Avi Kivity <avi@scylladb.com>
> Suggested-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_file.c | 127 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 96 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 1470fc4f2e0255..59d4c6e90f06c1 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -521,6 +521,57 @@ static const struct iomap_dio_ops xfs_dio_write_ops = {
>  	.end_io		= xfs_dio_write_end_io,
>  };
>  
> +static int
> +xfs_dio_write_exclusive(
> +	struct kiocb		*iocb,
> +	size_t			count,
> +	bool			*exclusive_io)
> +{
> +	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_ifork	*ifp = &ip->i_df;
> +	loff_t			offset = iocb->ki_pos;
> +	loff_t			end = offset + count;
> +	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
> +	xfs_fileoff_t		end_fsb = XFS_B_TO_FSB(mp, end);
> +	struct xfs_bmbt_irec	got = { };
> +	struct xfs_iext_cursor	icur;
> +	int			ret;
> +
> +	*exclusive_io = true;
> +
> +	/*
> +	 * Bmap information not read in yet or no blocks allocated at all?
> +	 */
> +	if (!(ifp->if_flags & XFS_IFEXTENTS) || !ip->i_d.di_nblocks)
> +		return 0;
> +
> +	ret = xfs_ilock_iocb(iocb, XFS_ILOCK_SHARED);
> +	if (ret)
> +		return ret;
> +
> +	if (offset & mp->m_blockmask) {
> +		if (!xfs_iext_lookup_extent(ip, ifp, offset_fsb, &icur, &got) ||
> +		    got.br_startoff > offset_fsb ||
> +		    got.br_state == XFS_EXT_UNWRITTEN)
> +		    	goto out_unlock;
> +	}
> +
> +	if ((end & mp->m_blockmask) &&
> +	    got.br_startoff + got.br_blockcount <= end_fsb) {
> +		if (!xfs_iext_lookup_extent(ip, ifp, end_fsb, &icur, &got) ||
> +		    got.br_startoff > end_fsb ||
> +		    got.br_state == XFS_EXT_UNWRITTEN)
> +		    	goto out_unlock;
> +	}
> +
> +	*exclusive_io = false;
> +
> +out_unlock:
> +	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +	return ret;
> +}

OK, this has a bug in it. We have to do exclusive sub-block IO when
the end of the IO is >= i_size_read(inode) because in this situation
iomap_dio_rw_actor() will issue an extra bio to zero the portion of
the filesystem block that lies beyond EOF. generic/418 hammers this
case, and randomly ends up with zeroes where there should be data.

Otherwise, it largely makes a similar mess of
xfs_file_dio_aio_write() as my initial patchset does. We should just
move the unaligned IO out of the main path, regardless of whether it
is exclusive or not.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com