From: Pavel Begunkov <asml.silence@gmail.com> Date: 2021-01-02 15:23:40
Currently, when iomap and block direct IO gets a bvec based iterator
the bvec will be copied, with all other accounting that takes much
CPU time and causes additional allocation for larger bvecs. The
patchset makes it to reuse the passed in iter bvec.
[1,2] are forbidding zero-length bvec segments to not pile special
cases, [3] skip/fix PSI tracking to not iterate over bvecs extra
time.
nullblk completion_nsec=0 submit_queues=NR_CORES, no merges, no stats
fio/t/io_uring /dev/nullb0 -d 128 -s 32 -c 32 -p 0 -B 1 -F 1 -b BLOCK_SIZE
BLOCK_SIZE 512 4K 8K 16K 32K 64K
===================================================================
old (KIOPS) 1208 1208 1131 1039 863 699
new (KIOPS) 1222 1222 1170 1137 1083 982
Previously, Jens got before 10% difference for polling real HW and small
block sizes, but that was for an older version that had one
iov_iter_advance() less
since RFC:
- add target_core_file patch by Christoph
- make no-copy default behaviour, remove iter flag
- iter_advance() instead of hacks to revert to work
- add bvec iter_advance() optimisation patch
- remove PSI annotations from direct IO (iomap, block and fs/direct)
- note in d/f/porting
since v1:
- don't allow zero-length bvec segments (Ming)
- don't add a BIO_WORKINGSET-less version of bio_add_page(), just clear
the flag at the end and leave it for further cleanups (Christoph)
- commit message and comments rewording (Dave)
- other nits by Christoph
Christoph Hellwig (1):
target/file: allocate the bvec array as part of struct
target_core_file_cmd
Pavel Begunkov (6):
splice: don't generate zero-len segement bvecs
bvec/iter: disallow zero-length segment bvecs
block/psi: remove PSI annotations from direct IO
iov_iter: optimise bvec iov_iter_advance()
bio: add a helper calculating nr segments to alloc
bio: don't copy bvec for direct IO
Documentation/filesystems/porting.rst | 16 ++++++
block/bio.c | 71 +++++++++++++--------------
drivers/target/target_core_file.c | 20 +++-----
fs/block_dev.c | 7 +--
fs/direct-io.c | 2 +
fs/iomap/direct-io.c | 9 ++--
fs/splice.c | 9 ++--
include/linux/bio.h | 13 +++++
lib/iov_iter.c | 21 +++++++-
9 files changed, 103 insertions(+), 65 deletions(-)
--
2.24.0
From: Pavel Begunkov <asml.silence@gmail.com> Date: 2021-01-02 15:22:22
Direct IO does not operate on the current working set of pages managed
by the kernel, so it should not be accounted as memory stall to PSI
infrastructure.
The block layer and iomap direct IO use bio_iov_iter_get_pages()
to build bios, and they are the only users of it, so to avoid PSI
tracking for them clear out BIO_WORKINGSET flag. Do same for
dio_bio_submit() because fs/direct_io constructs bios by hand directly
calling bio_add_page().
Reported-by: Christoph Hellwig <hch@infradead.org>
Suggested-by: Christoph Hellwig <hch@infradead.org>
Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
block/bio.c | 6 ++++++
fs/direct-io.c | 2 ++
2 files changed, 8 insertions(+)
@@ -1099,6 +1099,9 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)*fitintothebio,orarerequestedin@iter,whateverissmaller.If*MMencountersanerrorpinningtherequestedpages,itstops.Error*isreturnedonlyif0pagescouldbepinned.+*+*It'sintendedfordirectIO,sodoesn'tdoPSItracking,thecalleris+*responsibleforsettingBIO_WORKINGSETifnecessary.*/intbio_iov_iter_get_pages(structbio*bio,structiov_iter*iter){
@@ -1123,6 +1126,9 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)if(is_bvec)bio_set_flag(bio,BIO_NO_PAGE_REF);++/* don't account direct I/O as memory stall */+bio_clear_flag(bio,BIO_WORKINGSET);returnbio->bi_vcnt?0:ret;}EXPORT_SYMBOL_GPL(bio_iov_iter_get_pages);
From: Pavel Begunkov <asml.silence@gmail.com> Date: 2021-01-02 15:22:41
From: Christoph Hellwig <hch@lst.de>
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 <hch@lst.de>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
drivers/target/target_core_file.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
From: Pavel Begunkov <asml.silence@gmail.com> Date: 2021-01-02 15:22:41
zero-length bvec segments are allowed in general, but not handled by bio
and down the block layer so filtered out. This inconsistency may be
confusing and prevent from optimisations. As zero-length segments are
useless and places that were generating them are patched, declare them
not allowed.
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
Documentation/filesystems/porting.rst | 7 +++++++
lib/iov_iter.c | 2 --
2 files changed, 7 insertions(+), 2 deletions(-)
@@ -865,3 +865,10 @@ no matter what. Everything is handled by the caller. clone_private_mount() returns a longterm mount now, so the proper destructor of its result is kern_unmount() or kern_unmount_array().++---++**mandatory**++zero-length bvec segments are disallowed, they must be filtered out before+passed on to an iterator.
From: Pavel Begunkov <asml.silence@gmail.com> Date: 2021-01-02 15:22:42
iter_file_splice_write() may spawn bvec segments with zero-length. In
preparation for prohibiting them, filter out by hand at splice level.
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
fs/splice.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
From: Pavel Begunkov <asml.silence@gmail.com> Date: 2021-01-02 15:23:41
Add a helper function calculating the number of bvec segments we need to
allocate to construct a bio. It doesn't change anything functionally,
but will be used to not duplicate special cases in the future.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
fs/block_dev.c | 7 ++++---
fs/iomap/direct-io.c | 9 ++++-----
include/linux/bio.h | 10 ++++++++++
3 files changed, 18 insertions(+), 8 deletions(-)
@@ -250,11 +250,8 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,orig_count=iov_iter_count(dio->submit.iter);iov_iter_truncate(dio->submit.iter,length);-nr_pages=iov_iter_npages(dio->submit.iter,BIO_MAX_PAGES);-if(nr_pages<=0){-ret=nr_pages;+if(!iov_iter_count(dio->submit.iter))gotoout;-}if(need_zeroout){/* zero out from the start of the block to the write offset */
@@ -10,6 +10,7 @@#include<linux/ioprio.h>/* struct bio, bio_vec and BIO_* flags are defined in blk_types.h */#include<linux/blk_types.h>+#include<linux/uio.h>#define BIO_DEBUG
@@ -441,6 +442,15 @@ static inline void bio_wouldblock_error(struct bio *bio)bio_endio(bio);}+/*+*Calculatenumberofbvecsegmentsthatshouldbeallocatedtofitdata+*pointedby@iter.+*/+staticinlineintbio_iov_vecs_to_alloc(structiov_iter*iter,intmax_segs)+{+returniov_iter_npages(iter,max_segs);+}+structrequest_queue;externintsubmit_bio_wait(structbio*bio);
From: Pavel Begunkov <asml.silence@gmail.com> Date: 2021-01-02 15:23:42
iov_iter_advance() is heavily used, but implemented through generic
means. For bvecs there is a specifically crafted function for that, so
use bvec_iter_advance() instead, it's faster and slimmer.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
lib/iov_iter.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
From: Pavel Begunkov <asml.silence@gmail.com> Date: 2021-01-02 15:23:43
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.
Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
Documentation/filesystems/porting.rst | 9 ++++
block/bio.c | 67 ++++++++++++---------------
include/linux/bio.h | 5 +-
3 files changed, 42 insertions(+), 39 deletions(-)
@@ -872,3 +872,12 @@ its result is kern_unmount() or kern_unmount_array(). zero-length bvec segments are disallowed, they must be filtered out before passed on to an iterator.++---++**mandatory**++For bvec based itererators bio_iov_iter_get_pages() now doesn't copy bvecs but+uses the one provided. Anyone issuing kiocb-I/O should ensure that the bvec and+page references stay until I/O has completed, i.e. until ->ki_complete() has+been called or returned with non -EIOCBQUEUED code.
@@ -960,21 +960,17 @@ void bio_release_pages(struct bio *bio, bool mark_dirty)}EXPORT_SYMBOL_GPL(bio_release_pages);-staticint__bio_iov_bvec_add_pages(structbio*bio,structiov_iter*iter)+staticintbio_iov_bvec_set(structbio*bio,structiov_iter*iter){-conststructbio_vec*bv=iter->bvec;-unsignedintlen;-size_tsize;--if(WARN_ON_ONCE(iter->iov_offset>bv->bv_len))-return-EINVAL;--len=min_t(size_t,bv->bv_len-iter->iov_offset,iter->count);-size=bio_add_page(bio,bv->bv_page,len,-bv->bv_offset+iter->iov_offset);-if(unlikely(size!=len))-return-EINVAL;-iov_iter_advance(iter,size);+WARN_ON_ONCE(BVEC_POOL_IDX(bio)!=0);++bio->bi_vcnt=iter->nr_segs;+bio->bi_max_vecs=iter->nr_segs;+bio->bi_io_vec=(structbio_vec*)iter->bvec;+bio->bi_iter.bi_bvec_done=iter->iov_offset;+bio->bi_iter.bi_size=iter->count;++iov_iter_advance(iter,iter->count);return0;}
@@ -1088,12 +1084,12 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)*Thistakeseitheraniteratorpointingtousermemory,oronepointingto*kernelpages(BVECiterator).Ifwe'readdinguserpages,wepinthemand*mapthemintothekernel.OnIOcompletion,thecallershouldputthose-*pages.Ifwe'readdingkernelpages,andthecallertoldusit'ssafeto-*doso,wejusthavetoaddthepagestothebiodirectly.Wedon'tgraban-*extrareferencetothosepages(theusershouldalreadyhavethat),andwe-*don'tputthepageonIOcompletion.Thecallerneedstocheckifthebiois-*flaggedBIO_NO_PAGE_REFonIOcompletion.Ifitisn't,thenpagesshouldbe-*released.+*pages.Forbvecbasediteratorsbio_iov_iter_get_pages()usestheprovided+*bvecsratherthancopyingthem.HenceanyoneissuingkiocbbasedIOneeds+*toensurethebvecsandpagesstayreferenceduntilthesubmittedI/Ois+*completedbyacallto->ki_complete()orreturnswithanerrorotherthan+*-EIOCBQUEUED.ThecallerneedstocheckifthebioisflaggedBIO_NO_PAGE_REF+*onIOcompletion.Ifitisn't,thenpagesshouldbereleased.**Thefunctiontries,butdoesnotguarantee,topinasmanypagesas*fitintothebio,orarerequestedin@iter,whateverissmaller.If
@@ -1105,27 +1101,22 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)*/intbio_iov_iter_get_pages(structbio*bio,structiov_iter*iter){-constboolis_bvec=iov_iter_is_bvec(iter);-intret;--if(WARN_ON_ONCE(bio->bi_vcnt))-return-EINVAL;+intret=0;-do{-if(bio_op(bio)==REQ_OP_ZONE_APPEND){-if(WARN_ON_ONCE(is_bvec))-return-EINVAL;-ret=__bio_iov_append_get_pages(bio,iter);-}else{-if(is_bvec)-ret=__bio_iov_bvec_add_pages(bio,iter);+if(iov_iter_is_bvec(iter)){+if(WARN_ON_ONCE(bio_op(bio)==REQ_OP_ZONE_APPEND))+return-EINVAL;+bio_iov_bvec_set(bio,iter);+bio_set_flag(bio,BIO_NO_PAGE_REF);+return0;+}else{+do{+if(bio_op(bio)==REQ_OP_ZONE_APPEND)+ret=__bio_iov_append_get_pages(bio,iter);elseret=__bio_iov_iter_get_pages(bio,iter);-}-}while(!ret&&iov_iter_count(iter)&&!bio_full(bio,0));--if(is_bvec)-bio_set_flag(bio,BIO_NO_PAGE_REF);+}while(!ret&&iov_iter_count(iter)&&!bio_full(bio,0));+}/* don't account direct I/O as memory stall */bio_clear_flag(bio,BIO_WORKINGSET);
From: Christoph Hellwig <hch@infradead.org> Date: 2021-01-04 16:20:22
On Sat, Jan 02, 2021 at 03:17:33PM +0000, Pavel Begunkov wrote:
quoted hunk
iter_file_splice_write() may spawn bvec segments with zero-length. In
preparation for prohibiting them, filter out by hand at splice level.
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
fs/splice.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
From: Christoph Hellwig <hch@infradead.org> Date: 2021-01-04 16:20:23
On Sat, Jan 02, 2021 at 03:17:34PM +0000, Pavel Begunkov wrote:
zero-length bvec segments are allowed in general, but not handled by bio
and down the block layer so filtered out. This inconsistency may be
confusing and prevent from optimisations. As zero-length segments are
useless and places that were generating them are patched, declare them
not allowed.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
@@ -865,3 +865,10 @@ no matter what. Everything is handled by the caller. clone_private_mount() returns a longterm mount now, so the proper destructor of its result is kern_unmount() or kern_unmount_array().++---++**mandatory**++zero-length bvec segments are disallowed, they must be filtered out before+passed on to an iterator.
Why are you putting this in filesystems/porting? Filesystems don't usually
generate bvecs ... there's nothing in this current series that stops them.
I'd suggest Documentation/block/biovecs.rst or biodoc.rst (and frankly,
biodoc.rst needs a good cleanup)
From: Pavel Begunkov <asml.silence@gmail.com> Date: 2021-01-04 16:59:34
On 04/01/2021 16:17, Christoph Hellwig wrote:
On Sat, Jan 02, 2021 at 03:17:33PM +0000, Pavel Begunkov wrote:
quoted
iter_file_splice_write() may spawn bvec segments with zero-length. In
preparation for prohibiting them, filter out by hand at splice level.
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
fs/splice.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
@@ -865,3 +865,10 @@ no matter what. Everything is handled by the caller. clone_private_mount() returns a longterm mount now, so the proper destructor of its result is kern_unmount() or kern_unmount_array().++---++**mandatory**++zero-length bvec segments are disallowed, they must be filtered out before+passed on to an iterator.
Why are you putting this in filesystems/porting? Filesystems don't usually
At least splice and a dozen of others do. As block apriori doesn't have them,
it's mainly addressed to fs + net.
generate bvecs ... there's nothing in this current series that stops them.
Yes, just mixing it with splice changes, which did all the work, looks
awkward to me. Would you suggest another segregation for the patches?
I'd suggest Documentation/block/biovecs.rst or biodoc.rst (and frankly,
biodoc.rst needs a good cleanup)