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

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

Sorry for the late reply, I am fighting with customer bug this week.

On Tue, Jul 28, 2026 at 06:16:57AM +0200, Christoph Hellwig wrote:
> On Mon, Jul 27, 2026 at 02:17:58PM +0800, Heming Zhao wrote:
> > This patch migrates the DIO read path from the legacy buffer_head
> 
> Please never start commit messages with "This patch", that context
> is implied already.

OK.
> 
> > The rw cluster-lock level is not stashed in iocb->private: iomap DIO owns
> 
> Explaining what you don't do is always a bit odd, I'd start with what
> you do, and only really mention what not later if really needed.

Will change in next version.
> 
> > that field (it stores a bio there for polled I/O and unconditionally
> > clears it before calling ->end_io), so any side-channel packed into
> > iocb->private is corrupted and would trip the end_io lock-state check.
> > Instead the unlock level is encoded by which iomap_dio_ops is passed -
> > ocfs2_iomap_dio_ops_pr (PRMODE) or ocfs2_iomap_dio_ops_ex (EXMODE) - and
> > 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).
> 
> .. and a lot of this just seems to be about the locking, but very little
> about the iomap changes.

Will change in next version.
> 
> > +	if (flags & IOMAP_WRITE) {
> > +		/* todo */
> 
> should this be a WARN_ON for now?

Will follow Joseph's review comment, replace with "return -NOTSUPP".

> 
> > +	if (direct_io && ocfs2_should_use_dio(iocb, to, inode)) {
> > +		struct iomap_dio *dio;
> > +
> > +		dio = __iomap_dio_rw(iocb, to, &ocfs2_iomap_ops,
> > +				&ocfs2_iomap_dio_ops_r_pr, 0, NULL, 0);
> > +		if (dio == NULL) {
> > +			/* No I/O issued; rw_lock still held. */
> > +			ret = 0;
> > +		} else if (IS_ERR(dio)) {
> > +			ret = PTR_ERR(dio);
> > +			if (ret == -EIOCBQUEUED)
> > +				rw_level = -1;
> > +		} else {
> > +			ret = iomap_dio_complete(dio);
> > +			if (ret != 0)
> > +				rw_level = -1;
> > +		}
> 
> What information is missing if you use plain iomap_dio_rw here?

ocfs2 should release the cluster lock in end_dio flow, where "rw_level = -1"
indicates that end_dio is responsible for releasing it.
Btw, I previously tried using iomap_dio_rw() here, but found that some xfstests
triggered crashes because of this cluster lock issue. The current code (idea) is
based on Joseph's patch, and the code work fine.

> 
> > +		/*
> > +		 * A 0 result means the mapping bounced us back to buffered I/O
> > +		 * (e.g. inline data); the rw_lock is still held. Clear
> > +		 * IOCB_DIRECT so generic_file_read_iter() takes the buffered
> > +		 * path rather than re-entering direct I/O.
> > +		 */
> > +		if (ret == 0) {
> > +			iocb->ki_flags &= ~IOCB_DIRECT;
> > +			ret = generic_file_read_iter(iocb, to);
> > +		}
> > +	} else {
> > +		iocb->ki_flags &= ~IOCB_DIRECT;
> > +		ret = generic_file_read_iter(iocb, to);
> > +	}
> 
> Can you share this code somehow?
> 

I can't follow what you mean. What code do you want to see?

Thanks,
Heming