| Message ID | 2e63549f6bce3442c27997fae83082f1c9f4e6c3.1635006010.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block optimisations | expand |
On 10/23/21 17:21, Pavel Begunkov wrote: > With addition of __blkdev_direct_IO_async(), __blkdev_direct_IO() now > serves only multio-bio I/O, which we don't poll. Now we can remove > anything related to I/O polling from it. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > block/fops.c | 20 +++----------------- > 1 file changed, 3 insertions(+), 17 deletions(-) > > diff --git a/block/fops.c b/block/fops.c > index 8800b0ad5c29..997904963a9d 100644 > --- a/block/fops.c > +++ b/block/fops.c > @@ -190,7 +190,6 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, > struct blk_plug plug; > struct blkdev_dio *dio; > struct bio *bio; > - bool do_poll = (iocb->ki_flags & IOCB_HIPRI); > bool is_read = (iov_iter_rw(iter) == READ), is_sync; > loff_t pos = iocb->ki_pos; > int ret = 0; > @@ -216,12 +215,7 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, > if (is_read && iter_is_iovec(iter)) > dio->flags |= DIO_SHOULD_DIRTY; > > - /* > - * Don't plug for HIPRI/polled IO, as those should go straight > - * to issue > - */ > - if (!(iocb->ki_flags & IOCB_HIPRI)) > - blk_start_plug(&plug); I'm not sure, do we want to leave it conditional here?
On 10/23/21 10:46 AM, Pavel Begunkov wrote: > On 10/23/21 17:21, Pavel Begunkov wrote: >> With addition of __blkdev_direct_IO_async(), __blkdev_direct_IO() now >> serves only multio-bio I/O, which we don't poll. Now we can remove >> anything related to I/O polling from it. >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> block/fops.c | 20 +++----------------- >> 1 file changed, 3 insertions(+), 17 deletions(-) >> >> diff --git a/block/fops.c b/block/fops.c >> index 8800b0ad5c29..997904963a9d 100644 >> --- a/block/fops.c >> +++ b/block/fops.c >> @@ -190,7 +190,6 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, >> struct blk_plug plug; >> struct blkdev_dio *dio; >> struct bio *bio; >> - bool do_poll = (iocb->ki_flags & IOCB_HIPRI); >> bool is_read = (iov_iter_rw(iter) == READ), is_sync; >> loff_t pos = iocb->ki_pos; >> int ret = 0; >> @@ -216,12 +215,7 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, >> if (is_read && iter_is_iovec(iter)) >> dio->flags |= DIO_SHOULD_DIRTY; >> >> - /* >> - * Don't plug for HIPRI/polled IO, as those should go straight >> - * to issue >> - */ >> - if (!(iocb->ki_flags & IOCB_HIPRI)) >> - blk_start_plug(&plug); > > I'm not sure, do we want to leave it conditional here? For async polled there's only one user and that user plug already...
On 10/24/21 16:09, Jens Axboe wrote: > On 10/23/21 10:46 AM, Pavel Begunkov wrote: >> On 10/23/21 17:21, Pavel Begunkov wrote: >>> With addition of __blkdev_direct_IO_async(), __blkdev_direct_IO() now >>> serves only multio-bio I/O, which we don't poll. Now we can remove >>> anything related to I/O polling from it. >>> >>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>> --- >>> block/fops.c | 20 +++----------------- >>> 1 file changed, 3 insertions(+), 17 deletions(-) >>> >>> diff --git a/block/fops.c b/block/fops.c >>> index 8800b0ad5c29..997904963a9d 100644 >>> --- a/block/fops.c >>> +++ b/block/fops.c >>> @@ -190,7 +190,6 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, >>> struct blk_plug plug; >>> struct blkdev_dio *dio; >>> struct bio *bio; >>> - bool do_poll = (iocb->ki_flags & IOCB_HIPRI); >>> bool is_read = (iov_iter_rw(iter) == READ), is_sync; >>> loff_t pos = iocb->ki_pos; >>> int ret = 0; >>> @@ -216,12 +215,7 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, >>> if (is_read && iter_is_iovec(iter)) >>> dio->flags |= DIO_SHOULD_DIRTY; >>> >>> - /* >>> - * Don't plug for HIPRI/polled IO, as those should go straight >>> - * to issue >>> - */ >>> - if (!(iocb->ki_flags & IOCB_HIPRI)) >>> - blk_start_plug(&plug); >> >> I'm not sure, do we want to leave it conditional here? > > For async polled there's only one user and that user plug already... It's __blkdev_direct_IO() though, covers both sync and async
On Oct 24, 2021, at 12:18 PM, Pavel Begunkov <asml.silence@gmail.com> wrote: > > On 10/24/21 16:09, Jens Axboe wrote: >>> On 10/23/21 10:46 AM, Pavel Begunkov wrote: >>> On 10/23/21 17:21, Pavel Begunkov wrote: >>>> With addition of __blkdev_direct_IO_async(), __blkdev_direct_IO() now >>>> serves only multio-bio I/O, which we don't poll. Now we can remove >>>> anything related to I/O polling from it. >>>> >>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>> --- >>>> block/fops.c | 20 +++----------------- >>>> 1 file changed, 3 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/block/fops.c b/block/fops.c >>>> index 8800b0ad5c29..997904963a9d 100644 >>>> --- a/block/fops.c >>>> +++ b/block/fops.c >>>> @@ -190,7 +190,6 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, >>>> struct blk_plug plug; >>>> struct blkdev_dio *dio; >>>> struct bio *bio; >>>> - bool do_poll = (iocb->ki_flags & IOCB_HIPRI); >>>> bool is_read = (iov_iter_rw(iter) == READ), is_sync; >>>> loff_t pos = iocb->ki_pos; >>>> int ret = 0; >>>> @@ -216,12 +215,7 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, >>>> if (is_read && iter_is_iovec(iter)) >>>> dio->flags |= DIO_SHOULD_DIRTY; >>>> - /* >>>> - * Don't plug for HIPRI/polled IO, as those should go straight >>>> - * to issue >>>> - */ >>>> - if (!(iocb->ki_flags & IOCB_HIPRI)) >>>> - blk_start_plug(&plug); >>> >>> I'm not sure, do we want to leave it conditional here? >> For async polled there's only one user and that user plug already... > > It's __blkdev_direct_IO() though, covers both sync and async Pointless to plug for sync, though.
On Sat, Oct 23, 2021 at 05:21:35PM +0100, Pavel Begunkov wrote: > With addition of __blkdev_direct_IO_async(), __blkdev_direct_IO() now > serves only multio-bio I/O, which we don't poll. Now we can remove > anything related to I/O polling from it. Looks good, but please also remove the entire non-multi bio support while you're at it.
On 10/25/21 08:35, Christoph Hellwig wrote: > On Sat, Oct 23, 2021 at 05:21:35PM +0100, Pavel Begunkov wrote: >> With addition of __blkdev_direct_IO_async(), __blkdev_direct_IO() now >> serves only multio-bio I/O, which we don't poll. Now we can remove >> anything related to I/O polling from it. > > Looks good, but please also remove the entire non-multi bio support > while you're at it. ok, will include into the series
On 10/25/21 11:12, Pavel Begunkov wrote: > On 10/25/21 08:35, Christoph Hellwig wrote: >> On Sat, Oct 23, 2021 at 05:21:35PM +0100, Pavel Begunkov wrote: >>> With addition of __blkdev_direct_IO_async(), __blkdev_direct_IO() now >>> serves only multio-bio I/O, which we don't poll. Now we can remove >>> anything related to I/O polling from it. >> >> Looks good, but please also remove the entire non-multi bio support >> while you're at it. > > ok, will include into the series You mean simplifying __blkdev_direct_IO(), right? E.g. as mentioned removing DIO_MULTI_BIO. There is also some potential for doing bio_iov_vecs_to_alloc() only once and doing some refactoring on top of it, but would need extra legwork that is better to go separately.
On Mon, Oct 25, 2021 at 11:27:12AM +0100, Pavel Begunkov wrote: > On 10/25/21 11:12, Pavel Begunkov wrote: > > On 10/25/21 08:35, Christoph Hellwig wrote: > > > On Sat, Oct 23, 2021 at 05:21:35PM +0100, Pavel Begunkov wrote: > > > > With addition of __blkdev_direct_IO_async(), __blkdev_direct_IO() now > > > > serves only multio-bio I/O, which we don't poll. Now we can remove > > > > anything related to I/O polling from it. > > > > > > Looks good, but please also remove the entire non-multi bio support > > > while you're at it. > > > > ok, will include into the series > > You mean simplifying __blkdev_direct_IO(), right? E.g. as mentioned > removing DIO_MULTI_BIO. Yes.
diff --git a/block/fops.c b/block/fops.c index 8800b0ad5c29..997904963a9d 100644 --- a/block/fops.c +++ b/block/fops.c @@ -190,7 +190,6 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, struct blk_plug plug; struct blkdev_dio *dio; struct bio *bio; - bool do_poll = (iocb->ki_flags & IOCB_HIPRI); bool is_read = (iov_iter_rw(iter) == READ), is_sync; loff_t pos = iocb->ki_pos; int ret = 0; @@ -216,12 +215,7 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, if (is_read && iter_is_iovec(iter)) dio->flags |= DIO_SHOULD_DIRTY; - /* - * Don't plug for HIPRI/polled IO, as those should go straight - * to issue - */ - if (!(iocb->ki_flags & IOCB_HIPRI)) - blk_start_plug(&plug); + blk_start_plug(&plug); for (;;) { bio_set_dev(bio, bdev); @@ -254,11 +248,7 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, nr_pages = bio_iov_vecs_to_alloc(iter, BIO_MAX_VECS); if (!nr_pages) { - if (do_poll) - bio_set_polled(bio, iocb); submit_bio(bio); - if (do_poll) - WRITE_ONCE(iocb->private, bio); break; } if (!(dio->flags & DIO_MULTI_BIO)) { @@ -271,7 +261,6 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, bio_get(bio); dio->flags |= DIO_MULTI_BIO; atomic_set(&dio->ref, 2); - do_poll = false; } else { atomic_inc(&dio->ref); } @@ -280,8 +269,7 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, bio = bio_alloc(GFP_KERNEL, nr_pages); } - if (!(iocb->ki_flags & IOCB_HIPRI)) - blk_finish_plug(&plug); + blk_finish_plug(&plug); if (!is_sync) return -EIOCBQUEUED; @@ -290,9 +278,7 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, set_current_state(TASK_UNINTERRUPTIBLE); if (!READ_ONCE(dio->waiter)) break; - - if (!do_poll || !bio_poll(bio, NULL, 0)) - blk_io_schedule(); + blk_io_schedule(); } __set_current_state(TASK_RUNNING);
With addition of __blkdev_direct_IO_async(), __blkdev_direct_IO() now serves only multio-bio I/O, which we don't poll. Now we can remove anything related to I/O polling from it. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- block/fops.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-)