[PATCH 0/6] some block optimisations

[PATCH 0/6] some block optimisations

[Pavel Begunkov]

On top of perf-wip branch,

./io_uring -d32 -s32 -c32 -b512 -p1 /dev/nullb0
~3.3 MIOPS vs 3.5 MIOPS, so gives around extra ~4-5%.

The main part is caching struct block_device + some inlining.

Pavel Begunkov (6):
  block: cache bdev in struct file for raw bdev IO
  block: inline BDEV_I and friends
  blk-mq: optimise *end_request non-stat path
  block: inline hot paths of blk_account_io_*()
  blk-mq: inline hot part of __blk_mq_sched_restart
  block: convert ->bd_inode to container_of()

 block/bdev.c           | 16 ----------------
 block/blk-core.c       | 30 +++++++++---------------------
 block/blk-mq-sched.c   |  4 +---
 block/blk-mq-sched.h   |  8 +++++++-
 block/blk-mq.c         |  9 ++++-----
 block/blk.h            | 24 +++++++++++++++++++++---
 block/fops.c           | 40 ++++++++++++++++++++++------------------
 include/linux/blkdev.h | 31 +++++++++++++++++++++++++------
 8 files changed, 89 insertions(+), 73 deletions(-)

-- 
2.33.0

[PATCH 1/6] block: cache bdev in struct file for raw bdev IO

[Pavel Begunkov]

bdev = &BDEV_I(file->f_mapping->host)->bdev

Getting struct block_device from a file requires 2 memory dereferences
as illustrated above, that takes a toll on performance, so cache it in
yet unused file->private_data. That gives a noticeable peak performance
improvement.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/fops.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/block/fops.c b/block/fops.c
index 765086d51f8b..99e699427f31 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -17,11 +17,16 @@
 #include <linux/fs.h>
 #include "blk.h"
 
-static struct inode *bdev_file_inode(struct file *file)
+static inline struct inode *bdev_file_inode(struct file *file)
 {
 	return file->f_mapping->host;
 }
 
+static inline struct block_device *blkdev_get_bdev(struct file *file)
+{
+	return file->private_data;
+}
+
 static int blkdev_get_block(struct inode *inode, sector_t iblock,
 		struct buffer_head *bh, int create)
 {
@@ -54,8 +59,7 @@ static void blkdev_bio_end_io_simple(struct bio *bio)
 static ssize_t __blkdev_direct_IO_simple(struct kiocb *iocb,
 		struct iov_iter *iter, unsigned int nr_pages)
 {
-	struct file *file = iocb->ki_filp;
-	struct block_device *bdev = I_BDEV(bdev_file_inode(file));
+	struct block_device *bdev = blkdev_get_bdev(iocb->ki_filp);
 	struct bio_vec inline_vecs[DIO_INLINE_BIO_VECS], *vecs;
 	loff_t pos = iocb->ki_pos;
 	bool should_dirty = false;
@@ -143,7 +147,7 @@ static struct bio_set blkdev_dio_pool;
 
 static int blkdev_iopoll(struct kiocb *kiocb, struct io_batch *ib, bool wait)
 {
-	struct block_device *bdev = I_BDEV(kiocb->ki_filp->f_mapping->host);
+	struct block_device *bdev = blkdev_get_bdev(kiocb->ki_filp);
 	struct request_queue *q = bdev_get_queue(bdev);
 
 	return blk_poll(q, READ_ONCE(kiocb->ki_cookie), ib, wait);
@@ -191,9 +195,7 @@ static void blkdev_bio_end_io(struct bio *bio)
 static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
 		unsigned int nr_pages)
 {
-	struct file *file = iocb->ki_filp;
-	struct inode *inode = bdev_file_inode(file);
-	struct block_device *bdev = I_BDEV(inode);
+	struct block_device *bdev = blkdev_get_bdev(iocb->ki_filp);
 	struct blk_plug plug;
 	struct blkdev_dio *dio;
 	struct bio *bio;
@@ -405,8 +407,7 @@ static loff_t blkdev_llseek(struct file *file, loff_t offset, int whence)
 static int blkdev_fsync(struct file *filp, loff_t start, loff_t end,
 		int datasync)
 {
-	struct inode *bd_inode = bdev_file_inode(filp);
-	struct block_device *bdev = I_BDEV(bd_inode);
+	struct block_device *bdev = blkdev_get_bdev(filp);
 	int error;
 
 	error = file_write_and_wait_range(filp, start, end);
@@ -448,6 +449,8 @@ static int blkdev_open(struct inode *inode, struct file *filp)
 	bdev = blkdev_get_by_dev(inode->i_rdev, filp->f_mode, filp);
 	if (IS_ERR(bdev))
 		return PTR_ERR(bdev);
+
+	filp->private_data = bdev;
 	filp->f_mapping = bdev->bd_inode->i_mapping;
 	filp->f_wb_err = filemap_sample_wb_err(filp->f_mapping);
 	return 0;
@@ -455,7 +458,7 @@ static int blkdev_open(struct inode *inode, struct file *filp)
 
 static int blkdev_close(struct inode *inode, struct file *filp)
 {
-	struct block_device *bdev = I_BDEV(bdev_file_inode(filp));
+	struct block_device *bdev = blkdev_get_bdev(filp);
 
 	blkdev_put(bdev, filp->f_mode);
 	return 0;
@@ -463,7 +466,7 @@ static int blkdev_close(struct inode *inode, struct file *filp)
 
 static long block_ioctl(struct file *file, unsigned cmd, unsigned long arg)
 {
-	struct block_device *bdev = I_BDEV(bdev_file_inode(file));
+	struct block_device *bdev = blkdev_get_bdev(file);
 	fmode_t mode = file->f_mode;
 
 	/*
@@ -487,14 +490,14 @@ static long block_ioctl(struct file *file, unsigned cmd, unsigned long arg)
  */
 static ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
-	struct file *file = iocb->ki_filp;
-	struct inode *bd_inode = bdev_file_inode(file);
+	struct block_device *bdev = blkdev_get_bdev(iocb->ki_filp);
+	struct inode *bd_inode = bdev->bd_inode;
 	loff_t size = i_size_read(bd_inode);
 	struct blk_plug plug;
 	size_t shorted = 0;
 	ssize_t ret;
 
-	if (bdev_read_only(I_BDEV(bd_inode)))
+	if (bdev_read_only(bdev))
 		return -EPERM;
 
 	if (IS_SWAPFILE(bd_inode) && !is_hibernate_resume_dev(bd_inode->i_rdev))
@@ -526,9 +529,8 @@ static ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 
 static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 {
-	struct file *file = iocb->ki_filp;
-	struct inode *bd_inode = bdev_file_inode(file);
-	loff_t size = i_size_read(bd_inode);
+	struct block_device *bdev = blkdev_get_bdev(iocb->ki_filp);
+	loff_t size = (loff_t)bdev->bd_nr_sectors << SECTOR_SHIFT;
 	loff_t pos = iocb->ki_pos;
 	size_t shorted = 0;
 	ssize_t ret;
-- 
2.33.0

Re: [PATCH 1/6] block: cache bdev in struct file for raw bdev IO

[Jens Axboe]

On 10/9/21 6:25 AM, Pavel Begunkov wrote:
> bdev = &BDEV_I(file->f_mapping->host)->bdev
> 
> Getting struct block_device from a file requires 2 memory dereferences
> as illustrated above, that takes a toll on performance, so cache it in
> yet unused file->private_data. That gives a noticeable peak performance
> improvement.

It's hilariously bad right now, so I really welcome this change. One
comment:

> +static inline struct block_device *blkdev_get_bdev(struct file *file)
> +{
> +	return file->private_data;
> +}

Get rid of this and just use bdev = file->private_data where
appropriate. Easier to read, we don't need to hide this in a function.

-- 
Jens Axboe

Re: [PATCH 1/6] block: cache bdev in struct file for raw bdev IO

[Christoph Hellwig]

On Sat, Oct 09, 2021 at 10:33:17AM -0600, Jens Axboe wrote:
> > +static inline struct block_device *blkdev_get_bdev(struct file *file)
> > +{
> > +	return file->private_data;
> > +}
> 
> Get rid of this and just use bdev = file->private_data where
> appropriate. Easier to read, we don't need to hide this in a function.

100% agreed.

Re: [PATCH 1/6] block: cache bdev in struct file for raw bdev IO

[Pavel Begunkov]

On 10/11/21 09:26, Christoph Hellwig wrote:
> On Sat, Oct 09, 2021 at 10:33:17AM -0600, Jens Axboe wrote:
>>> +static inline struct block_device *blkdev_get_bdev(struct file *file)
>>> +{
>>> +	return file->private_data;
>>> +}
>>
>> Get rid of this and just use bdev = file->private_data where
>> appropriate. Easier to read, we don't need to hide this in a function.
> 
> 100% agreed.

The reasoning is as always, it's much easier to change if we change
what we store there. I don't agree, but don't care enough to stay
on the point, will resend with the change

-- 
Pavel Begunkov

[PATCH 2/6] block: inline BDEV_I and friends

[Pavel Begunkov]

I_BDEV and BDEV_I are very simple, they worth a single arith instruction
or can even be almost completely compiled out. Inline them.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/bdev.c           | 16 ----------------
 include/linux/blkdev.h | 16 +++++++++++++++-
 2 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index 567534c63f3d..a6cdfc49bc7e 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -30,22 +30,6 @@
 #include "../fs/internal.h"
 #include "blk.h"
 
-struct bdev_inode {
-	struct block_device bdev;
-	struct inode vfs_inode;
-};
-
-static inline struct bdev_inode *BDEV_I(struct inode *inode)
-{
-	return container_of(inode, struct bdev_inode, vfs_inode);
-}
-
-struct block_device *I_BDEV(struct inode *inode)
-{
-	return &BDEV_I(inode)->bdev;
-}
-EXPORT_SYMBOL(I_BDEV);
-
 static void bdev_write_inode(struct block_device *bdev)
 {
 	struct inode *inode = bdev->bd_inode;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4522acee81fb..591f14522f78 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1271,10 +1271,24 @@ void blkdev_put_no_open(struct block_device *bdev);
 
 struct block_device *bdev_alloc(struct gendisk *disk, u8 partno);
 void bdev_add(struct block_device *bdev, dev_t dev);
-struct block_device *I_BDEV(struct inode *inode);
 int truncate_bdev_range(struct block_device *bdev, fmode_t mode, loff_t lstart,
 		loff_t lend);
 
+struct bdev_inode {
+	struct block_device bdev;
+	struct inode vfs_inode;
+};
+
+static inline struct bdev_inode *BDEV_I(struct inode *inode)
+{
+	return container_of(inode, struct bdev_inode, vfs_inode);
+}
+
+static inline struct block_device *I_BDEV(struct inode *inode)
+{
+	return &BDEV_I(inode)->bdev;
+}
+
 #ifdef CONFIG_BLOCK
 void invalidate_bdev(struct block_device *bdev);
 int sync_blockdev(struct block_device *bdev);
-- 
2.33.0

Re: [PATCH 2/6] block: inline BDEV_I and friends

[Christoph Hellwig]

On Sat, Oct 09, 2021 at 01:25:39PM +0100, Pavel Begunkov wrote:
> I_BDEV and BDEV_I are very simple, they worth a single arith instruction
> or can even be almost completely compiled out. Inline them.

I see the benefit, but this is moving in the wrong direction.

struct bdev_inode is private to hide the struct inode.  Which at
the momen is a bit pointless given the bd_inode pointer in struct
block_device.

So we have two choices here that make sense:

 1) remove struct bdev_inode and kill the illusion that the inode
    is a private implementation detail
 2) remove direct references to bd_inode entirely.  Most of them are
    to i_size which already has proper helpers that should be used,
    and the rest can probably be covered with a handful wrappes or
    is bogus anyway

[PATCH 3/6] blk-mq: optimise *end_request non-stat path

[Pavel Begunkov]

We already have a blk_mq_need_time_stamp() check in
__blk_mq_end_request() to get a timestamp, hide all the statistics
accounting under it. It cuts some cycles for requests that don't need
stats, and is free otherwise.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/blk-mq.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 9430a0def2c9..c3da521efd35 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -584,12 +584,11 @@ static inline void __blk_mq_end_request_acct(struct request *rq,
 
 inline void __blk_mq_end_request(struct request *rq, blk_status_t error)
 {
-	u64 now = 0;
-
-	if (blk_mq_need_time_stamp(rq))
-		now = ktime_get_ns();
+	if (blk_mq_need_time_stamp(rq)) {
+		u64 now = ktime_get_ns();
 
-	__blk_mq_end_request_acct(rq, error, now);
+		__blk_mq_end_request_acct(rq, error, now);
+	}
 
 	if (rq->end_io) {
 		rq_qos_done(rq->q, rq);
-- 
2.33.0

[PATCH 4/6] block: inline hot paths of blk_account_io_*()

[Pavel Begunkov]

Extract hot paths of __blk_account_io_start() and
__blk_account_io_done() into inline functions, so we don't always pay
for function calls.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/blk-core.c | 30 +++++++++---------------------
 block/blk.h      | 24 +++++++++++++++++++++---
 2 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 9b8c70670190..6a9607a22589 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1172,8 +1172,7 @@ blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request *
 	if (blk_crypto_insert_cloned_request(rq))
 		return BLK_STS_IOERR;
 
-	if (blk_queue_io_stat(q))
-		blk_account_io_start(rq);
+	blk_account_io_start(rq);
 
 	/*
 	 * Since we have a scheduler attached on the top device,
@@ -1252,30 +1251,19 @@ static void blk_account_io_completion(struct request *req, unsigned int bytes)
 	}
 }
 
-void blk_account_io_done(struct request *req, u64 now)
+void __blk_account_io_done(struct request *req, u64 now)
 {
-	/*
-	 * Account IO completion.  flush_rq isn't accounted as a
-	 * normal IO on queueing nor completion.  Accounting the
-	 * containing request is enough.
-	 */
-	if (req->part && blk_do_io_stat(req) &&
-	    !(req->rq_flags & RQF_FLUSH_SEQ)) {
-		const int sgrp = op_stat_group(req_op(req));
+	const int sgrp = op_stat_group(req_op(req));
 
-		part_stat_lock();
-		update_io_ticks(req->part, jiffies, true);
-		part_stat_inc(req->part, ios[sgrp]);
-		part_stat_add(req->part, nsecs[sgrp], now - req->start_time_ns);
-		part_stat_unlock();
-	}
+	part_stat_lock();
+	update_io_ticks(req->part, jiffies, true);
+	part_stat_inc(req->part, ios[sgrp]);
+	part_stat_add(req->part, nsecs[sgrp], now - req->start_time_ns);
+	part_stat_unlock();
 }
 
-void blk_account_io_start(struct request *rq)
+void __blk_account_io_start(struct request *rq)
 {
-	if (!blk_do_io_stat(rq))
-		return;
-
 	/* passthrough requests can hold bios that do not have ->bi_bdev set */
 	if (rq->bio && rq->bio->bi_bdev)
 		rq->part = rq->bio->bi_bdev;
diff --git a/block/blk.h b/block/blk.h
index 38867b4c5c7e..5d74270314ea 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -219,8 +219,8 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
 bool blk_bio_list_merge(struct request_queue *q, struct list_head *list,
 			struct bio *bio, unsigned int nr_segs);
 
-void blk_account_io_start(struct request *req);
-void blk_account_io_done(struct request *req, u64 now);
+void __blk_account_io_start(struct request *req);
+void __blk_account_io_done(struct request *req, u64 now);
 
 /*
  * Plug flush limits
@@ -284,7 +284,25 @@ int blk_dev_init(void);
  */
 static inline bool blk_do_io_stat(struct request *rq)
 {
-	return rq->rq_disk && (rq->rq_flags & RQF_IO_STAT);
+	return (rq->rq_flags & RQF_IO_STAT) && rq->rq_disk;
+}
+
+static inline void blk_account_io_done(struct request *req, u64 now)
+{
+	/*
+	 * Account IO completion.  flush_rq isn't accounted as a
+	 * normal IO on queueing nor completion.  Accounting the
+	 * containing request is enough.
+	 */
+	if (blk_do_io_stat(req) && req->part &&
+	    !(req->rq_flags & RQF_FLUSH_SEQ))
+		__blk_account_io_done(req, now);
+}
+
+static inline void blk_account_io_start(struct request *req)
+{
+	if (blk_do_io_stat(req))
+		__blk_account_io_start(req);
 }
 
 static inline void req_set_nomerge(struct request_queue *q, struct request *req)
-- 
2.33.0

[PATCH 5/6] blk-mq: inline hot part of __blk_mq_sched_restart

[Pavel Begunkov]

Extract a fast check out of __block_mq_sched_restart() and inline it for
performance reasons.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/blk-mq-sched.c | 4 +---
 block/blk-mq-sched.h | 8 +++++++-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 27312da7d638..efc1374b8831 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -57,10 +57,8 @@ void blk_mq_sched_mark_restart_hctx(struct blk_mq_hw_ctx *hctx)
 }
 EXPORT_SYMBOL_GPL(blk_mq_sched_mark_restart_hctx);
 
-void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
+void __blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
 {
-	if (!test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state))
-		return;
 	clear_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
 
 	/*
diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
index e4d367e0b2a3..c97b816c3800 100644
--- a/block/blk-mq-sched.h
+++ b/block/blk-mq-sched.h
@@ -17,7 +17,7 @@ bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
 bool blk_mq_sched_try_insert_merge(struct request_queue *q, struct request *rq,
 				   struct list_head *free);
 void blk_mq_sched_mark_restart_hctx(struct blk_mq_hw_ctx *hctx);
-void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx);
+void __blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx);
 
 void blk_mq_sched_insert_request(struct request *rq, bool at_head,
 				 bool run_queue, bool async);
@@ -31,6 +31,12 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e);
 void blk_mq_exit_sched(struct request_queue *q, struct elevator_queue *e);
 void blk_mq_sched_free_rqs(struct request_queue *q);
 
+static inline void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
+{
+	if (test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state))
+		__blk_mq_sched_restart(hctx);
+}
+
 static inline bool
 blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
 		unsigned int nr_segs)
-- 
2.33.0

[PATCH 6/6] block: convert ->bd_inode to container_of()

[Pavel Begunkov]

We don't need bdev->bd_inode as we know the layout, they are stored in
the same structure and so offset_of is enough. Convert extra
dereferencing to offseting starting from the block layer.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/fops.c           | 10 ++++++----
 include/linux/blkdev.h | 15 ++++++++++-----
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/block/fops.c b/block/fops.c
index 99e699427f31..5438ed9cfcf7 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -17,14 +17,16 @@
 #include <linux/fs.h>
 #include "blk.h"
 
-static inline struct inode *bdev_file_inode(struct file *file)
+static inline struct block_device *blkdev_get_bdev(struct file *file)
 {
-	return file->f_mapping->host;
+	return file->private_data;
 }
 
-static inline struct block_device *blkdev_get_bdev(struct file *file)
+static inline struct inode *bdev_file_inode(struct file *file)
 {
-	return file->private_data;
+	struct block_device *bdev = blkdev_get_bdev(file);
+
+	return bdev_get_inode(bdev);
 }
 
 static int blkdev_get_block(struct inode *inode, sector_t iblock,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 591f14522f78..a56f3a852206 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1139,11 +1139,6 @@ static inline unsigned int blksize_bits(unsigned int size)
 	return bits;
 }
 
-static inline unsigned int block_size(struct block_device *bdev)
-{
-	return 1 << bdev->bd_inode->i_blkbits;
-}
-
 int kblockd_schedule_work(struct work_struct *work);
 int kblockd_mod_delayed_work_on(int cpu, struct delayed_work *dwork, unsigned long delay);
 
@@ -1289,6 +1284,16 @@ static inline struct block_device *I_BDEV(struct inode *inode)
 	return &BDEV_I(inode)->bdev;
 }
 
+static inline struct inode *bdev_get_inode(struct block_device *bdev)
+{
+	return &container_of(bdev, struct bdev_inode, bdev)->vfs_inode;
+}
+
+static inline unsigned int block_size(struct block_device *bdev)
+{
+	return 1 << bdev_get_inode(bdev)->i_blkbits;
+}
+
 #ifdef CONFIG_BLOCK
 void invalidate_bdev(struct block_device *bdev);
 int sync_blockdev(struct block_device *bdev);
-- 
2.33.0

Re: [PATCH 6/6] block: convert ->bd_inode to container_of()

[Christoph Hellwig]

On Sat, Oct 09, 2021 at 01:25:43PM +0100, Pavel Begunkov wrote:
> +static inline struct inode *bdev_file_inode(struct file *file)
>  {
> +	struct block_device *bdev = blkdev_get_bdev(file);
> +
> +	return bdev_get_inode(bdev);
>  }

No need for this helper either.

> +static inline struct inode *bdev_get_inode(struct block_device *bdev)
> +{
> +	return &container_of(bdev, struct bdev_inode, bdev)->vfs_inode;
> +}

This is rather misnamed, not get anywhere in here.