Re: [RFC PATCH 1/2] xfs: Add i_direct_mode to indicate the IO mode of inode

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Apr 25, 2025 at 06:38:40PM +0800, Chi Zhiling wrote:
> From: Chi Zhiling <chizhiling@kylinos.cn>
> 
> Direct IO already uses shared lock. If buffered write also uses
> shared lock, we need to ensure mutual exclusion between DIO and
> buffered IO. Therefore, Now introduce a flag `i_direct_mode` to
> indicate the IO mode currently used by the file. In practical
> scenarios, DIO and buffered IO are typically not used together,
> so this flag is usually not modified.
> 
> Additionally, this flag is protected by the i_rwsem lock,
> which avoids the need to introduce new lock. When reading this
> flag, we need to hold a read lock, and when writing, a write lock
> is required.
> 
> When a file that uses buffered IO starts using DIO, it needs to
> acquire a write lock to modify i_direct_mode, which will force DIO
> to wait for the previous IO to complete before starting. After
> acquiring the write lock to modify `i_direct_mode`, subsequent
> buffered IO will need to acquire the write lock again to modify
> i_direct_mode, which will force those IOs to wait for the current
> IO to complete.
> 
> Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
> ---
>  fs/xfs/xfs_file.c  | 37 +++++++++++++++++++++++++++++++++----
>  fs/xfs/xfs_inode.h |  6 ++++++
>  2 files changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 84f08c976ac4..a6f214f57238 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -206,7 +206,8 @@ xfs_ilock_iocb(
>  static int
>  xfs_ilock_iocb_for_write(
>  	struct kiocb		*iocb,
> -	unsigned int		*lock_mode)
> +	unsigned int		*lock_mode,
> +	bool			is_dio)

Is an explicit flag required here, or can you determine directness from
IS_DAX() || (iocb->ki_flags & IOCB_DIRECT) ?

Hmm, I guess not, since a directio falling back to the pagecache for an
unaligned out of place write doesn't clear IOCB_DIRECT?

How does this new flag intersect with XFS_IREMAPPING?  Are we actually
modelling three states here: bufferedio <-> directio <-> remapping?

>  {
>  	ssize_t			ret;
>  	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
> @@ -226,6 +227,21 @@ xfs_ilock_iocb_for_write(
>  		return xfs_ilock_iocb(iocb, *lock_mode);
>  	}
>  
> +	/*
> +	 * If the i_direct_mode need update, take the iolock exclusively to write
> +	 * it.
> +	 */
> +	if (ip->i_direct_mode != is_dio) {
> +		if (*lock_mode == XFS_IOLOCK_SHARED) {
> +			xfs_iunlock(ip, *lock_mode);
> +			*lock_mode = XFS_IOLOCK_EXCL;
> +			ret = xfs_ilock_iocb(iocb, *lock_mode);
> +			if (ret)
> +				return ret;
> +		}
> +		ip->i_direct_mode = is_dio;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -247,6 +263,19 @@ xfs_file_dio_read(
>  	ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_SHARED);
>  	if (ret)
>  		return ret;
> +
> +	if (!ip->i_direct_mode) {
> +		xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> +		ret = xfs_ilock_iocb(iocb, XFS_IOLOCK_EXCL);
> +		if (ret)
> +			return ret;
> +
> +		ip->i_direct_mode = 1;
> +
> +		/* Update finished, now downgrade to shared lock */
> +		xfs_ilock_demote(ip, XFS_IOLOCK_EXCL);
> +	}
> +
>  	ret = iomap_dio_rw(iocb, to, &xfs_read_iomap_ops, NULL, 0, NULL, 0);
>  	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>  
> @@ -680,7 +709,7 @@ xfs_file_dio_write_aligned(
>  	unsigned int		iolock = XFS_IOLOCK_SHARED;
>  	ssize_t			ret;
>  
> -	ret = xfs_ilock_iocb_for_write(iocb, &iolock);
> +	ret = xfs_ilock_iocb_for_write(iocb, &iolock, true);
>  	if (ret)
>  		return ret;
>  	ret = xfs_file_write_checks(iocb, from, &iolock, ac);
> @@ -767,7 +796,7 @@ xfs_file_dio_write_unaligned(
>  		flags = IOMAP_DIO_FORCE_WAIT;
>  	}
>  
> -	ret = xfs_ilock_iocb_for_write(iocb, &iolock);
> +	ret = xfs_ilock_iocb_for_write(iocb, &iolock, true);
>  	if (ret)
>  		return ret;
>  
> @@ -898,7 +927,7 @@ xfs_file_buffered_write(
>  
>  write_retry:
>  	iolock = XFS_IOLOCK_EXCL;
> -	ret = xfs_ilock_iocb(iocb, iolock);
> +	ret = xfs_ilock_iocb_for_write(iocb, &iolock, false);
>  	if (ret)
>  		return ret;
>  
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index eae0159983ca..04f6c4174fab 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -51,6 +51,12 @@ typedef struct xfs_inode {
>  	uint16_t		i_checked;
>  	uint16_t		i_sick;
>  
> +	/*
> +	 * Indicates the current IO mode of this inode, (DIO/buffered IO)
> +	 * protected by i_rwsem lock.
> +	 */
> +	uint32_t		i_direct_mode;

Yeesh, a whole u32 to encode a single bit.  Can you use i_flags instead?

--D

> +
>  	spinlock_t		i_flags_lock;	/* inode i_flags lock */
>  	/* Miscellaneous state. */
>  	unsigned long		i_flags;	/* see defined flags below */
> -- 
> 2.43.0
> 
>