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

[Jan Kara <jack@suse.cz>]

On Wed 12-11-25 08:21:28, Christoph Hellwig wrote:
> Completions for pure overwrites don't need to be deferred to a workqueue
> as there is no work to be done, or at least no work that needs a user
> context.  Set the IOMAP_DIO_INLINE_COMP by default for writes like we
> already do for reads, and the clear it for all the cases that actually
> do need a user context for completions to update the inode size or
> record updates to the logical to physical mapping.
> 
> I've audited all users of the ->end_io callback, and they only require
> user context for I/O that involves unwritten extents, COW, size
> extensions, or error handling and all those are still run from workqueue
> context.
> 
> This restores the behavior of the old pre-iomap direct I/O code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
...
> @@ -365,6 +374,16 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  			else
>  				dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
>  		}
> +
> +		/*
> +		 * 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. 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?

								Honza

> @@ -669,6 +691,12 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  				dio->flags |= IOMAP_DIO_WRITE_THROUGH;
>  		}
>  
> +		/*
> +		 * Inode size updates must to happen from process context.
> +		 */
> +		if (iomi.pos + iomi.len > dio->i_size)
> +			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
> +
>  		/*
>  		 * Try to invalidate cache pages for the range we are writing.
>  		 * If this invalidation fails, let the caller fall back to
> @@ -741,9 +769,14 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  	 * If all the writes we issued were already written through to the
>  	 * media, we don't need to flush the cache on IO completion. Clear the
>  	 * sync flag for this case.
> +	 *
> +	 * Otherwise clear the inline completion flag if any sync work is
> +	 * needed, as that needs to be performed from process context.
>  	 */
>  	if (dio->flags & IOMAP_DIO_WRITE_THROUGH)
>  		dio->flags &= ~IOMAP_DIO_NEED_SYNC;
> +	else if (dio->flags & IOMAP_DIO_NEED_SYNC)
> +		dio->flags &= ~IOMAP_DIO_INLINE_COMP;
>  
>  	/*
>  	 * We are about to drop our additional submission reference, which
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR