2021-10-13 09:00:53

by Pavel Begunkov

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

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 <[email protected]>
---
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


2021-10-13 14:08:00

by kernel test robot

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

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 <[email protected]>

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/[email protected]


Attachments:
(No filename) (3.71 kB)
.config.gz (9.48 kB)
Download all attachments

2021-10-13 15:06:39

by kernel test robot

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

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 <[email protected]>

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/[email protected]


Attachments:
(No filename) (3.90 kB)
.config.gz (28.01 kB)
Download all attachments

2021-10-13 15:15:46

by Pavel Begunkov

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

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 <[email protected]>
>
> 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/[email protected]
>

--
Pavel Begunkov

2021-10-13 15:32:04

by Christoph Hellwig

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

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.

2021-10-13 18:50:26

by Pavel Begunkov

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

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

2021-10-14 04:57:25

by Christoph Hellwig

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

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.

2021-10-14 09:40:25

by Pavel Begunkov

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

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

2021-10-15 07:48:19

by Chen, Rong A

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



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 <[email protected]>
>>
>> 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/[email protected]
>>
>