Re: [PATCH 06/10] iomap: factor out a iomap_dio_done helper

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

On Thu, Dec 19, 2024 at 05:39:11PM +0000, Christoph Hellwig wrote:
> Split out the struct iomap-dio level final completion from
> iomap_dio_bio_end_io into a helper to clean up the code and make it
> reusable.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/iomap/direct-io.c | 76 ++++++++++++++++++++++----------------------
>  1 file changed, 38 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 641649a04614..ed658eb09a1a 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -165,43 +165,31 @@ static inline void iomap_dio_set_error(struct iomap_dio *dio, int ret)
>  	cmpxchg(&dio->error, 0, ret);
>  }
>  
> -void iomap_dio_bio_end_io(struct bio *bio)
> +/*
> + * Called when dio->ref reaches zero from and I/O completion.

                                             an I/O completion

This hoist looks fine to me, so with that fixed:
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> + */
> +static void iomap_dio_done(struct iomap_dio *dio)
>  {
> -	struct iomap_dio *dio = bio->bi_private;
> -	bool should_dirty = (dio->flags & IOMAP_DIO_DIRTY);
>  	struct kiocb *iocb = dio->iocb;
>  
> -	if (bio->bi_status)
> -		iomap_dio_set_error(dio, blk_status_to_errno(bio->bi_status));
> -	if (!atomic_dec_and_test(&dio->ref))
> -		goto release_bio;
> -
> -	/*
> -	 * Synchronous dio, task itself will handle any completion work
> -	 * that needs after IO. All we need to do is wake the task.
> -	 */
>  	if (dio->wait_for_completion) {
> +		/*
> +		 * Synchronous I/O, task itself will handle any completion work
> +		 * that needs after IO. All we need to do is wake the task.
> +		 */
>  		struct task_struct *waiter = dio->submit.waiter;
>  
>  		WRITE_ONCE(dio->submit.waiter, NULL);
>  		blk_wake_io_task(waiter);
> -		goto release_bio;
> -	}
> -
> -	/*
> -	 * Flagged with IOMAP_DIO_INLINE_COMP, we can complete it inline
> -	 */
> -	if (dio->flags & IOMAP_DIO_INLINE_COMP) {
> +	} else if (dio->flags & IOMAP_DIO_INLINE_COMP) {
>  		WRITE_ONCE(iocb->private, NULL);
>  		iomap_dio_complete_work(&dio->aio.work);
> -		goto release_bio;
> -	}
> -
> -	/*
> -	 * If this dio is flagged with IOMAP_DIO_CALLER_COMP, then schedule
> -	 * our completion that way to avoid an async punt to a workqueue.
> -	 */
> -	if (dio->flags & IOMAP_DIO_CALLER_COMP) {
> +	} else if (dio->flags & IOMAP_DIO_CALLER_COMP) {
> +		/*
> +		 * If this dio is flagged with IOMAP_DIO_CALLER_COMP, then
> +		 * schedule our completion that way to avoid an async punt to a
> +		 * workqueue.
> +		 */
>  		/* only polled IO cares about private cleared */
>  		iocb->private = dio;
>  		iocb->dio_complete = iomap_dio_deferred_complete;
> @@ -219,19 +207,31 @@ void iomap_dio_bio_end_io(struct bio *bio)
>  		 * issuer.
>  		 */
>  		iocb->ki_complete(iocb, 0);
> -		goto release_bio;
> +	} else {
> +		struct inode *inode = file_inode(iocb->ki_filp);
> +
> +		/*
> +		 * Async DIO completion that requires filesystem level
> +		 * completion work gets punted to a work queue to complete as
> +		 * the operation may require more IO to be issued to finalise
> +		 * filesystem metadata changes or guarantee data integrity.
> +		 */
> +		INIT_WORK(&dio->aio.work, iomap_dio_complete_work);
> +		queue_work(inode->i_sb->s_dio_done_wq, &dio->aio.work);
>  	}
> +}
> +
> +void iomap_dio_bio_end_io(struct bio *bio)
> +{
> +	struct iomap_dio *dio = bio->bi_private;
> +	bool should_dirty = (dio->flags & IOMAP_DIO_DIRTY);
> +
> +	if (bio->bi_status)
> +		iomap_dio_set_error(dio, blk_status_to_errno(bio->bi_status));
> +
> +	if (atomic_dec_and_test(&dio->ref))
> +		iomap_dio_done(dio);
>  
> -	/*
> -	 * Async DIO completion that requires filesystem level completion work
> -	 * gets punted to a work queue to complete as the operation may require
> -	 * more IO to be issued to finalise filesystem metadata changes or
> -	 * guarantee data integrity.
> -	 */
> -	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);
> -release_bio:
>  	if (should_dirty) {
>  		bio_check_pages_dirty(bio);
>  	} else {
> -- 
> 2.45.2
> 
>