diff mbox series

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

Message ID cfc66d9946422fa1778504f976621c91be2befb5.1633781740.git.asml.silence@gmail.com (mailing list archive)
State New, archived
Headers show
Series some block optimisations | expand

Commit Message

Pavel Begunkov Oct. 9, 2021, 12:25 p.m. UTC 
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(-)

Comments

Jens Axboe Oct. 9, 2021, 4:33 p.m. UTC | #1 
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.
Christoph Hellwig Oct. 11, 2021, 8:26 a.m. UTC | #2 
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.
Pavel Begunkov Oct. 13, 2021, 8:45 a.m. UTC | #3 
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
diff mbox series

Patch

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;