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@xxxxxxxxxx>
--D
> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
> ---
> 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
>
