Re: [PATCH 4/5] iomap: support write completions from interrupt context

[Jan Kara <jack@suse.cz>]

On Thu 13-11-25 07:50:55, Christoph Hellwig wrote:
> On Wed, Nov 12, 2025 at 09:25:58PM +0100, Jan Kara wrote:
> > > +
> > > +		/*
> > > +		 * We can only do inline completion for pure overwrites that
> > > +		 * don't require additional I/O at completion time.
> > > +		 *
> > > +		 * This rules out writes that need zeroing or extent conversion,
> > > +		 * or extend the file size.
> > > +		 */
> > > +		if (!iomap_dio_is_overwrite(iomap))
> > > +			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
> > >  	} else {
> > >  		bio_opf |= REQ_OP_READ;
> > >  	}
> > 
> > OK, now I see why you wrote iomap_dio_is_overwrite() the way you did. You
> > still want to keep completions inline for overwrites of possibly
> > uncommitted extents.
> 
> Yes.
> 
> > But I have to admit it all seems somewhat fragile and
> > difficult to follow. Can't we just check for IOMAP_DIO_UNWRITTEN |
> > IOMAP_DIO_COW | IOMAP_DIO_NEED_SYNC in flags (plus the i_size check) and be
> > done with it?
> 
> You mean drop the common helper?  How would that be better and less
> fragile?   Note that I care strongly, but I don't really see the point.

Sorry I was a bit terse. What I meant is that the two users of
iomap_dio_is_overwrite() actually care about different things and that
results in that function having a bit odd semantics IMHO. The first user
wants to figure out whether calling generic_write_sync() is needed upon io
completion to make data persistent (crash safe). The second user cares
whether we need to do metadata modifications upon io completion to make data
visible at all.

So it would be IMO more understandable if we had one helper like
iomap_dio_extent_stable() for the first case (which would be the same as
current iomap_dio_is_overwrite() + IOMAP_F_DIRTY) and for the second case
we already gather that information from extents in DIO flags and
filesystems do the non-trivial work in their end_io handlers based on these
flags so just checking IOMAP_DIO_UNWRITTEN | IOMAP_DIO_COW |
IOMAP_DIO_NEED_SYNC in iomap_dio_complete() should be a reliable trigger
for offloading to a workqueue (plus the check for dio->error). That way the
handling of IOMAP_DIO_INLINE_COMP happens only before we start IO (i_size
check) and in iomap_dio_complete() which seems easier to follow to me.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR