Re: [RFC PATCH v5 2/4] ocfs2: switch DIO read path from buffer_head to iomap

[Heming Zhao <heming.zhao@suse.com>]

On 11/6/26 18:48, Christoph Hellwig wrote:
> On Thu, Nov 05, 2026 at 03:19:26PM +0800, Heming Zhao wrote:
>> Migrates the DIO read path from the legacy buffer_head infrastructure
>> to the modern and more efficient iomap framework.
>>
>> Under the current code, ocfs2 holds a cluster lock during DIO via
>> ocfs2_iocb_set_rw_locked(), and releases the cluster lock in
>> ocfs2_dio_end_io() via ocfs2_iocb_clear_rw_locked(). The lock level is
>> stashed in iocb->private.
>>
>> However, iomap DIO owns that field (it stores a bio there for polled I/O
>> and unconditionally clears it before calling ->end_io). Any side-channel
>> state packed into iocb->private is corrupted, which trips the end_io
>> lock-state check.
>>
>> Instead, encode the unlock level by passing distinct iomap_dio_ops methods
>> (ocfs2_iomap_dio_ops_pr for PRMODE or ocfs2_iomap_dio_ops_ex for EXMODE).
>> Additionally, the *_iter() functions use the split __iomap_dio_rw() +
>> iomap_dio_complete() API so they can determine unambiguously, from the
>> return value, whether the completion handler already dropped the lock
>> (real I/O, sync or -EIOCBQUEUED) or whether the caller must drop it
>> (no I/O issued, or buffered fallback).
>>
>> The new function ocfs2_iomap_begin() performs a similar role to
>> ocfs2_get_block(). While ocfs2_get_block() retrieves only one block per
>> call, ocfs2_iomap_begin() can handle multiple blocks at once. Limit the
>> map offset and length in ocfs2_iomap_begin() to OCFS2_MAX_LOGICAL_BLOCK
>> to avoid overflow issues.
>>
>> Co-developed-by: Joseph Qi <joseph.qi@linux.alibaba.com>
>> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
>> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
>> ---
>>   fs/ocfs2/Kconfig    |   1 +
>>   fs/ocfs2/aops.c     | 111 ++++++++++++++++++++++++++++++++++++++++++++
>>   fs/ocfs2/aops.h     |   4 ++
>>   fs/ocfs2/file.c     |  62 +++++++++++++++++++++----
>>   fs/ocfs2/ocfs2_fs.h |   3 ++
>>   5 files changed, 172 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/ocfs2/Kconfig b/fs/ocfs2/Kconfig
>> index 2514d36cbe01..bf1678a5eb01 100644
>> --- a/fs/ocfs2/Kconfig
>> +++ b/fs/ocfs2/Kconfig
>> @@ -7,6 +7,7 @@ config OCFS2_FS
>>   	select CRC32
>>   	select QUOTA
>>   	select QUOTA_TREE
>> +	select FS_IOMAP
>>   	select FS_POSIX_ACL
>>   	select LEGACY_DIRECT_IO
>>   	help
>> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
>> index 0f7502de9b8d..8e4d31cb85db 100644
>> --- a/fs/ocfs2/aops.c
>> +++ b/fs/ocfs2/aops.c
>> @@ -4,6 +4,7 @@
>>    */
>>   
>>   #include <linux/fs.h>
>> +#include <linux/iomap.h>
>>   #include <linux/slab.h>
>>   #include <linux/highmem.h>
>>   #include <linux/pagemap.h>
>> @@ -2534,6 +2535,116 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>>   				    ocfs2_dio_end_io, 0);
>>   }
>>   
>> +static int ocfs2_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>> +		unsigned int flags, struct iomap *iomap, struct iomap *srcmap)
>> +{
>> +	int ret;
>> +	struct ocfs2_map_block map;
>> +	struct ocfs2_inode_info *oi = OCFS2_I(inode);
>> +	u8 blkbits = inode->i_blkbits;
>> +
>> +	if ((offset >> blkbits) > OCFS2_MAX_LOGICAL_BLOCK)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * Calculate the first and last logical blocks respectively.
>> +	 */
>> +	map.lblk = offset >> blkbits;
>> +	map.len = min_t(loff_t, (offset + length - 1) >> blkbits,
>> +			OCFS2_MAX_LOGICAL_BLOCK) - map.lblk + 1;
>> +	map.flags = 0;
>> +
>> +	if (flags & IOMAP_WRITE) {
>> +		return -EOPNOTSUPP;
>> +	} else {
> 
> No need for an else after a return..

OK.

> 
>> +	/*
>> +	 * Before returning to iomap, let's ensure the allocated mapping
>> +	 * covers the entire requested length for atomic writes.
>> +	 */
>> +	if (flags & IOMAP_ATOMIC) {
> 
> IOMAP_ATOMIC is never set for reads.
> 

Thanks for your review, I will remove it on next version.

Thanks,
Heming