nocopy bvec for direct IO

RFC In review Pavel Begunkov 7 participants 26 messages Dec 9, 2020 – Dec 13, 2020

Subsystems (5)

VFS Block layer io_uring SCSI target +1 more subsystems

lore

Share & tools

lore

Pavel Begunkov

author

2020-12-09 02:24 UTC #0 lore raw

cover nocopy bvec for direct IO

The idea is to avoid copying, merging, etc. bvec from iterator to bio in direct I/O and use the one we've already got. Hook it up for io_uring. Had an eye on it for a long, and it also was brought up by Matthew just recently. Let me know if I forgot or misplaced some tags.

A benchmark got me 430KIOPS vs 540KIOPS, or +25% on bare metal. And perf shows that bio_iov_iter_get_pages() was taking ~20%. The test is pretty silly, but still imposing. I'll redo it closer to reality for next iteration, anyway need to double check some cases.

If same applied to iomap, common chunck can be moved from block_dev into bio_iov_iter_get_pages(), but if there any benefit for filesystems, they should explicitly opt in with ITER_BVEC_FLAG_FIXED.

# how to apply
based on Jens' for-11/block
+ Ming's nr_vec patch,
+ io_uring fix, 9c3a205c5ffa36e96903c2 ("io_uring: fix ITER_BVEC check")

or there:
https://github.com/isilence/linux/commits/bvec_nocopy

# how to reproduce
null_blk queue_mode=2 completion_nsec=0 submit_queues=NUM_CPU
fio/t/io_uring with null blk, no iopoll, BS=16*4096

Cc Christoph Hellwig <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc Ming Lei <[email protected]>

Cc Johannes Weiner <[email protected]>

Pavel Begunkov (2):
iov: introduce ITER_BVEC_FLAG_FIXED
block: no-copy bvec for direct IO

 fs/block_dev.c      | 30 +++++++++++++++++++++++++++++-
 fs/io_uring.c       |  1 +
 include/linux/uio.h | 14 +++++++++++---
 3 files changed, 41 insertions(+), 4 deletions(-)

Signature · 1 line

2.24.0

Pavel Begunkov

author

2020-12-09 02:24 UTC #1 lore raw

PATCH 1/2 iov: introduce ITER_BVEC_FLAG_FIXED

Add ITER_BVEC_FLAG_FIXED iov iter flag, which will allow us to reuse passed in bvec instead of copying it. In particular it means that iter->bvec won't be freed and page references are taken remain so until callees don't need them, including asynchronous execution.

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

---
 fs/io_uring.c       |  1 +
 include/linux/uio.h | 14 +++++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

fs/io_uring.c +1 -0

index c536462920a3..9ff2805d0075 100644
@@ -2920,6 +2920,7 @@ static ssize_t io_import_fixed(struct io_kiocb *req, int rw,
 		}
 	}
 
+	iter->type |= ITER_BVEC_FLAG_FIXED;
 	return len;
 }

include/linux/uio.h +11 -3

index 72d88566694e..af626eb970cf 100644
@@ -18,6 +18,8 @@ struct kvec {
 };
 
 enum iter_type {
+	ITER_BVEC_FLAG_FIXED = 2,
+
 	/* iter types */
 	ITER_IOVEC = 4,
 	ITER_KVEC = 8,

@@ -29,8 +31,9 @@ enum iter_type {
 struct iov_iter {
 	/*
 	 * Bit 0 is the read/write bit, set if we're writing.
-	 * Bit 1 is the BVEC_FLAG_NO_REF bit, set if type is a bvec and
-	 * the caller isn't expecting to drop a page reference when done.
+	 * Bit 1 is the BVEC_FLAG_FIXED bit, set if type is a bvec and the
+	 * caller ensures that page references and memory baking bvec won't
+	 * go away until callees finish with them.
 	 */
 	unsigned int type;
 	size_t iov_offset;

@@ -52,7 +55,7 @@ struct iov_iter {
 
 static inline enum iter_type iov_iter_type(const struct iov_iter *i)
 {
-	return i->type & ~(READ | WRITE);
+	return i->type & ~(READ | WRITE | ITER_BVEC_FLAG_FIXED);
 }
 
 static inline bool iter_is_iovec(const struct iov_iter *i)

@@ -85,6 +88,11 @@ static inline unsigned char iov_iter_rw(const struct iov_iter *i)
 	return i->type & (READ | WRITE);
 }
 
+static inline unsigned char iov_iter_bvec_fixed(const struct iov_iter *i)
+{
+	return i->type & ITER_BVEC_FLAG_FIXED;
+}
+
 /*
  * Total number of bytes covered by an iovec.
  *

Signature · 1 line

2.24.0

Christoph Hellwig

2020-12-09 08:37 UTC #2 lore raw

Re: [PATCH 1/2] iov: introduce ITER_BVEC_FLAG_FIXED

Ok, seems like the patches made it to the lists, while oyu only send the cover letter to my address which is very strange.

include/linux/uio.h struct kvec { · line 25

Quoted 13 lines

include/linux/uio.h +2 -0

index 72d88566694e..af626eb970cf 100644
@@ -18,6 +18,8 @@ struct kvec {
 };
 
 enum iter_type {
+	ITER_BVEC_FLAG_FIXED = 2,
+
 	/* iter types */
 	ITER_IOVEC = 4,
 	ITER_KVEC = 8,

This is making the iter type even more of a mess than it already is. I think we at least need placeholders for 0/1 here and an explicit flags namespace, preferably after the types.

Then again I'd much prefer if we didn't even add the flag or at best just add it for a short-term transition and move everyone over to the new scheme. Otherwise the amount of different interfaces and supporting code keeps exploding.

include/linux/uio.h enum iter_type { · line 38

Quoted 11 lines

include/linux/uio.h +3 -2

@@ -29,8 +31,9 @@ enum iter_type {
 struct iov_iter {
 	/*
 	 * Bit 0 is the read/write bit, set if we're writing.
-	 * Bit 1 is the BVEC_FLAG_NO_REF bit, set if type is a bvec and
-	 * the caller isn't expecting to drop a page reference when done.
+	 * Bit 1 is the BVEC_FLAG_FIXED bit, set if type is a bvec and the
+	 * caller ensures that page references and memory baking bvec won't
+	 * go away until callees finish with them.
 	 */
 	unsigned int type;

I think the comment needs to move to the enum.

Christoph Hellwig

2020-12-09 09:07 UTC #3 lore raw

Re: [PATCH 1/2] iov: introduce ITER_BVEC_FLAG_FIXED

Quoted 8 lines · Christoph Hellwig

This is making the iter type even more of a mess than it already is. I think we at least need placeholders for 0/1 here and an explicit flags namespace, preferably after the types.
Then again I'd much prefer if we didn't even add the flag or at best just add it for a short-term transition and move everyone over to the new scheme. Otherwise the amount of different interfaces and supporting code keeps exploding.

Note that the only other callers that use iov_iter_bvec and asynchronous read/write are loop, target and nvme-target, so this should actually be pretty simple. Out of these only target needs something like this trivial change to keep the bvec available over the duration of the I/O, the other two should be fine already:

---
From 581a8eabbb1759e3dcfee4b1d2e419f814a8cb80 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
Date: Wed, 9 Dec 2020 10:05:04 +0100
Subject: target/file: allocate the bvec array as part of struct target_core_file_cmd

This saves one memory allocation, and ensures the bvecs aren't freed before the AIO completion. This will allow the lower level code to be optimized so that it can avoid allocating another bvec array.

Signed-off-by Christoph Hellwig <[email protected]>

---
 drivers/target/target_core_file.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

drivers/target/target_core_file.c +6 ~~-14~~

index 7143d03f0e027e..ed0c39a1f7c649 100644
@@ -241,6 +241,7 @@ struct target_core_file_cmd {
 	unsigned long	len;
 	struct se_cmd	*cmd;
 	struct kiocb	iocb;
+	struct bio_vec	bvecs[];
 };
 
 static void cmd_rw_aio_complete(struct kiocb *iocb, long ret, long ret2)

@@ -268,29 +269,22 @@ fd_execute_rw_aio(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	struct target_core_file_cmd *aio_cmd;
 	struct iov_iter iter = {};
 	struct scatterlist *sg;
-	struct bio_vec *bvec;
 	ssize_t len = 0;
 	int ret = 0, i;
 
-	aio_cmd = kmalloc(sizeof(struct target_core_file_cmd), GFP_KERNEL);
+	aio_cmd = kmalloc(struct_size(aio_cmd, bvecs, sgl_nents), GFP_KERNEL);
 	if (!aio_cmd)
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
-	bvec = kcalloc(sgl_nents, sizeof(struct bio_vec), GFP_KERNEL);
-	if (!bvec) {
-		kfree(aio_cmd);
-		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-	}
-
 	for_each_sg(sgl, sg, sgl_nents, i) {
-		bvec[i].bv_page = sg_page(sg);
-		bvec[i].bv_len = sg->length;
-		bvec[i].bv_offset = sg->offset;
+		aio_cmd->bvecs[i].bv_page = sg_page(sg);
+		aio_cmd->bvecs[i].bv_len = sg->length;
+		aio_cmd->bvecs[i].bv_offset = sg->offset;
 
 		len += sg->length;
 	}
 
-	iov_iter_bvec(&iter, is_write, bvec, sgl_nents, len);
+	iov_iter_bvec(&iter, is_write, aio_cmd->bvecs, sgl_nents, len);
 
 	aio_cmd->cmd = cmd;
 	aio_cmd->len = len;

@@ -307,8 +301,6 @@ fd_execute_rw_aio(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	else
 		ret = call_read_iter(file, &aio_cmd->iocb, &iter);
 
-	kfree(bvec);
-
 	if (ret != -EIOCBQUEUED)
 		cmd_rw_aio_complete(&aio_cmd->iocb, ret, 0);

Signature · 1 line

2.29.2

Pavel Begunkov

author

2020-12-09 11:58 UTC #4 lore raw

Re: [PATCH 1/2] iov: introduce ITER_BVEC_FLAG_FIXED

Quoted 9 lines · Christoph Hellwig

Quoted 8 lines · Christoph Hellwig
This is making the iter type even more of a mess than it already is. I think we at least need placeholders for 0/1 here and an explicit flags namespace, preferably after the types.
Then again I'd much prefer if we didn't even add the flag or at best just add it for a short-term transition and move everyone over to the new scheme. Otherwise the amount of different interfaces and supporting code keeps exploding.

At least the flag can be ignored. Anyway sounds good to me. I'll take your patch below to the series, thanks!

Quoted 79 lines

Signed-off-by Christoph Hellwig <[email protected]>

---
 drivers/target/target_core_file.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

drivers/target/target_core_file.c +6 ~~-14~~

index 7143d03f0e027e..ed0c39a1f7c649 100644
@@ -241,6 +241,7 @@ struct target_core_file_cmd {
 	unsigned long	len;
 	struct se_cmd	*cmd;
 	struct kiocb	iocb;
+	struct bio_vec	bvecs[];
 };
 
 static void cmd_rw_aio_complete(struct kiocb *iocb, long ret, long ret2)

@@ -268,29 +269,22 @@ fd_execute_rw_aio(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	struct target_core_file_cmd *aio_cmd;
 	struct iov_iter iter = {};
 	struct scatterlist *sg;
-	struct bio_vec *bvec;
 	ssize_t len = 0;
 	int ret = 0, i;
 
-	aio_cmd = kmalloc(sizeof(struct target_core_file_cmd), GFP_KERNEL);
+	aio_cmd = kmalloc(struct_size(aio_cmd, bvecs, sgl_nents), GFP_KERNEL);
 	if (!aio_cmd)
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
-	bvec = kcalloc(sgl_nents, sizeof(struct bio_vec), GFP_KERNEL);
-	if (!bvec) {
-		kfree(aio_cmd);
-		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-	}
-
 	for_each_sg(sgl, sg, sgl_nents, i) {
-		bvec[i].bv_page = sg_page(sg);
-		bvec[i].bv_len = sg->length;
-		bvec[i].bv_offset = sg->offset;
+		aio_cmd->bvecs[i].bv_page = sg_page(sg);
+		aio_cmd->bvecs[i].bv_len = sg->length;
+		aio_cmd->bvecs[i].bv_offset = sg->offset;
 
 		len += sg->length;
 	}
 
-	iov_iter_bvec(&iter, is_write, bvec, sgl_nents, len);
+	iov_iter_bvec(&iter, is_write, aio_cmd->bvecs, sgl_nents, len);
 
 	aio_cmd->cmd = cmd;
 	aio_cmd->len = len;

@@ -307,8 +301,6 @@ fd_execute_rw_aio(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	else
 		ret = call_read_iter(file, &aio_cmd->iocb, &iter);
 
-	kfree(bvec);
-
 	if (ret != -EIOCBQUEUED)
 		cmd_rw_aio_complete(&aio_cmd->iocb, ret, 0);

Signature · 1 line

Pavel Begunkov

Al Viro

maintainer

2020-12-09 13:08 UTC #5 lore raw

Re: [PATCH 1/2] iov: introduce ITER_BVEC_FLAG_FIXED

In reply to Christoph Hellwig

Quoted 9 lines · Christoph Hellwig

This is making the iter type even more of a mess than it already is. I think we at least need placeholders for 0/1 here and an explicit flags namespace, preferably after the types.
Then again I'd much prefer if we didn't even add the flag or at best just add it for a short-term transition and move everyone over to the new scheme. Otherwise the amount of different interfaces and supporting code keeps exploding.

Yes. The only problem I see is how to describe the rules - "bdev-backed iterators need the bvec array to stay allocated until IO completes"? And one way or another, that needs to be documented - D/f/porting with "mandatory" for priority.

Pavel Begunkov

author

2020-12-09 13:41 UTC #6 lore raw

Re: [PATCH 1/2] iov: introduce ITER_BVEC_FLAG_FIXED

Quoted 11 lines · Al Viro

Quoted 9 lines · Christoph Hellwig
This is making the iter type even more of a mess than it already is. I think we at least need placeholders for 0/1 here and an explicit flags namespace, preferably after the types.
Then again I'd much prefer if we didn't even add the flag or at best just add it for a short-term transition and move everyone over to the new scheme. Otherwise the amount of different interfaces and supporting code keeps exploding.

Quoted 4 lines

Yes. The only problem I see is how to describe the rules - "bdev-backed iterators need the bvec array to stay allocated until IO completes"? And one way or another, that needs to be documented - D/f/porting with "mandatory" for priority.

Yeah, I had troubles to put comments around, and it's still open.

For current cases it can be bound to kiocb, e.g. "if an bvec iter passed "together" with kiocb then the vector should stay intact up to ->ki_complete()". But that "together" is rather full of holes.

Signature · 1 line

Pavel Begunkov

Christoph Hellwig

2020-12-09 17:56 UTC #7 lore raw

Re: [PATCH 1/2] iov: introduce ITER_BVEC_FLAG_FIXED

Quoted 5 lines · Pavel Begunkov

Yeah, I had troubles to put comments around, and it's still open.
For current cases it can be bound to kiocb, e.g. "if an bvec iter passed "together" with kiocb then the vector should stay intact up to ->ki_complete()". But that "together" is rather full of holes.

What about: "For bvec based iters the bvec must not be freed until the I/O has completed. For asynchronous I/O that means it must be freed no earlier than from ->ki_complete."

Matthew Wilcox

2020-12-09 18:25 UTC #8 lore raw

Re: [PATCH 1/2] iov: introduce ITER_BVEC_FLAG_FIXED

Quoted 10 lines · Christoph Hellwig

Quoted 5 lines · Pavel Begunkov
Yeah, I had troubles to put comments around, and it's still open.
For current cases it can be bound to kiocb, e.g. "if an bvec iter passed "together" with kiocb then the vector should stay intact up to ->ki_complete()". But that "together" is rather full of holes.
What about: "For bvec based iters the bvec must not be freed until the I/O has completed. For asynchronous I/O that means it must be freed no earlier than from ->ki_complete."

Perhaps for the second sentence "If the I/O is completed asynchronously, the bvec must not be freed before ->ki_complete() has been called"?

Pavel Begunkov

author

2020-12-13 22:13 UTC #9 lore raw

Re: [PATCH 1/2] iov: introduce ITER_BVEC_FLAG_FIXED

Quoted 8 lines · Matthew Wilcox

Quoted 7 lines · Christoph Hellwig
Quoted 5 lines · Pavel Begunkov
Yeah, I had troubles to put comments around, and it's still open.
For current cases it can be bound to kiocb, e.g. "if an bvec iter passed "together" with kiocb then the vector should stay intact up to ->ki_complete()". But that "together" is rather full of holes.

Quoted 6 lines

Quoted 3 lines
What about: "For bvec based iters the bvec must not be freed until the I/O has completed. For asynchronous I/O that means it must be freed no earlier than from ->ki_complete."
Perhaps for the second sentence "If the I/O is completed asynchronously, the bvec must not be freed before ->ki_complete() has been called"?

Sounds good, I'll use it. Thanks!

Signature · 1 line

Pavel Begunkov

Pavel Begunkov

author

2020-12-09 02:24 UTC #10 lore raw

PATCH 2/2 block: no-copy bvec for direct IO

In reply to Pavel Begunkov

The block layer spends quite a while in blkdev_direct_IO() to copy and initialise bio's bvec. However, if we've already got a bvec in the input iterator it might be reused in some cases, i.e. when new ITER_BVEC_FLAG_FIXED flag is set. Simple tests show considerable performance boost, and it also reduces memory footprint.

[BIO_WORKINGSET] Suggested-by: Johannes Weiner <[email protected]>

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

---
 fs/block_dev.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

fs/block_dev.c +29 -1

index d699f3af1a09..aee5d2e4f324 100644
@@ -349,6 +349,28 @@ static void blkdev_bio_end_io(struct bio *bio)
 	}
 }
 
+static int bio_iov_fixed_bvec_get_pages(struct bio *bio, struct iov_iter *iter)
+{
+	bio->bi_vcnt = iter->nr_segs;
+	bio->bi_max_vecs = 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;
+
+	/*
+	 * In practice groups of pages tend to be accessed/reclaimed/refaulted
+	 * together. To not go over bvec for those who didn't set BIO_WORKINGSET
+	 * approximate it by looking at the first page and inducing it to the
+	 * whole bio
+	 */
+	if (unlikely(PageWorkingset(iter->bvec->bv_page)))
+		bio_set_flag(bio, BIO_WORKINGSET);
+	bio_set_flag(bio, BIO_NO_PAGE_REF);
+
+	iter->count = 0;
+	return 0;
+}
+
 static ssize_t
 __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_vecs)
 {

@@ -368,6 +390,8 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_vecs)
 	    (bdev_logical_block_size(bdev) - 1))
 		return -EINVAL;
 
+	if (iov_iter_bvec_fixed(iter))
+		nr_vecs = 0;
 	bio = bio_alloc_bioset(GFP_KERNEL, nr_vecs, &blkdev_dio_pool);
 
 	dio = container_of(bio, struct blkdev_dio, bio);

@@ -398,7 +422,11 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_vecs)
 		bio->bi_end_io = blkdev_bio_end_io;
 		bio->bi_ioprio = iocb->ki_ioprio;
 
-		ret = bio_iov_iter_get_pages(bio, iter);
+		if (iov_iter_is_bvec(iter) && iov_iter_bvec_fixed(iter))
+			ret = bio_iov_fixed_bvec_get_pages(bio, iter);
+		else
+			ret = bio_iov_iter_get_pages(bio, iter);
+
 		if (unlikely(ret)) {
 			bio->bi_status = BLK_STS_IOERR;
 			bio_endio(bio);

Signature · 1 line

2.24.0

Christoph Hellwig

2020-12-09 08:40 UTC #11 lore raw

Re: [PATCH 2/2] block: no-copy bvec for direct IO

fs/block_dev.c static void blkdev_bio_end_io(struct bio *bio) · line 367

Quoted 8 lines

fs/block_dev.c +8 -0

+	/*
+	 * In practice groups of pages tend to be accessed/reclaimed/refaulted
+	 * together. To not go over bvec for those who didn't set BIO_WORKINGSET
+	 * approximate it by looking at the first page and inducing it to the
+	 * whole bio
+	 */
+	if (unlikely(PageWorkingset(iter->bvec->bv_page)))
+		bio_set_flag(bio, BIO_WORKINGSET);

IIRC the feedback was that we do not need to deal with BIO_WORKINGSET at all for direct I/O.

fs/block_dev.c static void blkdev_bio_end_io(struct bio *bio) · line 372

Quoted 5 lines

fs/block_dev.c +5 -0

+	bio_set_flag(bio, BIO_NO_PAGE_REF);
+
+	iter->count = 0;
+	return 0;
+}

This helper should go into bio.c, next to bio_iov_iter_get_pages. And please convert the other callers of bio_iov_iter_get_pages to this scheme as well.

Pavel Begunkov

author

2020-12-09 12:05 UTC #12 lore raw

Re: [PATCH 2/2] block: no-copy bvec for direct IO

Quoted 11 lines · Christoph Hellwig

fs/block_dev.c static void blkdev_bio_end_io(struct bio *bio) · line 367
Quoted 8 lines
fs/block_dev.c +8 -0
+	/*
+	 * In practice groups of pages tend to be accessed/reclaimed/refaulted
+	 * together. To not go over bvec for those who didn't set BIO_WORKINGSET
+	 * approximate it by looking at the first page and inducing it to the
+	 * whole bio
+	 */
+	if (unlikely(PageWorkingset(iter->bvec->bv_page)))
+		bio_set_flag(bio, BIO_WORKINGSET);
IIRC the feedback was that we do not need to deal with BIO_WORKINGSET at all for direct I/O.

I was waiting for the conversation to unfold, i.e. for Johannes to answer. BTW, would the same (skipping BIO_WORKINGSET) stand true for iomap?

Quoted 10 lines

fs/block_dev.c static void blkdev_bio_end_io(struct bio *bio) · line 372
Quoted 6 lines
fs/block_dev.c +5 -0
+	bio_set_flag(bio, BIO_NO_PAGE_REF);
+
+	iter->count = 0;
+	return 0;
+}
This helper should go into bio.c, next to bio_iov_iter_get_pages. And please convert the other callers of bio_iov_iter_get_pages to this scheme as well.

Agree. In the end I want to merge that into bio_iov_iter_get_pages().

Signature · 1 line

Pavel Begunkov

Christoph Hellwig

2020-12-09 12:06 UTC #13 lore raw

Re: [PATCH 2/2] block: no-copy bvec for direct IO

Quoted 13 lines · Pavel Begunkov

Quoted 11 lines · Christoph Hellwig
fs/block_dev.c static void blkdev_bio_end_io(struct bio *bio) · line 367
Quoted 8 lines
fs/block_dev.c +8 -0
+	/*
+	 * In practice groups of pages tend to be accessed/reclaimed/refaulted
+	 * together. To not go over bvec for those who didn't set BIO_WORKINGSET
+	 * approximate it by looking at the first page and inducing it to the
+	 * whole bio
+	 */
+	if (unlikely(PageWorkingset(iter->bvec->bv_page)))
+		bio_set_flag(bio, BIO_WORKINGSET);
IIRC the feedback was that we do not need to deal with BIO_WORKINGSET at all for direct I/O.

Quoted 3 lines

I was waiting for the conversation to unfold, i.e. for Johannes to answer. BTW, would the same (skipping BIO_WORKINGSET) stand true for iomap?

iomap direct I/O: yes.

Pavel Begunkov

author

2020-12-09 12:07 UTC #14 lore raw

Re: [PATCH 2/2] block: no-copy bvec for direct IO

Quoted 14 lines · Christoph Hellwig

Quoted 13 lines · Pavel Begunkov
Quoted 11 lines · Christoph Hellwig
fs/block_dev.c static void blkdev_bio_end_io(struct bio *bio) · line 367
Quoted 8 lines
fs/block_dev.c +8 -0
+	/*
+	 * In practice groups of pages tend to be accessed/reclaimed/refaulted
+	 * together. To not go over bvec for those who didn't set BIO_WORKINGSET
+	 * approximate it by looking at the first page and inducing it to the
+	 * whole bio
+	 */
+	if (unlikely(PageWorkingset(iter->bvec->bv_page)))
+		bio_set_flag(bio, BIO_WORKINGSET);
IIRC the feedback was that we do not need to deal with BIO_WORKINGSET at all for direct I/O.

Quoted 5 lines

Quoted 3 lines
I was waiting for the conversation to unfold, i.e. for Johannes to answer. BTW, would the same (skipping BIO_WORKINGSET) stand true for iomap?
iomap direct I/O: yes.

That one, got it

Signature · 1 line

Pavel Begunkov

Johannes Weiner

2020-12-12 01:00 UTC #15 lore raw

Re: [PATCH 2/2] block: no-copy bvec for direct IO

In reply to Christoph Hellwig

Quoted 11 lines · Christoph Hellwig

fs/block_dev.c static void blkdev_bio_end_io(struct bio *bio) · line 367
Quoted 8 lines
fs/block_dev.c +8 -0
+	/*
+	 * In practice groups of pages tend to be accessed/reclaimed/refaulted
+	 * together. To not go over bvec for those who didn't set BIO_WORKINGSET
+	 * approximate it by looking at the first page and inducing it to the
+	 * whole bio
+	 */
+	if (unlikely(PageWorkingset(iter->bvec->bv_page)))
+		bio_set_flag(bio, BIO_WORKINGSET);
IIRC the feedback was that we do not need to deal with BIO_WORKINGSET at all for direct I/O.

Yes, this hunk is incorrect. We must not use this flag for direct IO. It's only for paging IO, when you bring in the data at page->mapping + page->index. Otherwise you tell the pressure accounting code that you are paging in a thrashing page, when really you're just reading new data into a page frame that happens to be hot.

(As per the other thread, bio_add_page() currently makes that same mistake for direct IO. I'm fixing that.)

Pavel Begunkov

author

2020-12-11 14:53 UTC #16 lore raw

Re: [PATCH 2/2] block: no-copy bvec for direct IO

Quoted 13 lines · Johannes Weiner

Quoted 11 lines · Christoph Hellwig
fs/block_dev.c static void blkdev_bio_end_io(struct bio *bio) · line 367
Quoted 8 lines
fs/block_dev.c +8 -0
+	/*
+	 * In practice groups of pages tend to be accessed/reclaimed/refaulted
+	 * together. To not go over bvec for those who didn't set BIO_WORKINGSET
+	 * approximate it by looking at the first page and inducing it to the
+	 * whole bio
+	 */
+	if (unlikely(PageWorkingset(iter->bvec->bv_page)))
+		bio_set_flag(bio, BIO_WORKINGSET);
IIRC the feedback was that we do not need to deal with BIO_WORKINGSET at all for direct I/O.

Quoted 8 lines

Yes, this hunk is incorrect. We must not use this flag for direct IO. It's only for paging IO, when you bring in the data at page->mapping + page->index. Otherwise you tell the pressure accounting code that you are paging in a thrashing page, when really you're just reading new data into a page frame that happens to be hot.
(As per the other thread, bio_add_page() currently makes that same mistake for direct IO. I'm fixing that.)

I have that stuff fixed, it just didn't go into the RFC. That's basically removing replacing add_page() with its version without BIO_WORKINGSET in bio_iov_iter_get_pages() and all __bio_iov_*_{add,get}_pages() + fix up ./fs/direct-io.c. Should cover all direct cases if I didn't miss some.

Signature · 1 line

Pavel Begunkov

Johannes Weiner

2020-12-11 16:34 UTC #17 lore raw

Re: [PATCH 2/2] block: no-copy bvec for direct IO

Quoted 20 lines · Pavel Begunkov

Quoted 19 lines · Johannes Weiner
Quoted 11 lines · Christoph Hellwig
fs/block_dev.c static void blkdev_bio_end_io(struct bio *bio) · line 367
Quoted 8 lines
fs/block_dev.c +8 -0
+	/*
+	 * In practice groups of pages tend to be accessed/reclaimed/refaulted
+	 * together. To not go over bvec for those who didn't set BIO_WORKINGSET
+	 * approximate it by looking at the first page and inducing it to the
+	 * whole bio
+	 */
+	if (unlikely(PageWorkingset(iter->bvec->bv_page)))
+		bio_set_flag(bio, BIO_WORKINGSET);
IIRC the feedback was that we do not need to deal with BIO_WORKINGSET at all for direct I/O.
Yes, this hunk is incorrect. We must not use this flag for direct IO. It's only for paging IO, when you bring in the data at page->mapping + page->index. Otherwise you tell the pressure accounting code that you are paging in a thrashing page, when really you're just reading new data into a page frame that happens to be hot.

Quoted 8 lines

Quoted 2 lines
(As per the other thread, bio_add_page() currently makes that same mistake for direct IO. I'm fixing that.)
I have that stuff fixed, it just didn't go into the RFC. That's basically removing replacing add_page() with its version without BIO_WORKINGSET in bio_iov_iter_get_pages() and all __bio_iov_*_{add,get}_pages() + fix up ./fs/direct-io.c. Should cover all direct cases if I didn't miss some.

Ah, that's fantastic! Thanks for clarifying.

Pavel Begunkov

author

2020-12-11 16:50 UTC #18 lore raw

Re: [PATCH 2/2] block: no-copy bvec for direct IO

Quoted 21 lines · Johannes Weiner

Quoted 20 lines · Pavel Begunkov
Quoted 19 lines · Johannes Weiner
Quoted 11 lines · Christoph Hellwig
fs/block_dev.c static void blkdev_bio_end_io(struct bio *bio) · line 367
Quoted 8 lines
fs/block_dev.c +8 -0
+	/*
+	 * In practice groups of pages tend to be accessed/reclaimed/refaulted
+	 * together. To not go over bvec for those who didn't set BIO_WORKINGSET
+	 * approximate it by looking at the first page and inducing it to the
+	 * whole bio
+	 */
+	if (unlikely(PageWorkingset(iter->bvec->bv_page)))
+		bio_set_flag(bio, BIO_WORKINGSET);
IIRC the feedback was that we do not need to deal with BIO_WORKINGSET at all for direct I/O.
Yes, this hunk is incorrect. We must not use this flag for direct IO. It's only for paging IO, when you bring in the data at page->mapping + page->index. Otherwise you tell the pressure accounting code that you are paging in a thrashing page, when really you're just reading new data into a page frame that happens to be hot.

Quoted 5 lines

Quoted 5 lines
Quoted 2 lines
(As per the other thread, bio_add_page() currently makes that same mistake for direct IO. I'm fixing that.)
I have that stuff fixed, it just didn't go into the RFC. That's basically removing replacing add_page() with its version without BIO_WORKINGSET

I wrote something strange... Should have been "replacing add_page() in those functions with a version without BIO_WORKINGSET".

Quoted 5 lines

Quoted 3 lines
in bio_iov_iter_get_pages() and all __bio_iov_*_{add,get}_pages() + fix up ./fs/direct-io.c. Should cover all direct cases if I didn't miss some.
Ah, that's fantastic! Thanks for clarifying.

To keep it clear, do we go with what I have stashed (I'm planning to reiterate this weekend)? or you're going to write it up yourself? Just in case there is some cooler way you have in mind :)

Signature · 1 line

Pavel Begunkov

Johannes Weiner

2020-12-11 17:22 UTC #19 lore raw

Re: [PATCH 2/2] block: no-copy bvec for direct IO

Quoted 22 lines · Pavel Begunkov

Quoted 21 lines · Johannes Weiner
Quoted 20 lines · Pavel Begunkov
Quoted 19 lines · Johannes Weiner
Quoted 11 lines · Christoph Hellwig
fs/block_dev.c static void blkdev_bio_end_io(struct bio *bio) · line 367
Quoted 8 lines
fs/block_dev.c +8 -0
+	/*
+	 * In practice groups of pages tend to be accessed/reclaimed/refaulted
+	 * together. To not go over bvec for those who didn't set BIO_WORKINGSET
+	 * approximate it by looking at the first page and inducing it to the
+	 * whole bio
+	 */
+	if (unlikely(PageWorkingset(iter->bvec->bv_page)))
+		bio_set_flag(bio, BIO_WORKINGSET);
IIRC the feedback was that we do not need to deal with BIO_WORKINGSET at all for direct I/O.
Yes, this hunk is incorrect. We must not use this flag for direct IO. It's only for paging IO, when you bring in the data at page->mapping + page->index. Otherwise you tell the pressure accounting code that you are paging in a thrashing page, when really you're just reading new data into a page frame that happens to be hot.

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
(As per the other thread, bio_add_page() currently makes that same mistake for direct IO. I'm fixing that.)
I have that stuff fixed, it just didn't go into the RFC. That's basically removing replacing add_page() with its version without BIO_WORKINGSET
I wrote something strange... Should have been "replacing add_page() in those functions with a version without BIO_WORKINGSET".

No worries, I understood.

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
in bio_iov_iter_get_pages() and all __bio_iov_*_{add,get}_pages() + fix up ./fs/direct-io.c. Should cover all direct cases if I didn't miss some.
Ah, that's fantastic! Thanks for clarifying.
To keep it clear, do we go with what I have stashed (I'm planning to reiterate this weekend)? or you're going to write it up yourself? Just in case there is some cooler way you have in mind :)

Honestly, I only wrote all my ideas down and asked for feedback because I wasn't super excited about any of them ;-)

If your changes happen to separate the direct io path from the buffered io path naturally, I'm okay with it.

I'd say let's go with what you already have and see whether Jens and Christoph like it. We can always do follow-on cleanups.

David Laight

2020-12-09 21:15 UTC #20 lore raw

RE: [PATCH 2/2] block: no-copy bvec for direct IO

In reply to Pavel Begunkov

From: Pavel Begunkov

Quoted 7 lines

Sent: 09 December 2020 02:20
The block layer spends quite a while in blkdev_direct_IO() to copy and initialise bio's bvec. However, if we've already got a bvec in the input iterator it might be reused in some cases, i.e. when new ITER_BVEC_FLAG_FIXED flag is set. Simple tests show considerable performance boost, and it also reduces memory footprint.

...

fs/block_dev.c __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_vecs) · line 429

Quoted 10 lines

fs/block_dev.c +5 -1

@@ -398,7 +422,11 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_vecs)
 		bio->bi_end_io = blkdev_bio_end_io;
 		bio->bi_ioprio = iocb->ki_ioprio;
 
-		ret = bio_iov_iter_get_pages(bio, iter);
+		if (iov_iter_is_bvec(iter) && iov_iter_bvec_fixed(iter))
+			ret = bio_iov_fixed_bvec_get_pages(bio, iter);
+		else
+			ret = bio_iov_iter_get_pages(bio, iter);
+

Is it necessary to check iov_iter_is_bvec() as well as iov_iter_bvec_fixed() ? If so it is probably worth using & not && so the compiler stands a chance of generating a & (B | C) == B instead of 2 conditionals. (I think I saw the bits in the same field being tested.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Christoph Hellwig

2020-12-09 06:51 UTC #21 lore raw

In reply to Pavel Begunkov

Quoted 4 lines · Pavel Begunkov

A benchmark got me 430KIOPS vs 540KIOPS, or +25% on bare metal. And perf shows that bio_iov_iter_get_pages() was taking ~20%. The test is pretty silly, but still imposing. I'll redo it closer to reality for next iteration, anyway need to double check some cases.

That is pretty impressive. But I only got this cover letter, not the actual patches..

Pavel Begunkov

author

2020-12-09 11:58 UTC #22 lore raw

Quoted 7 lines · Christoph Hellwig

Quoted 4 lines · Pavel Begunkov
A benchmark got me 430KIOPS vs 540KIOPS, or +25% on bare metal. And perf shows that bio_iov_iter_get_pages() was taking ~20%. The test is pretty silly, but still imposing. I'll redo it closer to reality for next iteration, anyway need to double check some cases.
That is pretty impressive.

The difference will go down with BS=~1-2 pages, just need to to find a moment to test properly.

Quoted 2 lines

But I only got this cover letter, not the
actual patches..

Apologies, that goes for everyone as lost CCs in the patches.

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2020-12-09 16:54 UTC #23 lore raw

In reply to Pavel Begunkov

Quoted 5 lines · Pavel Begunkov

The idea is to avoid copying, merging, etc. bvec from iterator to bio in direct I/O and use the one we've already got. Hook it up for io_uring. Had an eye on it for a long, and it also was brought up by Matthew just recently. Let me know if I forgot or misplaced some tags.

Quoted 8 lines

A benchmark got me 430KIOPS vs 540KIOPS, or +25% on bare metal. And perf shows that bio_iov_iter_get_pages() was taking ~20%. The test is pretty silly, but still imposing. I'll redo it closer to reality for next iteration, anyway need to double check some cases.
If same applied to iomap, common chunck can be moved from block_dev into bio_iov_iter_get_pages(), but if there any benefit for filesystems, they should explicitly opt in with ITER_BVEC_FLAG_FIXED.

Ran this on a real device, and I get a 10% bump in performance with it. That's pretty amazing! So please do pursue this one and pull it to completion.

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2020-12-13 22:08 UTC #24 lore raw

Quoted 11 lines · Jens Axboe

Quoted 10 lines · Pavel Begunkov
The idea is to avoid copying, merging, etc. bvec from iterator to bio in direct I/O and use the one we've already got. Hook it up for io_uring. Had an eye on it for a long, and it also was brought up by Matthew just recently. Let me know if I forgot or misplaced some tags.
A benchmark got me 430KIOPS vs 540KIOPS, or +25% on bare metal. And perf shows that bio_iov_iter_get_pages() was taking ~20%. The test is pretty silly, but still imposing. I'll redo it closer to reality for next iteration, anyway need to double check some cases.

Quoted 7 lines

Quoted 3 lines
If same applied to iomap, common chunck can be moved from block_dev into bio_iov_iter_get_pages(), but if there any benefit for filesystems, they should explicitly opt in with ITER_BVEC_FLAG_FIXED.
Ran this on a real device, and I get a 10% bump in performance with it. That's pretty amazing! So please do pursue this one and pull it to completion.

I'm curious, what block size did you use?

Signature · 1 line

Pavel Begunkov

Al Viro

maintainer

2020-12-09 17:07 UTC #25 lore raw

In reply to Pavel Begunkov

Quoted 5 lines · Pavel Begunkov

The idea is to avoid copying, merging, etc. bvec from iterator to bio in direct I/O and use the one we've already got. Hook it up for io_uring. Had an eye on it for a long, and it also was brought up by Matthew just recently. Let me know if I forgot or misplaced some tags.

Quoted 8 lines

A benchmark got me 430KIOPS vs 540KIOPS, or +25% on bare metal. And perf shows that bio_iov_iter_get_pages() was taking ~20%. The test is pretty silly, but still imposing. I'll redo it closer to reality for next iteration, anyway need to double check some cases.
If same applied to iomap, common chunck can be moved from block_dev into bio_iov_iter_get_pages(), but if there any benefit for filesystems, they should explicitly opt in with ITER_BVEC_FLAG_FIXED.

To reiterate what hch said - this "opt in" is wrong. Out-of-tree code that does async IO on bvec-backed iov_iter, setting it up on its own will have to adapt, that all.

iov_iter and its users are already in serious need of simplification and cleanups; piling more on top of that would be a bloody bad idea.

Proposed semantics change for bvec-backed iov_iter makes a lot of sense, so let's make sure that everything in tree can live with it, document the change and switch to better semantics.

This thing should be unconditional. Document it in D/f/porting and if something out of tree complains, it's their problem - not ours.