| Message ID | 20230720181310.71589-6-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Improve async iomap DIO performance | expand |
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. Nice, with the current shape of the code poll_bio can indeed go away. Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
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 >
On 7/21/23 9:35?AM, Darrick J. Wong wrote: > 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 Correct, yes to both. For multi bio or not a straight overwrite, iomap disables polling. > Reviewed-by: Darrick J. Wong <djwong@kernel.org> Thanks!
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
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. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- fs/iomap/direct-io.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)