Re: [RFC PATCH v2 4/4] ocfs2: remove legacy blockdev direct-IO path APIs

On 7/27/26 2:18 PM, Heming Zhao wrote:
> Since the DIO path already migrated from buffer_head mode (by using
> a_ops->direct_IO method) to iomap mode, the VFS no longer sets
> FMODE_CAN_ODIRECT automatically, so ocfs2_file_open() sets it
> explicitly.
> 
> The now-unused blockdev get_block/end_io machinery
> (ocfs2_lock_get_block(), ocfs2_dio_wr_get_block(), ocfs2_dio_end_io(),
> ocfs2_dio_end_io_write(), and the ocfs2_dio_write_ctxt helpers) is removed,
> and OCFS2_FS Kconfig removes LEGACY_DIRECT_IO.
> 
> Co-developed-by: Joseph Qi <[email protected]>
> Signed-off-by: Joseph Qi <[email protected]>
> Signed-off-by: Heming Zhao <[email protected]>
> ---
>  fs/ocfs2/Kconfig |   1 -
>  fs/ocfs2/aops.c  | 411 -----------------------------------------------
>  fs/ocfs2/aops.h  |  35 +---
>  fs/ocfs2/file.c  |  13 +-
>  fs/ocfs2/ocfs2.h |   5 -
>  5 files changed, 16 insertions(+), 449 deletions(-)
> 
> diff --git a/fs/ocfs2/Kconfig b/fs/ocfs2/Kconfig
> index bf1678a5eb01..74bc2008ef0b 100644
> --- a/fs/ocfs2/Kconfig
> +++ b/fs/ocfs2/Kconfig
> @@ -9,7 +9,6 @@ config OCFS2_FS
>  	select QUOTA_TREE
>  	select FS_IOMAP
>  	select FS_POSIX_ACL
> -	select LEGACY_DIRECT_IO
>  	help
>  	  OCFS2 is a general purpose extent based shared disk cluster file
>  	  system with many similarities to ext3. It supports 64 bit inode
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 9a079436c9c0..3e4a7043074a 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -114,19 +114,6 @@ static int ocfs2_symlink_get_block(struct inode *inode, sector_t iblock,
>  	return err;
>  }
>  
> -static int ocfs2_lock_get_block(struct inode *inode, sector_t iblock,
> -		    struct buffer_head *bh_result, int create)
> -{
> -	int ret = 0;
> -	struct ocfs2_inode_info *oi = OCFS2_I(inode);
> -
> -	down_read(&oi->ip_alloc_sem);
> -	ret = ocfs2_get_block(inode, iblock, bh_result, create);
> -	up_read(&oi->ip_alloc_sem);
> -
> -	return ret;
> -}
> -
>  int ocfs2_map_blocks(struct inode *inode, struct ocfs2_map_block *map,
>  		    int flags)
>  {
> @@ -2170,204 +2157,6 @@ static int ocfs2_write_end(const struct kiocb *iocb,
>  	return ret;
>  }
>  
> -struct ocfs2_dio_write_ctxt {
> -	struct list_head	dw_zero_list;
> -	unsigned		dw_zero_count;
> -	int			dw_orphaned;
> -	pid_t			dw_writer_pid;
> -};
> -
> -static struct ocfs2_dio_write_ctxt *
> -ocfs2_dio_alloc_write_ctx(struct buffer_head *bh, int *alloc)
> -{
> -	struct ocfs2_dio_write_ctxt *dwc = NULL;
> -
> -	if (bh->b_private)
> -		return bh->b_private;
> -
> -	dwc = kmalloc_obj(struct ocfs2_dio_write_ctxt, GFP_NOFS);
> -	if (dwc == NULL)
> -		return NULL;
> -	INIT_LIST_HEAD(&dwc->dw_zero_list);
> -	dwc->dw_zero_count = 0;
> -	dwc->dw_orphaned = 0;
> -	dwc->dw_writer_pid = task_pid_nr(current);
> -	bh->b_private = dwc;
> -	*alloc = 1;
> -
> -	return dwc;
> -}
> -
> -static void ocfs2_dio_free_write_ctx(struct inode *inode,
> -				     struct ocfs2_dio_write_ctxt *dwc)
> -{
> -	ocfs2_free_unwritten_list(inode, &dwc->dw_zero_list);
> -	kfree(dwc);
> -}
> -
> -/*
> - * TODO: Make this into a generic get_blocks function.
> - *
> - * From do_direct_io in direct-io.c:
> - *  "So what we do is to permit the ->get_blocks function to populate
> - *   bh.b_size with the size of IO which is permitted at this offset and
> - *   this i_blkbits."
> - *
> - * This function is called directly from get_more_blocks in direct-io.c.
> - *
> - * called like this: dio->get_blocks(dio->inode, fs_startblk,
> - * 					fs_count, map_bh, dio->rw == WRITE);
> - */
> -static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock,
> -			       struct buffer_head *bh_result, int create)
> -{
> -	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> -	struct ocfs2_inode_info *oi = OCFS2_I(inode);
> -	struct ocfs2_write_ctxt *wc;
> -	struct ocfs2_write_cluster_desc *desc = NULL;
> -	struct ocfs2_dio_write_ctxt *dwc = NULL;
> -	struct buffer_head *di_bh = NULL;
> -	u64 p_blkno = 0;
> -	unsigned int i_blkbits = inode->i_sb->s_blocksize_bits;
> -	loff_t pos = iblock << i_blkbits;
> -	sector_t endblk = (i_size_read(inode) - 1) >> i_blkbits;
> -	unsigned len, total_len = bh_result->b_size;
> -	int ret = 0, first_get_block = 0;
> -
> -	len = osb->s_clustersize - (pos & (osb->s_clustersize - 1));
> -	len = min(total_len, len);
> -
> -	/*
> -	 * bh_result->b_size is count in get_more_blocks according to write
> -	 * "pos" and "end", we need map twice to return different buffer state:
> -	 * 1. area in file size, not set NEW;
> -	 * 2. area out file size, set  NEW.
> -	 *
> -	 *		   iblock    endblk
> -	 * |--------|---------|---------|---------
> -	 * |<-------area in file------->|
> -	 */
> -
> -	if ((iblock <= endblk) &&
> -	    ((iblock + ((len - 1) >> i_blkbits)) > endblk))
> -		len = (endblk - iblock + 1) << i_blkbits;
> -
> -	mlog(0, "get block of %llu at %llu:%u req %u\n",
> -			inode->i_ino, pos, len, total_len);
> -
> -	/*
> -	 * Because we need to change file size in ocfs2_dio_end_io_write(), or
> -	 * we may need to add it to orphan dir. So can not fall to fast path
> -	 * while file size will be changed.
> -	 */
> -	if (pos + total_len <= i_size_read(inode)) {
> -
> -		/* This is the fast path for re-write. */
> -		ret = ocfs2_lock_get_block(inode, iblock, bh_result, create);
> -		if (buffer_mapped(bh_result) &&
> -		    !buffer_new(bh_result) &&
> -		    ret == 0)
> -			goto out;
> -
> -		/* Clear state set by ocfs2_get_block. */
> -		bh_result->b_state = 0;
> -	}
> -
> -	dwc = ocfs2_dio_alloc_write_ctx(bh_result, &first_get_block);
> -	if (unlikely(dwc == NULL)) {
> -		ret = -ENOMEM;
> -		mlog_errno(ret);
> -		goto out;
> -	}
> -
> -	if (ocfs2_clusters_for_bytes(inode->i_sb, pos + total_len) >
> -	    ocfs2_clusters_for_bytes(inode->i_sb, i_size_read(inode)) &&
> -	    !dwc->dw_orphaned) {
> -		/*
> -		 * when we are going to alloc extents beyond file size, add the
> -		 * inode to orphan dir, so we can recall those spaces when
> -		 * system crashed during write.
> -		 */
> -		ret = ocfs2_add_inode_to_orphan(osb, inode);
> -		if (ret < 0) {
> -			mlog_errno(ret);
> -			goto out;
> -		}
> -		dwc->dw_orphaned = 1;
> -	}
> -
> -	ret = ocfs2_inode_lock(inode, &di_bh, 1);
> -	if (ret) {
> -		mlog_errno(ret);
> -		goto out;
> -	}
> -
> -	down_write(&oi->ip_alloc_sem);
> -
> -	if (first_get_block) {
> -		if (ocfs2_sparse_alloc(osb))
> -			ret = ocfs2_zero_tail(inode, di_bh, pos);
> -		else
> -			ret = ocfs2_expand_nonsparse_inode(inode, di_bh, pos,
> -							   total_len, NULL);
> -		if (ret < 0) {
> -			mlog_errno(ret);
> -			goto unlock;
> -		}
> -	}
> -
> -	ret = ocfs2_write_begin_nolock(inode->i_mapping, pos, len,
> -				       OCFS2_WRITE_DIRECT, NULL,
> -				       (void **)&wc, di_bh, NULL);
> -	if (ret) {
> -		mlog_errno(ret);
> -		goto unlock;
> -	}
> -
> -	desc = &wc->w_desc[0];
> -
> -	p_blkno = ocfs2_clusters_to_blocks(inode->i_sb, desc->c_phys);
> -	BUG_ON(p_blkno == 0);
> -	p_blkno += iblock & (u64)(ocfs2_clusters_to_blocks(inode->i_sb, 1) - 1);
> -
> -	map_bh(bh_result, inode->i_sb, p_blkno);
> -	bh_result->b_size = len;
> -	if (desc->c_needs_zero)
> -		set_buffer_new(bh_result);
> -
> -	if (iblock > endblk)
> -		set_buffer_new(bh_result);
> -
> -	/* May sleep in end_io. It should not happen in a irq context. So defer
> -	 * it to dio work queue. */
> -	set_buffer_defer_completion(bh_result);
> -
> -	if (!list_empty(&wc->w_unwritten_list)) {
> -		struct ocfs2_unwritten_extent *ue = NULL;
> -
> -		ue = list_first_entry(&wc->w_unwritten_list,
> -				      struct ocfs2_unwritten_extent,
> -				      ue_node);
> -		BUG_ON(ue->ue_cpos != desc->c_cpos);
> -		/* The physical address may be 0, fill it. */
> -		ue->ue_phys = desc->c_phys;
> -
> -		list_splice_tail_init(&wc->w_unwritten_list, &dwc->dw_zero_list);
> -		dwc->dw_zero_count += wc->w_unwritten_count;
> -	}
> -
> -	ret = ocfs2_write_end_nolock(inode->i_mapping, pos, len, len, wc);
> -	BUG_ON(ret != len);
> -	ret = 0;
> -unlock:
> -	up_write(&oi->ip_alloc_sem);
> -	ocfs2_inode_unlock(inode, 1);
> -	brelse(di_bh);
> -out:
> -	return ret;
> -}
> -
> -/* copy from ocfs2_dio_wr_get_block */
>  static int ocfs2_dio_wr_map_blocks(struct inode *inode,
>  			       struct ocfs2_map_block *map, int create)
>  {
> @@ -2509,205 +2298,6 @@ static int ocfs2_dio_wr_map_blocks(struct inode *inode,
>  	return ret;
>  }
>  
> -static int ocfs2_dio_end_io_write(struct inode *inode,
> -				  struct ocfs2_dio_write_ctxt *dwc,
> -				  loff_t offset,
> -				  ssize_t bytes)
> -{
> -	struct ocfs2_cached_dealloc_ctxt dealloc;
> -	struct ocfs2_extent_tree et;
> -	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> -	struct ocfs2_inode_info *oi = OCFS2_I(inode);
> -	struct ocfs2_unwritten_extent *ue = NULL;
> -	struct buffer_head *di_bh = NULL;
> -	struct ocfs2_dinode *di;
> -	struct ocfs2_alloc_context *data_ac = NULL;
> -	struct ocfs2_alloc_context *meta_ac = NULL;
> -	handle_t *handle = NULL;
> -	loff_t end = offset + bytes;
> -	int ret = 0, credits = 0, batch = 0;
> -
> -	ocfs2_init_dealloc_ctxt(&dealloc);
> -
> -	/* We do clear unwritten, delete orphan, change i_size here. If neither
> -	 * of these happen, we can skip all this. */
> -	if (list_empty(&dwc->dw_zero_list) &&
> -	    end <= i_size_read(inode) &&
> -	    !dwc->dw_orphaned)
> -		goto out;
> -
> -	ret = ocfs2_inode_lock(inode, &di_bh, 1);
> -	if (ret < 0) {
> -		mlog_errno(ret);
> -		goto out;
> -	}
> -
> -	down_write(&oi->ip_alloc_sem);
> -	di = (struct ocfs2_dinode *)di_bh->b_data;
> -
> -	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh);
> -
> -	/* Attach dealloc with extent tree in case that we may reuse extents
> -	 * which are already unlinked from current extent tree due to extent
> -	 * rotation and merging.
> -	 */
> -	et.et_dealloc = &dealloc;
> -
> -	ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2,
> -				    &data_ac, &meta_ac);
> -	if (ret) {
> -		mlog_errno(ret);
> -		goto unlock;
> -	}
> -
> -	credits = ocfs2_calc_extend_credits(inode->i_sb, &di->id2.i_list);
> -
> -	list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) {
> -		if (!handle) {
> -			handle = ocfs2_start_trans(osb, credits);
> -			if (IS_ERR(handle)) {
> -				ret = PTR_ERR(handle);
> -				mlog_errno(ret);
> -				goto unlock;
> -			}
> -			ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh,
> -					OCFS2_JOURNAL_ACCESS_WRITE);
> -			if (ret) {
> -				mlog_errno(ret);
> -				goto commit;
> -			}
> -		}
> -		ret = ocfs2_assure_trans_credits(handle, credits);
> -		if (ret < 0) {
> -			mlog_errno(ret);
> -			goto commit;
> -		}
> -		ret = ocfs2_mark_extent_written(inode, &et, handle,
> -						ue->ue_cpos, 1,
> -						ue->ue_phys,
> -						meta_ac, &dealloc);
> -		if (ret < 0) {
> -			mlog_errno(ret);
> -			goto commit;
> -		}
> -
> -		if (++batch == OCFS2_DIO_MARK_EXTENT_BATCH) {
> -			ocfs2_commit_trans(osb, handle);
> -			handle = NULL;
> -			batch = 0;
> -		}
> -	}
> -
> -	if (end > i_size_read(inode)) {
> -		if (!handle) {
> -			handle = ocfs2_start_trans(osb, credits);
> -			if (IS_ERR(handle)) {
> -				ret = PTR_ERR(handle);
> -				mlog_errno(ret);
> -				goto unlock;
> -			}
> -		}
> -		ret = ocfs2_set_inode_size(handle, inode, di_bh, end);
> -		if (ret < 0)
> -			mlog_errno(ret);
> -	}
> -
> -commit:
> -	if (handle)
> -		ocfs2_commit_trans(osb, handle);
> -unlock:
> -	up_write(&oi->ip_alloc_sem);
> -
> -	if (data_ac) {
> -		ocfs2_free_alloc_context(data_ac);
> -		data_ac = NULL;
> -	}
> -	if (meta_ac) {
> -		ocfs2_free_alloc_context(meta_ac);
> -		meta_ac = NULL;
> -	}
> -
> -	/* everything looks good, let's start the cleanup */
> -	if (!ret && dwc->dw_orphaned) {
> -		BUG_ON(dwc->dw_writer_pid != task_pid_nr(current));
> -
> -		ret = ocfs2_del_inode_from_orphan(osb, inode, di_bh, 0, 0);
> -		if (ret < 0)
> -			mlog_errno(ret);
> -	}
> -	ocfs2_inode_unlock(inode, 1);
> -	brelse(di_bh);
> -out:
> -	ocfs2_run_deallocs(osb, &dealloc);
> -	ocfs2_dio_free_write_ctx(inode, dwc);
> -
> -	return ret;
> -}
> -
> -/*
> - * ocfs2_dio_end_io is called by the dio core when a dio is finished.  We're
> - * particularly interested in the aio/dio case.  We use the rw_lock DLM lock
> - * to protect io on one node from truncation on another.
> - */
> -static int ocfs2_dio_end_io(struct kiocb *iocb,
> -			    loff_t offset,
> -			    ssize_t bytes,
> -			    void *private)
> -{
> -	struct inode *inode = file_inode(iocb->ki_filp);
> -	int level;
> -	int ret = 0;
> -
> -	/* this io's submitter should not have unlocked this before we could */
> -	BUG_ON(!ocfs2_iocb_is_rw_locked(iocb));
> -
> -	if (bytes <= 0)
> -		mlog_ratelimited(ML_ERROR, "Direct IO failed, bytes = %lld",
> -				 (long long)bytes);
> -	if (private) {
> -		if (bytes > 0)
> -			ret = ocfs2_dio_end_io_write(inode, private, offset,
> -						     bytes);
> -		else
> -			ocfs2_dio_free_write_ctx(inode, private);
> -	}
> -
> -	ocfs2_iocb_clear_rw_locked(iocb);
> -
> -	level = ocfs2_iocb_rw_locked_level(iocb);
> -	ocfs2_rw_unlock(inode, level);
> -	return ret;
> -}
> -
> -static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
> -{
> -	struct file *file = iocb->ki_filp;
> -	struct inode *inode = file->f_mapping->host;
> -	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> -	get_block_t *get_block;
> -
> -	/*
> -	 * Fallback to buffered I/O if we see an inode without
> -	 * extents.
> -	 */
> -	if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL)
> -		return 0;
> -
> -	/* Fallback to buffered I/O if we do not support append dio. */
> -	if (iocb->ki_pos + iter->count > i_size_read(inode) &&
> -	    !ocfs2_supports_append_dio(osb))
> -		return 0;
> -
> -	if (iov_iter_rw(iter) == READ)
> -		get_block = ocfs2_lock_get_block;
> -	else
> -		get_block = ocfs2_dio_wr_get_block;
> -
> -	return __blockdev_direct_IO(iocb, inode, inode->i_sb->s_bdev,
> -				    iter, get_block,
> -				    ocfs2_dio_end_io, 0);
> -}
> -
>  static int ocfs2_iomap_alloc(struct inode *inode, struct ocfs2_map_block *map,
>  			    unsigned int flags)
>  {
> @@ -3045,7 +2635,6 @@ const struct address_space_operations ocfs2_aops = {
>  	.write_begin		= ocfs2_write_begin,
>  	.write_end		= ocfs2_write_end,
>  	.bmap			= ocfs2_bmap,
> -	.direct_IO		= ocfs2_direct_IO,
>  	.invalidate_folio	= block_invalidate_folio,
>  	.release_folio		= ocfs2_release_folio,
>  	.migrate_folio		= buffer_migrate_folio,
> diff --git a/fs/ocfs2/aops.h b/fs/ocfs2/aops.h
> index 8dd6edd7c1a1..484b25d749d7 100644
> --- a/fs/ocfs2/aops.h
> +++ b/fs/ocfs2/aops.h
> @@ -7,6 +7,12 @@
>  #define OCFS2_AOPS_H
>  
>  #include <linux/fs.h>
> +#include <linux/iomap.h>
> +
> +extern const struct iomap_ops ocfs2_iomap_ops;
> +extern const struct iomap_dio_ops ocfs2_iomap_dio_ops_r_pr;
> +extern const struct iomap_dio_ops ocfs2_iomap_dio_ops_w_pr;
> +extern const struct iomap_dio_ops ocfs2_iomap_dio_ops_w_ex;
>  
>  int ocfs2_map_folio_blocks(struct folio *folio, u64 *p_blkno,
>  			  struct inode *inode, unsigned int from,
> @@ -44,34 +50,5 @@ int ocfs2_get_block(struct inode *inode, sector_t iblock,
>  		    struct buffer_head *bh_result, int create);
>  int ocfs2_map_blocks(struct inode *inode, struct ocfs2_map_block *map,
>  		    int flags);
> -/* all ocfs2_dio_end_io()'s fault */
> -#define ocfs2_iocb_is_rw_locked(iocb) \
> -	test_bit(0, (unsigned long *)&iocb->private)
> -static inline void ocfs2_iocb_set_rw_locked(struct kiocb *iocb, int level)
> -{
> -	set_bit(0, (unsigned long *)&iocb->private);
> -	if (level)
> -		set_bit(1, (unsigned long *)&iocb->private);
> -	else
> -		clear_bit(1, (unsigned long *)&iocb->private);
> -}
> -
> -/*
> - * Using a named enum representing lock types in terms of #N bit stored in
> - * iocb->private, which is going to be used for communication between
> - * ocfs2_dio_end_io() and ocfs2_file_write/read_iter().
> - */
> -enum ocfs2_iocb_lock_bits {
> -	OCFS2_IOCB_RW_LOCK = 0,
> -	OCFS2_IOCB_RW_LOCK_LEVEL,
> -	OCFS2_IOCB_NUM_LOCKS
> -};
> -
> -#define ocfs2_iocb_init_rw_locked(iocb) \
> -	(iocb->private = NULL)
> -#define ocfs2_iocb_clear_rw_locked(iocb) \
> -	clear_bit(OCFS2_IOCB_RW_LOCK, (unsigned long *)&iocb->private)
> -#define ocfs2_iocb_rw_locked_level(iocb) \
> -	test_bit(OCFS2_IOCB_RW_LOCK_LEVEL, (unsigned long *)&iocb->private)
>  
>  #endif /* OCFS2_FILE_H */
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index 2d78f2863acf..a54f92633582 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -92,6 +92,13 @@ static int ocfs2_file_open(struct inode *inode, struct file *file)
>  			      file->f_path.dentry->d_name.len,
>  			      file->f_path.dentry->d_name.name, mode);
>  
> +	/*
> +	 * Direct I/O is served through iomap_dio_rw() from
> +	 * ocfs2_file_{read,write}_iter() rather than an a_ops->direct_IO
> +	 * method, so advertise O_DIRECT capability explicitly here.
> +	 */
> +	file->f_mode |= FMODE_CAN_ODIRECT;

This unconditionally set may include inodes with inline data.
Though it will handle the fallback in ocfs2_should_use_dio(), it looks odd
and slightly misleading to userspace. 

> +
>  	if (file->f_mode & FMODE_WRITE) {
>  		status = dquot_initialize(inode);
>  		if (status)
> @@ -1171,9 +1178,9 @@ int ocfs2_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>  	size_change = S_ISREG(inode->i_mode) && attr->ia_valid & ATTR_SIZE;
>  	if (size_change) {
>  		/*
> -		 * Here we should wait dio to finish before inode lock
> -		 * to avoid a deadlock between ocfs2_setattr() and
> -		 * ocfs2_dio_end_io_write()
> +		 * Here we should wait for in-flight direct I/O to finish
> +		 * before taking the inode lock, to avoid a deadlock between
> +		 * ocfs2_setattr() and direct I/O completion.
>  		 */
>  		inode_dio_wait(inode);
>  
> diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
> index b228b10b5e71..095f7ae5dded 100644
> --- a/fs/ocfs2/ocfs2.h
> +++ b/fs/ocfs2/ocfs2.h
> @@ -549,11 +549,6 @@ struct ocfs2_map_block {
>  	unsigned int flags;
>  };
>  
> -extern const struct iomap_ops ocfs2_iomap_ops;
> -extern const struct iomap_dio_ops ocfs2_iomap_dio_ops_r_pr;
> -extern const struct iomap_dio_ops ocfs2_iomap_dio_ops_w_pr;
> -extern const struct iomap_dio_ops ocfs2_iomap_dio_ops_w_ex;
> -
>  /* Flags used by ocfs2_map_blocks() */
>  #define OCFS2_GET_BLOCKS_CREATE	(0x0001)
>