[PATCHSET 0/2] Optimize block_device utilization

[PATCHSET 0/2] Optimize block_device utilization

[Jens Axboe]

Hi,

First is just a prep patch that moves struct device, which is quite the
pig, out of line in block_device, and then shuffles a few fields for
better layout. It also saves 16 bytes of space in there while at it.

Second patch caches the bdev->disk->fops->submit_bio state in the bdev,
so we can avoid this long dependent memory load chain in the fast
path.

-- 
Jens Axboe

[PATCH 1/2] block: re-arrange the struct block_device fields for better layout

[Jens Axboe]

This moves struct device out-of-line as it's just used at open/close
time, so we can keep some of the commonly used fields closer together.
On a standard setup, it also reduces the size from 864 bytes to 848
bytes. Yes, struct device is a pig...

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/blk_types.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 99be590f952f..d68d6e951fad 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -40,26 +40,25 @@ struct bio_crypt_ctx;
 struct block_device {
 	sector_t		bd_start_sect;
 	sector_t		bd_nr_sectors;
+	struct gendisk *	bd_disk;
+	struct request_queue *	bd_queue;
 	struct disk_stats __percpu *bd_stats;
 	unsigned long		bd_stamp;
 	bool			bd_read_only;	/* read-only policy */
+	u8			bd_partno;
+	bool			bd_write_holder;
 	dev_t			bd_dev;
 	atomic_t		bd_openers;
+	spinlock_t		bd_size_lock; /* for bd_inode->i_size updates */
 	struct inode *		bd_inode;	/* will die */
 	struct super_block *	bd_super;
 	void *			bd_claiming;
-	struct device		bd_device;
 	void *			bd_holder;
+	/* The counter of freeze processes */
+	int			bd_fsfreeze_count;
 	int			bd_holders;
-	bool			bd_write_holder;
 	struct kobject		*bd_holder_dir;
-	u8			bd_partno;
-	spinlock_t		bd_size_lock; /* for bd_inode->i_size updates */
-	struct gendisk *	bd_disk;
-	struct request_queue *	bd_queue;
 
-	/* The counter of freeze processes */
-	int			bd_fsfreeze_count;
 	/* Mutex for freeze */
 	struct mutex		bd_fsfreeze_mutex;
 	struct super_block	*bd_fsfreeze_sb;
@@ -68,6 +67,7 @@ struct block_device {
 #ifdef CONFIG_FAIL_MAKE_REQUEST
 	bool			bd_make_it_fail;
 #endif
+	struct device		bd_device;
 } __randomize_layout;
 
 #define bdev_whole(_bdev) \
-- 
2.39.2

[PATCH 2/2] block: store bdev->bd_disk->fops->submit_bio state in bdev

[Jens Axboe]

We have a long chain of memory dereferencing just to whether or not
this disk has a special submit_bio helper. As that's not necessarily
the common case, add a bd_submit_bio state in the bdev to avoid
traversing this memory dependency chain if we don't need to.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/bdev.c              | 1 +
 block/blk-core.c          | 8 ++++----
 block/genhd.c             | 4 ++++
 include/linux/blk_types.h | 1 +
 4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index 1795c7d4b99e..31a5d25b2b44 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -419,6 +419,7 @@ struct block_device *bdev_alloc(struct gendisk *disk, u8 partno)
 	bdev->bd_inode = inode;
 	bdev->bd_queue = disk->queue;
 	bdev->bd_stats = alloc_percpu(struct disk_stats);
+	bdev->bd_submit_bio = 0;
 	if (!bdev->bd_stats) {
 		iput(inode);
 		return NULL;
diff --git a/block/blk-core.c b/block/blk-core.c
index 269765d16cfd..ae7953539dc0 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -587,14 +587,14 @@ static inline blk_status_t blk_check_zone_append(struct request_queue *q,
 
 static void __submit_bio(struct bio *bio)
 {
-	struct gendisk *disk = bio->bi_bdev->bd_disk;
-
 	if (unlikely(!blk_crypto_bio_prep(&bio)))
 		return;
 
-	if (!disk->fops->submit_bio) {
+	if (!bio->bi_bdev->bd_submit_bio) {
 		blk_mq_submit_bio(bio);
 	} else if (likely(bio_queue_enter(bio) == 0)) {
+		struct gendisk *disk = bio->bi_bdev->bd_disk;
+
 		disk->fops->submit_bio(bio);
 		blk_queue_exit(disk->queue);
 	}
@@ -698,7 +698,7 @@ void submit_bio_noacct_nocheck(struct bio *bio)
 	 */
 	if (current->bio_list)
 		bio_list_add(&current->bio_list[0], bio);
-	else if (!bio->bi_bdev->bd_disk->fops->submit_bio)
+	else if (!bio->bi_bdev->bd_submit_bio)
 		__submit_bio_noacct_mq(bio);
 	else
 		__submit_bio_noacct(bio);
diff --git a/block/genhd.c b/block/genhd.c
index 02d9cfb9e077..07736c5db988 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -420,6 +420,10 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
 	 */
 	elevator_init_mq(disk->queue);
 
+	/* Mark bdev as having a submit_bio, if needed */
+	if (disk->fops->submit_bio)
+		disk->part0->bd_submit_bio = 1;
+
 	/*
 	 * If the driver provides an explicit major number it also must provide
 	 * the number of minors numbers supported, and those will be used to
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index d68d6e951fad..c08e1c08b7ba 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -47,6 +47,7 @@ struct block_device {
 	bool			bd_read_only;	/* read-only policy */
 	u8			bd_partno;
 	bool			bd_write_holder;
+	bool			bd_submit_bio;
 	dev_t			bd_dev;
 	atomic_t		bd_openers;
 	spinlock_t		bd_size_lock; /* for bd_inode->i_size updates */
-- 
2.39.2

Re: [PATCH 2/2] block: store bdev->bd_disk->fops->submit_bio state in bdev

[Damien Le Moal]

On 4/14/23 22:48, Jens Axboe wrote:
> We have a long chain of memory dereferencing just to whether or not
> this disk has a special submit_bio helper. As that's not necessarily
> the common case, add a bd_submit_bio state in the bdev to avoid
> traversing this memory dependency chain if we don't need to.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  block/bdev.c              | 1 +
>  block/blk-core.c          | 8 ++++----
>  block/genhd.c             | 4 ++++
>  include/linux/blk_types.h | 1 +
>  4 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/block/bdev.c b/block/bdev.c
> index 1795c7d4b99e..31a5d25b2b44 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -419,6 +419,7 @@ struct block_device *bdev_alloc(struct gendisk *disk, u8 partno)
>  	bdev->bd_inode = inode;
>  	bdev->bd_queue = disk->queue;
>  	bdev->bd_stats = alloc_percpu(struct disk_stats);
> +	bdev->bd_submit_bio = 0;

"= false;" would be better to match bd_submit_bio type.

[...]

> diff --git a/block/genhd.c b/block/genhd.c
> index 02d9cfb9e077..07736c5db988 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -420,6 +420,10 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
>  	 */
>  	elevator_init_mq(disk->queue);
>  
> +	/* Mark bdev as having a submit_bio, if needed */
> +	if (disk->fops->submit_bio)
> +		disk->part0->bd_submit_bio = 1;

"= true;" would be better to match the type.

Note that this could also be:

disk->part0->bd_submit_bio = disk->fops->submit_bio;

thus removing the if.

Re: [PATCH 2/2] block: store bdev->bd_disk->fops->submit_bio state in bdev

[Jens Axboe]

On 4/14/23 8:43?PM, Damien Le Moal wrote:
> On 4/14/23 22:48, Jens Axboe wrote:
>> We have a long chain of memory dereferencing just to whether or not
>> this disk has a special submit_bio helper. As that's not necessarily
>> the common case, add a bd_submit_bio state in the bdev to avoid
>> traversing this memory dependency chain if we don't need to.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  block/bdev.c              | 1 +
>>  block/blk-core.c          | 8 ++++----
>>  block/genhd.c             | 4 ++++
>>  include/linux/blk_types.h | 1 +
>>  4 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/bdev.c b/block/bdev.c
>> index 1795c7d4b99e..31a5d25b2b44 100644
>> --- a/block/bdev.c
>> +++ b/block/bdev.c
>> @@ -419,6 +419,7 @@ struct block_device *bdev_alloc(struct gendisk *disk, u8 partno)
>>  	bdev->bd_inode = inode;
>>  	bdev->bd_queue = disk->queue;
>>  	bdev->bd_stats = alloc_percpu(struct disk_stats);
>> +	bdev->bd_submit_bio = 0;
> 
> "= false;" would be better to match bd_submit_bio type.

Done

>> diff --git a/block/genhd.c b/block/genhd.c
>> index 02d9cfb9e077..07736c5db988 100644
>> --- a/block/genhd.c
>> +++ b/block/genhd.c
>> @@ -420,6 +420,10 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
>>  	 */
>>  	elevator_init_mq(disk->queue);
>>  
>> +	/* Mark bdev as having a submit_bio, if needed */
>> +	if (disk->fops->submit_bio)
>> +		disk->part0->bd_submit_bio = 1;
> 
> "= true;" would be better to match the type.
> 
> Note that this could also be:
> 
> disk->part0->bd_submit_bio = disk->fops->submit_bio;
> 
> thus removing the if.

I made it:

disk->part0->bd_submit_bio = disk->fops->submit_bio != NULL;

instead to make it explicit, I don't think that assignment would be
happy otherwise.

-- 
Jens Axboe

Re: [PATCH 1/2] block: re-arrange the struct block_device fields for better layout

[Christoph Hellwig]

On Fri, Apr 14, 2023 at 07:48:47AM -0600, Jens Axboe wrote:
> This moves struct device out-of-line as it's just used at open/close
> time, so we can keep some of the commonly used fields closer together.
> On a standard setup, it also reduces the size from 864 bytes to 848
> bytes. Yes, struct device is a pig...

Maybe add a comment about keeping struct device last and why?

Otherwise looks good:

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

Re: [PATCH 2/2] block: store bdev->bd_disk->fops->submit_bio state in bdev

[Christoph Hellwig]

On Fri, Apr 14, 2023 at 07:48:48AM -0600, Jens Axboe wrote:
> We have a long chain of memory dereferencing just to whether or not
> this disk has a special submit_bio helper. As that's not necessarily
> the common case, add a bd_submit_bio state in the bdev to avoid
> traversing this memory dependency chain if we don't need to.

Do you have any numbers on how this helps?

> +	bdev->bd_submit_bio = 0;

bd_submit_bio sounds like a function call, so I'd name this
bd_has_submit_io.

But maybe it might make more sense to just add a bit that this is
a blk-mq backed device into bd_state as that might be handy in other
places as well?

Re: [PATCH 2/2] block: store bdev->bd_disk->fops->submit_bio state in bdev

[Jens Axboe]

On 4/15/23 11:53 PM, Christoph Hellwig wrote:
> On Fri, Apr 14, 2023 at 07:48:48AM -0600, Jens Axboe wrote:
>> We have a long chain of memory dereferencing just to whether or not
>> this disk has a special submit_bio helper. As that's not necessarily
>> the common case, add a bd_submit_bio state in the bdev to avoid
>> traversing this memory dependency chain if we don't need to.
> 
> Do you have any numbers on how this helps?

I didn't run any numbers, but seems obvious to me that we don't want
to pull in 3 layers deep of pointer indirections when we can avoid
it.

>> +	bdev->bd_submit_bio = 0;
> 
> bd_submit_bio sounds like a function call, so I'd name this
> bd_has_submit_io.

Good point, I'll rename it.

> But maybe it might make more sense to just add a bit that this is
> a blk-mq backed device into bd_state as that might be handy in other
> places as well?

I'd rather just do that if needed.

-- 
Jens Axboe

Re: [PATCH 1/2] block: re-arrange the struct block_device fields for better layout

[Jens Axboe]

On 4/15/23 11:51 PM, Christoph Hellwig wrote:
> On Fri, Apr 14, 2023 at 07:48:47AM -0600, Jens Axboe wrote:
>> This moves struct device out-of-line as it's just used at open/close
>> time, so we can keep some of the commonly used fields closer together.
>> On a standard setup, it also reduces the size from 864 bytes to 848
>> bytes. Yes, struct device is a pig...
> 
> Maybe add a comment about keeping struct device last and why?

Sure, done.

-- 
Jens Axboe