block optimisations

PATCH Applied Pavel Begunkov 3 participants 21 messages Oct 23, 2021 – Oct 27, 2021 Block layer

Share & tools

Pavel Begunkov

author

2021-10-23 16:21 UTC #0 lore raw

cover block optimisations

Another split off from the "block optimisation round" series focusing on fops.c. It addresses Cristoph's review, the main change is setting bvec directly instead of passing a "hint"
to bio_iov_iter_get_pages() in 3/5, and a prep patch 2/5.

I don't find a good way how to deduplicate __blkdev_direct_IO_async(), there are small differences in implementation. If that's fine I'd suggest to do it afterwards, anyway I want to brush up __blkdev_direct_IO(), e.g. remove effectively unnecessary DIO_MULTI_BIO flag.

Performance stays the same, solely 1/5 gives ~4.5 -> 4.7 MIOPS, plus 1-2% from 3/5.

Pavel Begunkov (5):
block: add single bio async direct IO helper
block: refactor bio_iov_bvec_set()
block: avoid extra iter advance with async iocb
block: kill unused polling bits in __blkdev_direct_IO()
block: add async version of bio_set_polled

 block/bio.c         |  37 ++++++--------
 block/fops.c        | 116 ++++++++++++++++++++++++++++++++++++--------
 include/linux/bio.h |   6 +++
 3 files changed, 116 insertions(+), 43 deletions(-)

Signature · 1 line

2.33.1

Pavel Begunkov

author

2021-10-23 16:21 UTC #1 lore raw

PATCH 1/5 block: add single bio async direct IO helper

As with __blkdev_direct_IO_simple(), we can implement direct IO more efficiently if there is only one bio. Add __blkdev_direct_IO_async() and blkdev_bio_end_io_async(). This patch brings me from 4.45-4.5 MIOPS with nullblk to 4.7+.

Signed-off-by Pavel Begunkov <[email protected]>

---
 block/fops.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 3 deletions(-)

block/fops.c +84 -3

index 396537598e3e..a7b328296912 100644
@@ -305,6 +305,85 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
 	return ret;
 }
 
+static void blkdev_bio_end_io_async(struct bio *bio)
+{
+	struct blkdev_dio *dio = container_of(bio, struct blkdev_dio, bio);
+	struct kiocb *iocb = dio->iocb;
+	ssize_t ret;
+
+	if (likely(!bio->bi_status)) {
+		ret = dio->size;
+		iocb->ki_pos += ret;
+	} else {
+		ret = blk_status_to_errno(bio->bi_status);
+	}
+
+	iocb->ki_complete(iocb, ret, 0);
+
+	if (dio->flags & DIO_SHOULD_DIRTY) {
+		bio_check_pages_dirty(bio);
+	} else {
+		bio_release_pages(bio, false);
+		bio_put(bio);
+	}
+}
+
+static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
+					struct iov_iter *iter,
+					unsigned int nr_pages)
+{
+	struct block_device *bdev = iocb->ki_filp->private_data;
+	struct blkdev_dio *dio;
+	struct bio *bio;
+	loff_t pos = iocb->ki_pos;
+	int ret = 0;
+
+	if ((pos | iov_iter_alignment(iter)) &
+	    (bdev_logical_block_size(bdev) - 1))
+		return -EINVAL;
+
+	bio = bio_alloc_kiocb(iocb, nr_pages, &blkdev_dio_pool);
+	dio = container_of(bio, struct blkdev_dio, bio);
+	dio->flags = 0;
+	dio->iocb = iocb;
+	bio_set_dev(bio, bdev);
+	bio->bi_iter.bi_sector = pos >> SECTOR_SHIFT;
+	bio->bi_write_hint = iocb->ki_hint;
+	bio->bi_end_io = blkdev_bio_end_io_async;
+	bio->bi_ioprio = iocb->ki_ioprio;
+
+	ret = bio_iov_iter_get_pages(bio, iter);
+	if (unlikely(ret)) {
+		bio->bi_status = BLK_STS_IOERR;
+		bio_endio(bio);
+		return ret;
+	}
+	dio->size = bio->bi_iter.bi_size;
+
+	if (iov_iter_rw(iter) == READ) {
+		bio->bi_opf = REQ_OP_READ;
+		if (iter_is_iovec(iter)) {
+			dio->flags |= DIO_SHOULD_DIRTY;
+			bio_set_pages_dirty(bio);
+		}
+	} else {
+		bio->bi_opf = dio_bio_write_op(iocb);
+		task_io_account_write(bio->bi_iter.bi_size);
+	}
+
+	if (iocb->ki_flags & IOCB_NOWAIT)
+		bio->bi_opf |= REQ_NOWAIT;
+
+	if (iocb->ki_flags & IOCB_HIPRI) {
+		bio_set_polled(bio, iocb);
+		submit_bio(bio);
+		WRITE_ONCE(iocb->private, bio);
+	} else {
+		submit_bio(bio);
+	}
+	return -EIOCBQUEUED;
+}
+
 static ssize_t blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
 	unsigned int nr_pages;

@@ -313,9 +392,11 @@ static ssize_t blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 		return 0;
 
 	nr_pages = bio_iov_vecs_to_alloc(iter, BIO_MAX_VECS + 1);
-	if (is_sync_kiocb(iocb) && nr_pages <= BIO_MAX_VECS)
-		return __blkdev_direct_IO_simple(iocb, iter, nr_pages);
-
+	if (likely(nr_pages <= BIO_MAX_VECS)) {
+		if (is_sync_kiocb(iocb))
+			return __blkdev_direct_IO_simple(iocb, iter, nr_pages);
+		return __blkdev_direct_IO_async(iocb, iter, nr_pages);
+	}
 	return __blkdev_direct_IO(iocb, iter, bio_max_segs(nr_pages));
 }

Signature · 1 line

2.33.1

Pavel Begunkov

author

2021-10-23 16:21 UTC #2 lore raw

PATCH 2/5 block: refactor bio_iov_bvec_set()

In reply to Pavel Begunkov

Combine bio_iov_bvec_set() and bio_iov_bvec_set_append() and let the caller to do iov_iter_advance(). Also get rid of __bio_iov_bvec_set(), which was duplicated in the final binary, and replace a weird iov_iter_truncate() of a temporal iter copy with min() better reflecting the intention.

Signed-off-by Pavel Begunkov <[email protected]>

---
 block/bio.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

block/bio.c +14 ~~-23~~

index 46a87c72d2b4..ead1f8a9ff5e 100644
@@ -1046,36 +1046,27 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
 }
 EXPORT_SYMBOL_GPL(__bio_release_pages);
 
-static void __bio_iov_bvec_set(struct bio *bio, struct iov_iter *iter)
+static void bio_iov_bvec_set(struct bio *bio, struct iov_iter *iter)
 {
+	size_t size = iov_iter_count(iter);
+
 	WARN_ON_ONCE(bio->bi_max_vecs);
 
+	if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
+		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
+		size_t max_sectors = queue_max_zone_append_sectors(q);
+
+		size = min(size, max_sectors << SECTOR_SHIFT);
+	}
+
 	bio->bi_vcnt = iter->nr_segs;
 	bio->bi_io_vec = (struct bio_vec *)iter->bvec;
 	bio->bi_iter.bi_bvec_done = iter->iov_offset;
-	bio->bi_iter.bi_size = iter->count;
+	bio->bi_iter.bi_size = size;
 	bio_set_flag(bio, BIO_NO_PAGE_REF);
 	bio_set_flag(bio, BIO_CLONED);
 }
 
-static int bio_iov_bvec_set(struct bio *bio, struct iov_iter *iter)
-{
-	__bio_iov_bvec_set(bio, iter);
-	iov_iter_advance(iter, iter->count);
-	return 0;
-}
-
-static int bio_iov_bvec_set_append(struct bio *bio, struct iov_iter *iter)
-{
-	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
-	struct iov_iter i = *iter;
-
-	iov_iter_truncate(&i, queue_max_zone_append_sectors(q) << 9);
-	__bio_iov_bvec_set(bio, &i);
-	iov_iter_advance(iter, i.count);
-	return 0;
-}
-
 static void bio_put_pages(struct page **pages, size_t size, size_t off)
 {
 	size_t i, nr = DIV_ROUND_UP(size + (off & ~PAGE_MASK), PAGE_SIZE);

@@ -1217,9 +1208,9 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 	int ret = 0;
 
 	if (iov_iter_is_bvec(iter)) {
-		if (bio_op(bio) == REQ_OP_ZONE_APPEND)
-			return bio_iov_bvec_set_append(bio, iter);
-		return bio_iov_bvec_set(bio, iter);
+		bio_iov_bvec_set(bio, iter);
+		iov_iter_advance(iter, bio->bi_iter.bi_size);
+		return 0;
 	}
 
 	do {

Signature · 1 line

2.33.1

Pavel Begunkov

author

2021-10-23 16:21 UTC #4 lore raw

PATCH 3/5 block: avoid extra iter advance with async iocb

In reply to Pavel Begunkov

Nobody cares about iov iterators state if we return -EIOCBQUEUED, so as the we now have __blkdev_direct_IO_async(), which gets pages only once, we can skip expensive iov_iter_advance(). It's around 1-2% of all CPU spent.

Signed-off-by Pavel Begunkov <[email protected]>

---
 block/bio.c         |  2 +-
 block/fops.c        | 20 +++++++++++++++-----
 include/linux/bio.h |  1 +
 3 files changed, 17 insertions(+), 6 deletions(-)

block/bio.c +1 -1

index ead1f8a9ff5e..15ab0d6d1c06 100644
@@ -1046,7 +1046,7 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
 }
 EXPORT_SYMBOL_GPL(__bio_release_pages);
 
-static void bio_iov_bvec_set(struct bio *bio, struct iov_iter *iter)
+void bio_iov_bvec_set(struct bio *bio, struct iov_iter *iter)
 {
 	size_t size = iov_iter_count(iter);

block/fops.c +15 -5

index a7b328296912..8800b0ad5c29 100644
@@ -352,11 +352,21 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
 	bio->bi_end_io = blkdev_bio_end_io_async;
 	bio->bi_ioprio = iocb->ki_ioprio;
 
-	ret = bio_iov_iter_get_pages(bio, iter);
-	if (unlikely(ret)) {
-		bio->bi_status = BLK_STS_IOERR;
-		bio_endio(bio);
-		return ret;
+	if (!iov_iter_is_bvec(iter)) {
+		ret = bio_iov_iter_get_pages(bio, iter);
+		if (unlikely(ret)) {
+			bio->bi_status = BLK_STS_IOERR;
+			bio_endio(bio);
+			return ret;
+		}
+	} else {
+		/*
+		 * Users don't rely on the iterator being in any particular
+		 * state for async I/O returning -EIOCBQUEUED, hence we can
+		 * avoid expensive iov_iter_advance(). Bypass
+		 * bio_iov_iter_get_pages() and set the bvec directly.
+		 */
+		bio_iov_bvec_set(bio, iter);
 	}
 	dio->size = bio->bi_iter.bi_size;

include/linux/bio.h +1 -0

index c88700d1bdc3..fe6bdfbbef66 100644
@@ -417,6 +417,7 @@ int bio_add_zone_append_page(struct bio *bio, struct page *page,
 void __bio_add_page(struct bio *bio, struct page *page,
 		unsigned int len, unsigned int off);
 int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter);
+void bio_iov_bvec_set(struct bio *bio, struct iov_iter *iter);
 void __bio_release_pages(struct bio *bio, bool mark_dirty);
 extern void bio_set_pages_dirty(struct bio *bio);
 extern void bio_check_pages_dirty(struct bio *bio);

Signature · 1 line

2.33.1

Pavel Begunkov

author

2021-10-23 16:21 UTC #7 lore raw

PATCH 4/5 block: kill unused polling bits in __blkdev_direct_IO()

In reply to Pavel Begunkov

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 <[email protected]>

---
 block/fops.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

block/fops.c +3 ~~-17~~

index 8800b0ad5c29..997904963a9d 100644
@@ -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);

Signature · 1 line

2.33.1

Pavel Begunkov

author

2021-10-23 16:21 UTC #16 lore raw

PATCH 5/5 block: add async version of bio_set_polled

In reply to Pavel Begunkov

If we know that a iocb is async we can optimise bio_set_polled() a bit, add a new helper bio_set_polled_async().

Signed-off-by Pavel Begunkov <[email protected]>

---
 block/fops.c        | 7 +++----
 include/linux/bio.h | 5 +++++
 2 files changed, 8 insertions(+), 4 deletions(-)

block/fops.c +3 -4

index 997904963a9d..9cb436de92bb 100644
@@ -367,14 +367,13 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
 		task_io_account_write(bio->bi_iter.bi_size);
 	}
 
-	if (iocb->ki_flags & IOCB_NOWAIT)
-		bio->bi_opf |= REQ_NOWAIT;
-
 	if (iocb->ki_flags & IOCB_HIPRI) {
-		bio_set_polled(bio, iocb);
+		bio_set_polled_async(bio, iocb);
 		submit_bio(bio);
 		WRITE_ONCE(iocb->private, bio);
 	} else {
+		if (iocb->ki_flags & IOCB_NOWAIT)
+			bio->bi_opf |= REQ_NOWAIT;
 		submit_bio(bio);
 	}
 	return -EIOCBQUEUED;

include/linux/bio.h +5 -0

index fe6bdfbbef66..b64161473f3e 100644
@@ -738,6 +738,11 @@ static inline void bio_set_polled(struct bio *bio, struct kiocb *kiocb)
 		bio->bi_opf |= REQ_NOWAIT;
 }
 
+static inline void bio_set_polled_async(struct bio *bio, struct kiocb *kiocb)
+{
+	bio->bi_opf |= REQ_POLLED | REQ_NOWAIT;
+}
+
 struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp);
 
 #endif /* __LINUX_BIO_H */

Signature · 1 line

2.33.1

Pavel Begunkov

author

2021-10-23 16:46 UTC #8 lore raw

Re: [PATCH 4/5] block: kill unused polling bits in __blkdev_direct_IO()

In reply to Pavel Begunkov

Quoted 22 lines · Pavel Begunkov

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 <[email protected]>
---
  block/fops.c | 20 +++-----------------
  1 file changed, 3 insertions(+), 17 deletions(-)
block/fops.c +0 -1
index 8800b0ad5c29..997904963a9d 100644
@@ -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;

block/fops.c static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, · line 224

Quoted 9 lines

block/fops.c +0 -6

@@ -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?

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2021-10-24 15:10 UTC #9 lore raw

Re: [PATCH 4/5] block: kill unused polling bits in __blkdev_direct_IO()

Quoted 26 lines · Pavel Begunkov

Quoted 25 lines · Pavel Begunkov

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 <[email protected]>

---
  block/fops.c | 20 +++-----------------
  1 file changed, 3 insertions(+), 17 deletions(-)

block/fops.c +0 -1

index 8800b0ad5c29..997904963a9d 100644
@@ -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;

Quoted 8 lines

block/fops.c static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, · line 224
Quoted 6 lines
block/fops.c +0 -6
-	/*
-	 * 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...

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2021-10-24 17:18 UTC #10 lore raw

Re: [PATCH 4/5] block: kill unused polling bits in __blkdev_direct_IO()

Quoted 36 lines · Jens Axboe

Quoted 26 lines · Pavel Begunkov
Quoted 25 lines · Pavel Begunkov
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 <[email protected]>
---
   block/fops.c | 20 +++-----------------
   1 file changed, 3 insertions(+), 17 deletions(-)
block/fops.c +0 -1
index 8800b0ad5c29..997904963a9d 100644
@@ -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;
 
Quoted 8 lines
block/fops.c static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, · line 224
Quoted 6 lines
block/fops.c +0 -6
-	/*
-	 * 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?

Quoted 1 line

For async polled there's only one user and that user plug already...

It's __blkdev_direct_IO() though, covers both sync and async

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2021-10-24 18:11 UTC #11 lore raw

Re: [PATCH 4/5] block: kill unused polling bits in __blkdev_direct_IO()

Quoted 36 lines · Pavel Begunkov

Quoted 34 lines · Jens Axboe
Quoted 34 lines · Pavel Begunkov
Quoted 22 lines · Pavel Begunkov
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 <[email protected]>
---
  block/fops.c | 20 +++-----------------
  1 file changed, 3 insertions(+), 17 deletions(-)
block/fops.c +0 -1
index 8800b0ad5c29..997904963a9d 100644
@@ -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;
block/fops.c static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, · line 223
Quoted 8 lines
block/fops.c +0 -5
@@ -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?

Quoted 3 lines

Quoted 1 line
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.

Christoph Hellwig

2021-10-25 07:28 UTC #19 lore raw

In reply to Pavel Begunkov

Quoted 5 lines · Pavel Begunkov

I don't find a good way how to deduplicate __blkdev_direct_IO_async(), there are small differences in implementation. If that's fine I'd suggest to do it afterwards, anyway I want to brush up __blkdev_direct_IO(), e.g. remove effectively unnecessary DIO_MULTI_BIO flag.

If we add another code path we should at last drop the DIO_MULTI_BIO code to keep the complexity down in that function.

Christoph Hellwig

Reviewed-by on v1 2/5

2021-10-25 07:31 UTC #3 lore raw

Re: [PATCH 2/5] block: refactor bio_iov_bvec_set()

In reply to Pavel Begunkov

Quoted 7 lines · Pavel Begunkov

Combine bio_iov_bvec_set() and bio_iov_bvec_set_append() and let the caller to do iov_iter_advance(). Also get rid of __bio_iov_bvec_set(), which was duplicated in the final binary, and replace a weird iov_iter_truncate() of a temporal iter copy with min() better reflecting the intention.
Signed-off-by Pavel Begunkov <[email protected]>

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

Christoph Hellwig

2021-10-25 07:33 UTC #5 lore raw

Re: [PATCH 3/5] block: avoid extra iter advance with async iocb

In reply to Pavel Begunkov

Quoted 8 lines · Pavel Begunkov

block/fops.c +0 -2

@@ -352,11 +352,21 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
 	bio->bi_end_io = blkdev_bio_end_io_async;
 	bio->bi_ioprio = iocb->ki_ioprio;
 
-	ret = bio_iov_iter_get_pages(bio, iter);
-	if (unlikely(ret)) {

block/fops.c static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb, · line 361

Quoted 10 lines

block/fops.c +7 -3

-		bio->bi_status = BLK_STS_IOERR;
-		bio_endio(bio);
-		return ret;
+	if (!iov_iter_is_bvec(iter)) {
+		ret = bio_iov_iter_get_pages(bio, iter);
+		if (unlikely(ret)) {
+			bio->bi_status = BLK_STS_IOERR;
+			bio_endio(bio);
+			return ret;
+		}

Nit: I generally find it much nicer to read if simple if statements don't use pointless negations.

block/fops.c static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb, · line 369

Quoted 8 lines

block/fops.c +8 -0

+	} else {
+		/*
+		 * Users don't rely on the iterator being in any particular
+		 * state for async I/O returning -EIOCBQUEUED, hence we can
+		 * avoid expensive iov_iter_advance(). Bypass
+		 * bio_iov_iter_get_pages() and set the bvec directly.
+		 */
+		bio_iov_bvec_set(bio, iter);

So if this optimization is so useful, please also do it for non-bvec iov_iters, which is what 99% of the applications actually use.

Christoph Hellwig

2021-10-25 07:35 UTC #12 lore raw

Re: [PATCH 4/5] block: kill unused polling bits in __blkdev_direct_IO()

In reply to Pavel Begunkov

Quoted 3 lines · Pavel Begunkov

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.

Christoph Hellwig

2021-10-25 07:36 UTC #17 lore raw

Re: [PATCH 5/5] block: add async version of bio_set_polled

In reply to Pavel Begunkov

include/linux/bio.h static inline void bio_set_polled(struct bio *bio, struct kiocb *kiocb) · line 744

Quoted 4 lines · Pavel Begunkov

include/linux/bio.h +4 -0

+static inline void bio_set_polled_async(struct bio *bio, struct kiocb *kiocb)
+{
+	bio->bi_opf |= REQ_POLLED | REQ_NOWAIT;
+}

As mentioned last time I'm a little skeptical of this optimization. But if you an Jens thing it is worth it just drop this helper and assign the flags directly.

Pavel Begunkov

author

2021-10-25 10:14 UTC #13 lore raw

Re: [PATCH 4/5] block: kill unused polling bits in __blkdev_direct_IO()

In reply to Christoph Hellwig

Quoted 7 lines · Christoph Hellwig

Quoted 3 lines · Pavel Begunkov
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

Signature · 1 line

Pavel Begunkov

Pavel Begunkov

author

2021-10-25 10:22 UTC #18 lore raw

Re: [PATCH 5/5] block: add async version of bio_set_polled

In reply to Christoph Hellwig

Quoted 9 lines · Christoph Hellwig

include/linux/bio.h static inline void bio_set_polled(struct bio *bio, struct kiocb *kiocb) · line 744
Quoted 4 lines · Pavel Begunkov
include/linux/bio.h +4 -0
+static inline void bio_set_polled_async(struct bio *bio, struct kiocb *kiocb)
+{
+	bio->bi_opf |= REQ_POLLED | REQ_NOWAIT;
+}
As mentioned last time I'm a little skeptical of this optimization. But if you an Jens thing it is worth it just drop this helper and assign the flags directly.

Ok. I'll keep it last in the series, looks Jens doesn't mind applying patchsets partially.

Signature · 1 line

Pavel Begunkov

Pavel Begunkov

author

2021-10-25 10:28 UTC #14 lore raw

Re: [PATCH 4/5] block: kill unused polling bits in __blkdev_direct_IO()

In reply to Pavel Begunkov

Quoted 10 lines · Pavel Begunkov

Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · Pavel Begunkov
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.

Signature · 1 line

Pavel Begunkov

Pavel Begunkov

author

2021-10-25 11:12 UTC #6 lore raw

Re: [PATCH 3/5] block: avoid extra iter advance with async iocb

In reply to Christoph Hellwig

Quoted 23 lines · Christoph Hellwig

Quoted 8 lines · Pavel Begunkov
block/fops.c +0 -2
@@ -352,11 +352,21 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
  	bio->bi_end_io = blkdev_bio_end_io_async;
  	bio->bi_ioprio = iocb->ki_ioprio;
 
-	ret = bio_iov_iter_get_pages(bio, iter);
-	if (unlikely(ret)) {
block/fops.c static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb, · line 361
Quoted 10 lines
block/fops.c +7 -3
-		bio->bi_status = BLK_STS_IOERR;
-		bio_endio(bio);
-		return ret;
+	if (!iov_iter_is_bvec(iter)) {
+		ret = bio_iov_iter_get_pages(bio, iter);
+		if (unlikely(ret)) {
+			bio->bi_status = BLK_STS_IOERR;
+			bio_endio(bio);
+			return ret;
+		}
Nit: I generally find it much nicer to read if simple if statements don't use pointless negations.

Quoted 12 lines

block/fops.c static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb, · line 369
Quoted 8 lines
block/fops.c +8 -0
+	} else {
+		/*
+		 * Users don't rely on the iterator being in any particular
+		 * state for async I/O returning -EIOCBQUEUED, hence we can
+		 * avoid expensive iov_iter_advance(). Bypass
+		 * bio_iov_iter_get_pages() and set the bvec directly.
+		 */
+		bio_iov_bvec_set(bio, iter);
So if this optimization is so useful, please also do it for non-bvec iov_iters, which is what 99% of the applications actually use.

It's an async path, so mainly io_uring or aio, I don't think there is much profit in doing that for iov, especially behind iov -> bvec translation with page referencing.

Could've been done nonetheless, but what I think about looks too ugly because of the loop inside of bio_iov_iter_get_pages(). Don't think the sketch below is viable, any better ideas?

ssize_t __bio_iov_iter_get_pages() {
	...
	/* not advancing */
	return size;
}

do {
	if (bio_op(bio) == REQ_OP_ZONE_APPEND)
		ret = __bio_iov_append_get_pages(bio, iter);
	else
		ret = __bio_iov_iter_get_pages(bio, iter);
	if (ret < 0)
		break;
	iov_iter_advance(ret);
} while (iov_iter_count(iter) && !bio_full(bio, 0));

and copy paste into fops.c

do {
	if (bio_op(bio) == REQ_OP_ZONE_APPEND)
		ret = __bio_iov_append_get_pages(bio, iter);
	else
		ret = __bio_iov_iter_get_pages(bio, iter);

	if (ret < 0 || iov_iter_count(iter) == ret || bio_full(bio, 0))
		break;
	iov_iter_advance(ret);
} while (1);

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer Applied

2021-10-26 23:30 UTC #20 lore raw

Re: (subset) [PATCH 0/5] block optimisations

In reply to Pavel Begunkov

Quoted 5 lines · Pavel Begunkov

Another split off from the "block optimisation round" series focusing on fops.c. It addresses Cristoph's review, the main change is setting bvec directly instead of passing a "hint"
to bio_iov_iter_get_pages() in 3/5, and a prep patch 2/5.

Quoted 7 lines

I don't find a good way how to deduplicate __blkdev_direct_IO_async(), there are small differences in implementation. If that's fine I'd suggest to do it afterwards, anyway I want to brush up __blkdev_direct_IO(), e.g. remove effectively unnecessary DIO_MULTI_BIO flag.
[...]

Applied, thanks!

[1/5] block: add single bio async direct IO helper
(no commit info)
[2/5] block: refactor bio_iov_bvec_set()
(no commit info)

Best regards,

Signature · 1 line

Jens Axboe

Christoph Hellwig

2021-10-27 06:47 UTC #15 lore raw

Re: [PATCH 4/5] block: kill unused polling bits in __blkdev_direct_IO()

In reply to Pavel Begunkov

Quoted 7 lines · Pavel Begunkov

Quoted 6 lines · Pavel Begunkov
Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Pavel Begunkov
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.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
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.