Re: [PATCH 5/8] iomap: only set iocb->private for polled bio

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Jul 20, 2023 at 12:13:07PM -0600, Jens Axboe wrote:
> iocb->private is only used for polled IO, where the completer will
> find the bio to poll through that field.
> 
> Assign it when we're submitting a polled bio, and get rid of the
> dio->poll_bio indirection.

IIRC, the only time iomap actually honors HIPRI requests from the iocb
is if the entire write can be satisfied with a single bio -- no zeroing
around, no dirty file metadata, no writes past EOF, no unwritten blocks,
etc.  Right?

There was only ever going to be one assign to dio->submit.poll_bio,
which means the WRITE_ONCE isn't going to overwrite some non-NULL value.
Correct?

All this does is remove the indirection like you said.

If the answers are {yes, yes} then I understand the HIPRI mechanism
enough to say

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  fs/iomap/direct-io.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index c3ea1839628f..cce9af019705 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -42,7 +42,6 @@ struct iomap_dio {
>  		struct {
>  			struct iov_iter		*iter;
>  			struct task_struct	*waiter;
> -			struct bio		*poll_bio;
>  		} submit;
>  
>  		/* used for aio completion: */
> @@ -64,12 +63,14 @@ static struct bio *iomap_dio_alloc_bio(const struct iomap_iter *iter,
>  static void iomap_dio_submit_bio(const struct iomap_iter *iter,
>  		struct iomap_dio *dio, struct bio *bio, loff_t pos)
>  {
> +	struct kiocb *iocb = dio->iocb;
> +
>  	atomic_inc(&dio->ref);
>  
>  	/* Sync dio can't be polled reliably */
> -	if ((dio->iocb->ki_flags & IOCB_HIPRI) && !is_sync_kiocb(dio->iocb)) {
> -		bio_set_polled(bio, dio->iocb);
> -		dio->submit.poll_bio = bio;
> +	if ((iocb->ki_flags & IOCB_HIPRI) && !is_sync_kiocb(iocb)) {
> +		bio_set_polled(bio, iocb);
> +		WRITE_ONCE(iocb->private, bio);
>  	}
>  
>  	if (dio->dops && dio->dops->submit_io)
> @@ -197,7 +198,6 @@ void iomap_dio_bio_end_io(struct bio *bio)
>  	 * more IO to be issued to finalise filesystem metadata changes or
>  	 * guarantee data integrity.
>  	 */
> -	WRITE_ONCE(iocb->private, NULL);
>  	INIT_WORK(&dio->aio.work, iomap_dio_complete_work);
>  	queue_work(file_inode(iocb->ki_filp)->i_sb->s_dio_done_wq,
>  			&dio->aio.work);
> @@ -536,7 +536,6 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  
>  	dio->submit.iter = iter;
>  	dio->submit.waiter = current;
> -	dio->submit.poll_bio = NULL;
>  
>  	if (iocb->ki_flags & IOCB_NOWAIT)
>  		iomi.flags |= IOMAP_NOWAIT;
> @@ -648,8 +647,6 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  	if (dio->flags & IOMAP_DIO_STABLE_WRITE)
>  		dio->flags &= ~IOMAP_DIO_NEED_SYNC;
>  
> -	WRITE_ONCE(iocb->private, dio->submit.poll_bio);
> -
>  	/*
>  	 * We are about to drop our additional submission reference, which
>  	 * might be the last reference to the dio.  There are three different
> -- 
> 2.40.1
>