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

[Chi Zhiling <chizhiling@163.com>]

On 2025/4/25 23:12, Darrick J. Wong wrote:
> 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?

Because DIO can fallback to buffered IO, I think checking 
(iocb->ki_flags & IOCB_DIRECT) is not accurate.

That's why we need to add an additional argument.

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

Yes, and these three states are mutually exclusive.

That's a good suggestion. I think we can include XFS_IREMAPPING in the
new flag as well.

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

I think we can add i_remapping to this new flag, and rename it to
i_current_state, so it can be remapping, DIO, or BIO.

The rule remains the same: it should be protected by i_rwsem, with the
write lock held to change it.

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

Sorry, It's a mistake, But I don't think we can use i_flags instead.

I tried using i_flags for this, but i_flags is protected by
i_flags_lock, which means that for every IO operation, it always
requires an additional acquisition of i_flags_lock to check this flag.


Thanks

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