On Fri, Apr 25, 2025 at 06:38:40PM +0800, Chi Zhiling wrote:
> From: Chi Zhiling <chizhiling@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
>
>
