[PATCH v2 0/3] on top of for-5.16/block

[PATCH v2 0/3] on top of for-5.16/block

[Pavel Begunkov]

./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.

v2: without applied patches, merge previous 6/6 into the second patch
    get rid of helpers (Jens, Christoph)
    kill bdev_inode and move inode into bdev (Christoph)

Pavel Begunkov (3):
  block: cache bdev in struct file for raw bdev IO
  block: don't hide inode from block_device users
  blk-mq: optimise *end_request non-stat path

 block/bdev.c              | 44 ++++++++++-----------------------------
 block/blk-mq.c            | 18 +++++++---------
 block/fops.c              | 43 ++++++++++++++++----------------------
 include/linux/blk_types.h |  1 +
 include/linux/blkdev.h    |  8 +++++--
 5 files changed, 44 insertions(+), 70 deletions(-)

-- 
2.33.0

[PATCH v2 1/3] 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 | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/block/fops.c b/block/fops.c
index 7bb9581a146c..c71e91cd6bcb 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -17,7 +17,7 @@
 #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;
 }
@@ -54,8 +54,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 = iocb->ki_filp->private_data;
 	struct bio_vec inline_vecs[DIO_INLINE_BIO_VECS], *vecs;
 	loff_t pos = iocb->ki_pos;
 	bool should_dirty = false;
@@ -143,7 +142,7 @@ static struct bio_set blkdev_dio_pool;
 
 static int blkdev_iopoll(struct kiocb *kiocb, bool wait)
 {
-	struct block_device *bdev = I_BDEV(kiocb->ki_filp->f_mapping->host);
+	struct block_device *bdev = kiocb->ki_filp->private_data;
 	struct request_queue *q = bdev_get_queue(bdev);
 
 	return blk_poll(q, READ_ONCE(kiocb->ki_cookie), wait);
@@ -191,9 +190,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 = iocb->ki_filp->private_data;
 	struct blk_plug plug;
 	struct blkdev_dio *dio;
 	struct bio *bio;
@@ -405,8 +402,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 = filp->private_data;
 	int error;
 
 	error = file_write_and_wait_range(filp, start, end);
@@ -448,6 +444,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 +453,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 = filp->private_data;
 
 	blkdev_put(bdev, filp->f_mode);
 	return 0;
@@ -470,14 +468,14 @@ static int blkdev_close(struct inode *inode, struct file *filp)
  */
 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 = iocb->ki_filp->private_data;
+	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))
@@ -509,9 +507,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 = iocb->ki_filp->private_data;
+	loff_t size = i_size_read(bdev->bd_inode);
 	loff_t pos = iocb->ki_pos;
 	size_t shorted = 0;
 	ssize_t ret;
-- 
2.33.0

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

[Christoph Hellwig]

On Wed, Oct 13, 2021 at 09:57:11AM +0100, 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.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  block/fops.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/block/fops.c b/block/fops.c
> index 7bb9581a146c..c71e91cd6bcb 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -17,7 +17,7 @@
>  #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;
>  }

This is unrelated to the rest.

But the actual described change looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 2/3] block: don't hide inode from block_device users

[Pavel Begunkov]

Instead of tricks with struct bdev_inode, just openly place the inode
inside struct block_device. First, it allows us to inline I_BDEV, which
is simple but non-inline nature of it impacts performance. Also, make it
possible to get rid of ->bd_inode pointer and hooping with extra
indirection, the amount of which became a noticeable problem for the
block layer.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/bdev.c              | 44 ++++++++++-----------------------------
 block/fops.c              | 20 +++++++-----------
 include/linux/blk_types.h |  1 +
 include/linux/blkdev.h    |  8 +++++--
 4 files changed, 26 insertions(+), 47 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index 567534c63f3d..541e24d240d1 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;
@@ -389,12 +373,13 @@ static struct kmem_cache * bdev_cachep __read_mostly;
 
 static struct inode *bdev_alloc_inode(struct super_block *sb)
 {
-	struct bdev_inode *ei = kmem_cache_alloc(bdev_cachep, GFP_KERNEL);
+	struct block_device *bdev = kmem_cache_alloc(bdev_cachep, GFP_KERNEL);
 
-	if (!ei)
+	if (!bdev)
 		return NULL;
-	memset(&ei->bdev, 0, sizeof(ei->bdev));
-	return &ei->vfs_inode;
+	memset(bdev, 0, sizeof(*bdev));
+	inode_init_once(&bdev->inode);
+	return &bdev->inode;
 }
 
 static void bdev_free_inode(struct inode *inode)
@@ -413,14 +398,7 @@ static void bdev_free_inode(struct inode *inode)
 	if (MAJOR(bdev->bd_dev) == BLOCK_EXT_MAJOR)
 		blk_free_ext_minor(MINOR(bdev->bd_dev));
 
-	kmem_cache_free(bdev_cachep, BDEV_I(inode));
-}
-
-static void init_once(void *data)
-{
-	struct bdev_inode *ei = data;
-
-	inode_init_once(&ei->vfs_inode);
+	kmem_cache_free(bdev_cachep, bdev);
 }
 
 static void bdev_evict_inode(struct inode *inode)
@@ -462,10 +440,10 @@ void __init bdev_cache_init(void)
 	int err;
 	static struct vfsmount *bd_mnt;
 
-	bdev_cachep = kmem_cache_create("bdev_cache", sizeof(struct bdev_inode),
-			0, (SLAB_HWCACHE_ALIGN|SLAB_RECLAIM_ACCOUNT|
-				SLAB_MEM_SPREAD|SLAB_ACCOUNT|SLAB_PANIC),
-			init_once);
+	bdev_cachep = kmem_cache_create("bdev_cache",
+			sizeof(struct block_device), 0,
+			(SLAB_HWCACHE_ALIGN|SLAB_RECLAIM_ACCOUNT|
+				SLAB_MEM_SPREAD|SLAB_ACCOUNT|SLAB_PANIC), NULL);
 	err = register_filesystem(&bd_type);
 	if (err)
 		panic("Cannot register bdev pseudo-fs");
@@ -744,7 +722,7 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 	}
 
 	/* switch from the inode reference to a device mode one: */
-	bdev = &BDEV_I(inode)->bdev;
+	bdev = I_BDEV(inode);
 	if (!kobject_get_unless_zero(&bdev->bd_device.kobj))
 		bdev = NULL;
 	iput(inode);
diff --git a/block/fops.c b/block/fops.c
index c71e91cd6bcb..9ae795f27f74 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -17,11 +17,6 @@
 #include <linux/fs.h>
 #include "blk.h"
 
-static inline struct inode *bdev_file_inode(struct file *file)
-{
-	return file->f_mapping->host;
-}
-
 static int blkdev_get_block(struct inode *inode, sector_t iblock,
 		struct buffer_head *bh, int create)
 {
@@ -390,7 +385,8 @@ const struct address_space_operations def_blk_aops = {
  */
 static loff_t blkdev_llseek(struct file *file, loff_t offset, int whence)
 {
-	struct inode *bd_inode = bdev_file_inode(file);
+	struct block_device *bdev = file->private_data;
+	struct inode *bd_inode = &bdev->inode;
 	loff_t retval;
 
 	inode_lock(bd_inode);
@@ -446,7 +442,7 @@ static int blkdev_open(struct inode *inode, struct file *filp)
 		return PTR_ERR(bdev);
 
 	filp->private_data = bdev;
-	filp->f_mapping = bdev->bd_inode->i_mapping;
+	filp->f_mapping = bdev->inode.i_mapping;
 	filp->f_wb_err = filemap_sample_wb_err(filp->f_mapping);
 	return 0;
 }
@@ -469,7 +465,7 @@ static int blkdev_close(struct inode *inode, struct file *filp)
 static ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 	struct block_device *bdev = iocb->ki_filp->private_data;
-	struct inode *bd_inode = bdev->bd_inode;
+	struct inode *bd_inode = &bdev->inode;
 	loff_t size = i_size_read(bd_inode);
 	struct blk_plug plug;
 	size_t shorted = 0;
@@ -508,7 +504,7 @@ 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 block_device *bdev = iocb->ki_filp->private_data;
-	loff_t size = i_size_read(bdev->bd_inode);
+	loff_t size = i_size_read(&bdev->inode);
 	loff_t pos = iocb->ki_pos;
 	size_t shorted = 0;
 	ssize_t ret;
@@ -534,8 +530,8 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 static long blkdev_fallocate(struct file *file, int mode, loff_t start,
 			     loff_t len)
 {
-	struct inode *inode = bdev_file_inode(file);
-	struct block_device *bdev = I_BDEV(inode);
+	struct block_device *bdev = file->private_data;
+	struct inode *inode = &bdev->inode;
 	loff_t end = start + len - 1;
 	loff_t isize;
 	int error;
@@ -545,7 +541,7 @@ static long blkdev_fallocate(struct file *file, int mode, loff_t start,
 		return -EOPNOTSUPP;
 
 	/* Don't go off the end of the device. */
-	isize = i_size_read(bdev->bd_inode);
+	isize = i_size_read(inode);
 	if (start >= isize)
 		return -EINVAL;
 	if (end >= isize) {
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 3b967053e9f5..e94d08eb4fa9 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -49,6 +49,7 @@ struct block_device {
 #ifdef CONFIG_FAIL_MAKE_REQUEST
 	bool			bd_make_it_fail;
 #endif
+	struct inode		inode;
 } __randomize_layout;
 
 #define bdev_whole(_bdev) \
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 17705c970d7e..75d2682a8a55 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1141,7 +1141,7 @@ static inline unsigned int blksize_bits(unsigned int size)
 
 static inline unsigned int block_size(struct block_device *bdev)
 {
-	return 1 << bdev->bd_inode->i_blkbits;
+	return 1 << bdev->inode.i_blkbits;
 }
 
 int kblockd_schedule_work(struct work_struct *work);
@@ -1271,10 +1271,14 @@ 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);
 
+static inline struct block_device *I_BDEV(struct inode *inode)
+{
+	return container_of(inode, struct block_device, inode);
+}
+
 #ifdef CONFIG_BLOCK
 void invalidate_bdev(struct block_device *bdev);
 int sync_blockdev(struct block_device *bdev);
-- 
2.33.0

Re: [PATCH v2 2/3] block: don't hide inode from block_device users

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3735 bytes --]

Hi Pavel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v5.15-rc5]
[cannot apply to axboe-block/for-next next-20211013]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Pavel-Begunkov/on-top-of-for-5-16-block/20211013-165953
base:    64570fbc14f8d7cb3fe3995f20e26bc25ce4b2cc
config: um-x86_64_defconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/9e538de87cc869bd26a3ca78da49d1437ec8688c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pavel-Begunkov/on-top-of-for-5-16-block/20211013-165953
        git checkout 9e538de87cc869bd26a3ca78da49d1437ec8688c
        # save the attached .config to linux build tree
        make W=1 ARCH=um SUBARCH=x86_64

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   block/fops.c: In function 'block_ioctl':
   block/fops.c:460:37: error: implicit declaration of function 'bdev_file_inode'; did you mean 'file_inode'? [-Werror=implicit-function-declaration]
     460 |  struct block_device *bdev = I_BDEV(bdev_file_inode(file));
         |                                     ^~~~~~~~~~~~~~~
         |                                     file_inode
>> block/fops.c:460:37: warning: passing argument 1 of 'I_BDEV' makes pointer from integer without a cast [-Wint-conversion]
     460 |  struct block_device *bdev = I_BDEV(bdev_file_inode(file));
         |                                     ^~~~~~~~~~~~~~~~~~~~~
         |                                     |
         |                                     int
   In file included from block/fops.c:9:
   include/linux/blkdev.h:1990:57: note: expected 'struct inode *' but argument is of type 'int'
    1990 | static inline struct block_device *I_BDEV(struct inode *inode)
         |                                           ~~~~~~~~~~~~~~^~~~~
   cc1: some warnings being treated as errors


vim +/I_BDEV +460 block/fops.c

cd82cca7ebfe9c Christoph Hellwig 2021-09-07  457  
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  458  static long block_ioctl(struct file *file, unsigned cmd, unsigned long arg)
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  459  {
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 @460  	struct block_device *bdev = I_BDEV(bdev_file_inode(file));
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  461  	fmode_t mode = file->f_mode;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  462  
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  463  	/*
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  464  	 * O_NDELAY can be altered using fcntl(.., F_SETFL, ..), so we have
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  465  	 * to updated it before every ioctl.
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  466  	 */
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  467  	if (file->f_flags & O_NDELAY)
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  468  		mode |= FMODE_NDELAY;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  469  	else
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  470  		mode &= ~FMODE_NDELAY;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  471  
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  472  	return blkdev_ioctl(bdev, mode, cmd, arg);
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  473  }
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  474  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 9710 bytes --]

Re: [PATCH v2 2/3] block: don't hide inode from block_device users

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3923 bytes --]

Hi Pavel,

I love your patch! Yet something to improve:

[auto build test ERROR on v5.15-rc5]
[cannot apply to axboe-block/for-next next-20211013]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Pavel-Begunkov/on-top-of-for-5-16-block/20211013-165953
base:    64570fbc14f8d7cb3fe3995f20e26bc25ce4b2cc
config: hexagon-randconfig-r045-20211013 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project b6a8c695542b2987eb9a203d5663a0740cb4725f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/9e538de87cc869bd26a3ca78da49d1437ec8688c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pavel-Begunkov/on-top-of-for-5-16-block/20211013-165953
        git checkout 9e538de87cc869bd26a3ca78da49d1437ec8688c
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

>> block/fops.c:460:37: error: implicit declaration of function 'bdev_file_inode' [-Werror,-Wimplicit-function-declaration]
           struct block_device *bdev = I_BDEV(bdev_file_inode(file));
                                              ^
   block/fops.c:460:37: note: did you mean 'file_inode'?
   include/linux/fs.h:1348:29: note: 'file_inode' declared here
   static inline struct inode *file_inode(const struct file *f)
                               ^
>> block/fops.c:460:37: warning: incompatible integer to pointer conversion passing 'int' to parameter of type 'struct inode *' [-Wint-conversion]
           struct block_device *bdev = I_BDEV(bdev_file_inode(file));
                                              ^~~~~~~~~~~~~~~~~~~~~
   include/linux/blkdev.h:1990:57: note: passing argument to parameter 'inode' here
   static inline struct block_device *I_BDEV(struct inode *inode)
                                                           ^
   1 warning and 1 error generated.


vim +/bdev_file_inode +460 block/fops.c

cd82cca7ebfe9c Christoph Hellwig 2021-09-07  457  
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  458  static long block_ioctl(struct file *file, unsigned cmd, unsigned long arg)
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  459  {
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 @460  	struct block_device *bdev = I_BDEV(bdev_file_inode(file));
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  461  	fmode_t mode = file->f_mode;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  462  
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  463  	/*
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  464  	 * O_NDELAY can be altered using fcntl(.., F_SETFL, ..), so we have
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  465  	 * to updated it before every ioctl.
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  466  	 */
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  467  	if (file->f_flags & O_NDELAY)
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  468  		mode |= FMODE_NDELAY;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  469  	else
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  470  		mode &= ~FMODE_NDELAY;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  471  
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  472  	return blkdev_ioctl(bdev, mode, cmd, arg);
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  473  }
cd82cca7ebfe9c Christoph Hellwig 2021-09-07  474  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28686 bytes --]

Re: [PATCH v2 2/3] block: don't hide inode from block_device users

[Pavel Begunkov]

On 10/13/21 16:03, kernel test robot wrote:
> Hi Pavel,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on v5.15-rc5]

It's against for-5.16/block and there is no more block_ioctl() in
fops.c, should be fine


> [cannot apply to axboe-block/for-next next-20211013]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Pavel-Begunkov/on-top-of-for-5-16-block/20211013-165953
> base:    64570fbc14f8d7cb3fe3995f20e26bc25ce4b2cc
> config: hexagon-randconfig-r045-20211013 (attached as .config)
> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project b6a8c695542b2987eb9a203d5663a0740cb4725f)
> reproduce (this is a W=1 build):
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # https://github.com/0day-ci/linux/commit/9e538de87cc869bd26a3ca78da49d1437ec8688c
>          git remote add linux-review https://github.com/0day-ci/linux
>          git fetch --no-tags linux-review Pavel-Begunkov/on-top-of-for-5-16-block/20211013-165953
>          git checkout 9e538de87cc869bd26a3ca78da49d1437ec8688c
>          # save the attached .config to linux build tree
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All error/warnings (new ones prefixed by >>):
> 
>>> block/fops.c:460:37: error: implicit declaration of function 'bdev_file_inode' [-Werror,-Wimplicit-function-declaration]
>             struct block_device *bdev = I_BDEV(bdev_file_inode(file));
>                                                ^
>     block/fops.c:460:37: note: did you mean 'file_inode'?
>     include/linux/fs.h:1348:29: note: 'file_inode' declared here
>     static inline struct inode *file_inode(const struct file *f)
>                                 ^
>>> block/fops.c:460:37: warning: incompatible integer to pointer conversion passing 'int' to parameter of type 'struct inode *' [-Wint-conversion]
>             struct block_device *bdev = I_BDEV(bdev_file_inode(file));
>                                                ^~~~~~~~~~~~~~~~~~~~~
>     include/linux/blkdev.h:1990:57: note: passing argument to parameter 'inode' here
>     static inline struct block_device *I_BDEV(struct inode *inode)
>                                                             ^
>     1 warning and 1 error generated.
> 
> 
> vim +/bdev_file_inode +460 block/fops.c
> 
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  457
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  458  static long block_ioctl(struct file *file, unsigned cmd, unsigned long arg)
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  459  {
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07 @460  	struct block_device *bdev = I_BDEV(bdev_file_inode(file));
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  461  	fmode_t mode = file->f_mode;
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  462
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  463  	/*
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  464  	 * O_NDELAY can be altered using fcntl(.., F_SETFL, ..), so we have
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  465  	 * to updated it before every ioctl.
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  466  	 */
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  467  	if (file->f_flags & O_NDELAY)
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  468  		mode |= FMODE_NDELAY;
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  469  	else
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  470  		mode &= ~FMODE_NDELAY;
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  471
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  472  	return blkdev_ioctl(bdev, mode, cmd, arg);
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  473  }
> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  474
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 

-- 
Pavel Begunkov

Re: [kbuild-all] Re: [PATCH v2 2/3] block: don't hide inode from block_device users

[Chen, Rong A]

On 10/13/2021 11:12 PM, Pavel Begunkov wrote:
> On 10/13/21 16:03, kernel test robot wrote:
>> Hi Pavel,
>>
>> I love your patch! Yet something to improve:
>>
>> [auto build test ERROR on v5.15-rc5]
> 
> It's against for-5.16/block and there is no more block_ioctl() in
> fops.c, should be fine

Hi Pavel,

Thanks for the feedback, we'll take a look.

Best Regards,
Rong Chen

> 
> 
>> [cannot apply to axboe-block/for-next next-20211013]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url:    
>> https://github.com/0day-ci/linux/commits/Pavel-Begunkov/on-top-of-for-5-16-block/20211013-165953 
>>
>> base:    64570fbc14f8d7cb3fe3995f20e26bc25ce4b2cc
>> config: hexagon-randconfig-r045-20211013 (attached as .config)
>> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 
>> b6a8c695542b2987eb9a203d5663a0740cb4725f)
>> reproduce (this is a W=1 build):
>>          wget 
>> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross 
>> -O ~/bin/make.cross
>>          chmod +x ~/bin/make.cross
>>          # 
>> https://github.com/0day-ci/linux/commit/9e538de87cc869bd26a3ca78da49d1437ec8688c 
>>
>>          git remote add linux-review https://github.com/0day-ci/linux
>>          git fetch --no-tags linux-review 
>> Pavel-Begunkov/on-top-of-for-5-16-block/20211013-165953
>>          git checkout 9e538de87cc869bd26a3ca78da49d1437ec8688c
>>          # save the attached .config to linux build tree
>>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross 
>> W=1 ARCH=hexagon
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> All error/warnings (new ones prefixed by >>):
>>
>>>> block/fops.c:460:37: error: implicit declaration of function 
>>>> 'bdev_file_inode' [-Werror,-Wimplicit-function-declaration]
>>             struct block_device *bdev = I_BDEV(bdev_file_inode(file));
>>                                                ^
>>     block/fops.c:460:37: note: did you mean 'file_inode'?
>>     include/linux/fs.h:1348:29: note: 'file_inode' declared here
>>     static inline struct inode *file_inode(const struct file *f)
>>                                 ^
>>>> block/fops.c:460:37: warning: incompatible integer to pointer 
>>>> conversion passing 'int' to parameter of type 'struct inode *' 
>>>> [-Wint-conversion]
>>             struct block_device *bdev = I_BDEV(bdev_file_inode(file));
>>                                                ^~~~~~~~~~~~~~~~~~~~~
>>     include/linux/blkdev.h:1990:57: note: passing argument to 
>> parameter 'inode' here
>>     static inline struct block_device *I_BDEV(struct inode *inode)
>>                                                             ^
>>     1 warning and 1 error generated.
>>
>>
>> vim +/bdev_file_inode +460 block/fops.c
>>
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  457
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  458  static long 
>> block_ioctl(struct file *file, unsigned cmd, unsigned long arg)
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  459  {
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07 @460      struct 
>> block_device *bdev = I_BDEV(bdev_file_inode(file));
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  461      fmode_t mode = 
>> file->f_mode;
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  462
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  463      /*
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  464       * O_NDELAY can 
>> be altered using fcntl(.., F_SETFL, ..), so we have
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  465       * to updated it 
>> before every ioctl.
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  466       */
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  467      if 
>> (file->f_flags & O_NDELAY)
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  468          mode |= 
>> FMODE_NDELAY;
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  469      else
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  470          mode &= 
>> ~FMODE_NDELAY;
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  471
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  472      return 
>> blkdev_ioctl(bdev, mode, cmd, arg);
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  473  }
>> cd82cca7ebfe9c Christoph Hellwig 2021-09-07  474
>>
>> ---
>> 0-DAY CI Kernel Test Service, Intel Corporation
>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>>
> 

Re: [PATCH v2 2/3] block: don't hide inode from block_device users

[Christoph Hellwig]

On Wed, Oct 13, 2021 at 09:57:12AM +0100, Pavel Begunkov wrote:
> Instead of tricks with struct bdev_inode, just openly place the inode
> inside struct block_device. First, it allows us to inline I_BDEV, which
> is simple but non-inline nature of it impacts performance. Also, make it
> possible to get rid of ->bd_inode pointer and hooping with extra
> indirection, the amount of which became a noticeable problem for the
> block layer.

What fast path outside of bdev.c cares about an inlined I_BDEV?
I have dusted off patches to reduce (and hopefully eventually kill)
accesses to bd_inode outside of bdev.c, so this goes into the wrong
direction.

If needed I'd rather figure out a way to fix any smoking gun without
this change.

Re: [PATCH v2 2/3] block: don't hide inode from block_device users

[Pavel Begunkov]

On 10/13/21 16:25, Christoph Hellwig wrote:
> On Wed, Oct 13, 2021 at 09:57:12AM +0100, Pavel Begunkov wrote:
>> Instead of tricks with struct bdev_inode, just openly place the inode
>> inside struct block_device. First, it allows us to inline I_BDEV, which
>> is simple but non-inline nature of it impacts performance. Also, make it
>> possible to get rid of ->bd_inode pointer and hooping with extra
>> indirection, the amount of which became a noticeable problem for the
>> block layer.
> 
> What fast path outside of bdev.c cares about an inlined I_BDEV?

Mildly hot in io_uring w/o fixed files, but that's not peak perf,
but would also be great to get rid of bdev->bd_inode dereference,
e.g. lots of in fops.c.

Are you going to just hid the dereference in helpers or kill it
with some offseting magic?


> I have dusted off patches to reduce (and hopefully eventually kill)
> accesses to bd_inode outside of bdev.c, so this goes into the wrong
> direction.
> 
> If needed I'd rather figure out a way to fix any smoking gun without
> this change.
> 

-- 
Pavel Begunkov

Re: [PATCH v2 2/3] block: don't hide inode from block_device users

[Christoph Hellwig]

On Wed, Oct 13, 2021 at 07:44:20PM +0100, Pavel Begunkov wrote:
> Mildly hot in io_uring w/o fixed files, but that's not peak perf,
> but would also be great to get rid of bdev->bd_inode dereference,
> e.g. lots of in fops.c.
> 
> Are you going to just hid the dereference in helpers or kill it
> with some offseting magic?

The only real hot path uses I found is the size (which you and Jens
already seem to have moved to use something out of the inode for the
fast path),  and maybe the blkbits for which we could do the same.

So basically the idea is to not touch the inode in the hot path,
and use accessors helpers in bdev.c that could do the offsetof trick.

Re: [PATCH v2 2/3] block: don't hide inode from block_device users

[Pavel Begunkov]

On 10/14/21 05:51, Christoph Hellwig wrote:
> On Wed, Oct 13, 2021 at 07:44:20PM +0100, Pavel Begunkov wrote:
>> Mildly hot in io_uring w/o fixed files, but that's not peak perf,
>> but would also be great to get rid of bdev->bd_inode dereference,
>> e.g. lots of in fops.c.
>>
>> Are you going to just hid the dereference in helpers or kill it
>> with some offseting magic?
> 
> The only real hot path uses I found is the size (which you and Jens
> already seem to have moved to use something out of the inode for the
> fast path),  and maybe the blkbits for which we could do the same.
> 
> So basically the idea is to not touch the inode in the hot path,
> and use accessors helpers in bdev.c that could do the offsetof trick.

I see, sounds good

-- 
Pavel Begunkov

[PATCH v2 3/3] 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 | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 38e6651d8b94..3dd3b22bc280 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -572,20 +572,18 @@ void blk_mq_free_plug_rqs(struct blk_plug *plug)
 
 inline void __blk_mq_end_request(struct request *rq, blk_status_t error)
 {
-	u64 now = 0;
+	if (blk_mq_need_time_stamp(rq)) {
+		u64 now = ktime_get_ns();
 
-	if (blk_mq_need_time_stamp(rq))
-		now = ktime_get_ns();
+		if (rq->rq_flags & RQF_STATS) {
+			blk_mq_poll_stats_start(rq->q);
+			blk_stat_add(rq, now);
+		}
 
-	if (rq->rq_flags & RQF_STATS) {
-		blk_mq_poll_stats_start(rq->q);
-		blk_stat_add(rq, now);
+		blk_mq_sched_completed_request(rq, now);
+		blk_account_io_done(rq, now);
 	}
 
-	blk_mq_sched_completed_request(rq, now);
-
-	blk_account_io_done(rq, now);
-
 	if (rq->end_io) {
 		rq_qos_done(rq->q, rq);
 		rq->end_io(rq, error);
-- 
2.33.0

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

[Christoph Hellwig]

On Wed, Oct 13, 2021 at 09:57:13AM +0100, Pavel Begunkov wrote:
> 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.

The transformation is a little non-obvious due to the checks hidden
inblk_mq_need_time_stamp, but after following all the spaghetti it does
looks like a correct transformation:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 0/3] on top of for-5.16/block

[Pavel Begunkov]

On 10/13/21 09:57, Pavel Begunkov wrote:

Bodged subject, should've been block optimisation or so.

> ./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.
> 
> v2: without applied patches, merge previous 6/6 into the second patch
>      get rid of helpers (Jens, Christoph)
>      kill bdev_inode and move inode into bdev (Christoph)
> 
> Pavel Begunkov (3):
>    block: cache bdev in struct file for raw bdev IO
>    block: don't hide inode from block_device users
>    blk-mq: optimise *end_request non-stat path
> 
>   block/bdev.c              | 44 ++++++++++-----------------------------
>   block/blk-mq.c            | 18 +++++++---------
>   block/fops.c              | 43 ++++++++++++++++----------------------
>   include/linux/blk_types.h |  1 +
>   include/linux/blkdev.h    |  8 +++++--
>   5 files changed, 44 insertions(+), 70 deletions(-)
> 

-- 
Pavel Begunkov

Re: (subset) [PATCH v2 0/3] on top of for-5.16/block

[Jens Axboe]

On Wed, 13 Oct 2021 09:57:10 +0100, Pavel Begunkov wrote:
> ./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.
> 
> v2: without applied patches, merge previous 6/6 into the second patch
>     get rid of helpers (Jens, Christoph)
>     kill bdev_inode and move inode into bdev (Christoph)
> 
> [...]

Applied, thanks!

[1/3] block: cache bdev in struct file for raw bdev IO
      commit: 82f652990a4ade2df9d6475ad82f4dd22a3265f6
[3/3] blk-mq: optimise *end_request non-stat path
      commit: e945e2edb67048c42527089cd9938454ae4f9c04

Best regards,
-- 
Jens Axboe