Re: [PATCH 5/8] iomap: optionally use ioends for direct I/O

[Christoph Hellwig <hch@lst.de>]

On Thu, Dec 12, 2024 at 08:29:36AM -0500, Brian Foster wrote:
> > +	bool should_dirty = (dio->flags & IOMAP_DIO_DIRTY);
> > +	struct kiocb *iocb = dio->iocb;
> > +	u32 vec_count = ioend->io_bio.bi_vcnt;
> > +
> > +	if (ioend->io_error)
> > +		iomap_dio_set_error(dio, ioend->io_error);
> > +
> > +	if (atomic_dec_and_test(&dio->ref)) {
> > +		struct inode *inode = file_inode(iocb->ki_filp);
> > +
> > +		if (dio->wait_for_completion) {
> > +			struct task_struct *waiter = dio->submit.waiter;
> > +
> > +			WRITE_ONCE(dio->submit.waiter, NULL);
> > +			blk_wake_io_task(waiter);
> > +		} else if (!inode->i_mapping->nrpages) {
> > +			WRITE_ONCE(iocb->private, NULL);
> > +
> > +			/*
> > +			 * We must never invalidate pages from this thread to
> > +			 * avoid deadlocks with buffered I/O completions.
> > +			 * Tough luck if you hit the tiny race with someone
> > +			 * dirtying the range now.
> > +			 */
> > +			dio->flags |= IOMAP_DIO_NO_INVALIDATE;
> > +			iomap_dio_complete_work(&dio->aio.work);
> > +		} else {
> > +			INIT_WORK(&dio->aio.work, iomap_dio_complete_work);
> > +			queue_work(inode->i_sb->s_dio_done_wq, &dio->aio.work);
> > +		}
> > +	}
> > +
> > +	if (should_dirty) {
> > +		bio_check_pages_dirty(&ioend->io_bio);
> > +	} else {
> > +		bio_release_pages(&ioend->io_bio, false);
> > +		bio_put(&ioend->io_bio);
> > +	}
> > +
> 
> Not that it matters all that much, but I'm a little curious about the
> reasoning for using vec_count here. AFAICS this correlates to per-folio
> writeback completions for buffered I/O, but that doesn't seem to apply
> to direct I/O. Is there a reason to have the caller throttle based on
> vec_counts, or are we just pulling some non-zero value for consistency
> sake?

So direct I/O also iterates over all folios for the bio, to unpin,
and in case of reads dirty all of them.

I wanted to plug something useful into cond_resched condition in the
caller.  Now number of bvecs isn't the number of folios as we can
physically merge outside the folio context, but I think this is about
as goot as it gets without changing the block code to return the
number of folios processed from __bio_release_pages and
bio_check_pages_dirty.