From: Yu Kuai <[email protected]>
Yu Kuai (2):
block: introduce new field flags in block_device
block: warn once for each partition in bio_check_ro()
block/bdev.c | 15 ++++++++-------
block/blk-core.c | 21 +++++++++++++++------
block/genhd.c | 8 +++++---
block/ioctl.c | 2 +-
include/linux/blk_types.h | 13 +++++++------
include/linux/blkdev.h | 5 +++--
6 files changed, 39 insertions(+), 25 deletions(-)
--
2.39.2
From: Yu Kuai <[email protected]>
There are multiple switches in struct block_device, use seperate bool
fields for them is not gracefully. Add a new field flags and replace
swithes to a bit, there are no functional changes, and preapre to add
a new switch in the next patch.
Signed-off-by: Yu Kuai <[email protected]>
---
block/bdev.c | 15 ++++++++-------
block/blk-core.c | 7 ++++---
block/genhd.c | 8 +++++---
block/ioctl.c | 2 +-
include/linux/blk_types.h | 12 ++++++------
include/linux/blkdev.h | 5 +++--
6 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/block/bdev.c b/block/bdev.c
index fc8d28d77495..cb849bcf61ae 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -408,10 +408,10 @@ struct block_device *bdev_alloc(struct gendisk *disk, u8 partno)
bdev->bd_partno = partno;
bdev->bd_inode = inode;
bdev->bd_queue = disk->queue;
- if (partno)
- bdev->bd_has_submit_bio = disk->part0->bd_has_submit_bio;
+ if (partno && test_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags))
+ set_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
else
- bdev->bd_has_submit_bio = false;
+ clear_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
bdev->bd_stats = alloc_percpu(struct disk_stats);
if (!bdev->bd_stats) {
iput(inode);
@@ -612,7 +612,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
bdev->bd_holder = NULL;
bdev->bd_holder_ops = NULL;
mutex_unlock(&bdev->bd_holder_lock);
- if (bdev->bd_write_holder)
+ if (test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags))
unblock = true;
}
if (!whole->bd_holders)
@@ -625,7 +625,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
*/
if (unblock) {
disk_unblock_events(bdev->bd_disk);
- bdev->bd_write_holder = false;
+ clear_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
}
}
@@ -878,9 +878,10 @@ struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
* writeable reference is too fragile given the way @mode is
* used in blkdev_get/put().
*/
- if ((mode & BLK_OPEN_WRITE) && !bdev->bd_write_holder &&
+ if ((mode & BLK_OPEN_WRITE) &&
+ !test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags) &&
(disk->event_flags & DISK_EVENT_FLAG_BLOCK_ON_EXCL_WRITE)) {
- bdev->bd_write_holder = true;
+ set_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
unblock_events = false;
}
}
diff --git a/block/blk-core.c b/block/blk-core.c
index fdf25b8d6e78..577a693165d8 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -482,7 +482,8 @@ __setup("fail_make_request=", setup_fail_make_request);
bool should_fail_request(struct block_device *part, unsigned int bytes)
{
- return part->bd_make_it_fail && should_fail(&fail_make_request, bytes);
+ return test_bit(BD_FLAG_MAKE_IT_FAIL, &part->flags) &&
+ should_fail(&fail_make_request, bytes);
}
static int __init fail_make_request_debugfs(void)
@@ -595,7 +596,7 @@ static void __submit_bio(struct bio *bio)
if (unlikely(!blk_crypto_bio_prep(&bio)))
return;
- if (!bio->bi_bdev->bd_has_submit_bio) {
+ if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags)) {
blk_mq_submit_bio(bio);
} else if (likely(bio_queue_enter(bio) == 0)) {
struct gendisk *disk = bio->bi_bdev->bd_disk;
@@ -703,7 +704,7 @@ void submit_bio_noacct_nocheck(struct bio *bio)
*/
if (current->bio_list)
bio_list_add(¤t->bio_list[0], bio);
- else if (!bio->bi_bdev->bd_has_submit_bio)
+ else if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags))
__submit_bio_noacct_mq(bio);
else
__submit_bio_noacct(bio);
diff --git a/block/genhd.c b/block/genhd.c
index c9d06f72c587..026f4c6d5b7e 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -413,7 +413,8 @@ 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 */
- disk->part0->bd_has_submit_bio = disk->fops->submit_bio != NULL;
+ if (disk->fops->submit_bio)
+ set_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags);
/*
* If the driver provides an explicit major number it also must provide
@@ -1062,7 +1063,8 @@ static DEVICE_ATTR(diskseq, 0444, diskseq_show, NULL);
ssize_t part_fail_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- return sprintf(buf, "%d\n", dev_to_bdev(dev)->bd_make_it_fail);
+ return sprintf(buf, "%d\n",
+ test_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags));
}
ssize_t part_fail_store(struct device *dev,
@@ -1072,7 +1074,7 @@ ssize_t part_fail_store(struct device *dev,
int i;
if (count > 0 && sscanf(buf, "%d", &i) > 0)
- dev_to_bdev(dev)->bd_make_it_fail = i;
+ set_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags);
return count;
}
diff --git a/block/ioctl.c b/block/ioctl.c
index 4160f4e6bd5b..a548c718a5fb 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -394,7 +394,7 @@ static int blkdev_roset(struct block_device *bdev, unsigned cmd,
if (ret)
return ret;
}
- bdev->bd_read_only = n;
+ set_bit(BD_FLAG_READ_ONLY, &bdev->flags);
return 0;
}
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 52e264d5a830..95bd26c62655 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -37,17 +37,20 @@ struct bio_crypt_ctx;
#define PAGE_SECTORS (1 << PAGE_SECTORS_SHIFT)
#define SECTOR_MASK (PAGE_SECTORS - 1)
+#define BD_FLAG_READ_ONLY 0 /* read-only-policy */
+#define BD_FLAG_WRITE_HOLDER 1
+#define BD_FLAG_HAS_SUBMIT_BIO 2
+#define BD_FLAG_MAKE_IT_FAIL 3
+
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 flags;
unsigned long bd_stamp;
- bool bd_read_only; /* read-only policy */
u8 bd_partno;
- bool bd_write_holder;
- bool bd_has_submit_bio;
dev_t bd_dev;
atomic_t bd_openers;
spinlock_t bd_size_lock; /* for bd_inode->i_size updates */
@@ -63,9 +66,6 @@ struct block_device {
struct mutex bd_fsfreeze_mutex; /* serialize freeze/thaw */
struct partition_meta_info *bd_meta_info;
-#ifdef CONFIG_FAIL_MAKE_REQUEST
- bool bd_make_it_fail;
-#endif
int bd_writers;
/*
* keep this out-of-line as it's both big and not needed in the fast
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 9f6c3373f9fc..ec8382ecfcf1 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -743,13 +743,14 @@ void disk_uevent(struct gendisk *disk, enum kobject_action action);
static inline int get_disk_ro(struct gendisk *disk)
{
- return disk->part0->bd_read_only ||
+ return test_bit(BD_FLAG_READ_ONLY, &disk->part0->flags) ||
test_bit(GD_READ_ONLY, &disk->state);
}
static inline int bdev_read_only(struct block_device *bdev)
{
- return bdev->bd_read_only || get_disk_ro(bdev->bd_disk);
+ return test_bit(BD_FLAG_READ_ONLY, &bdev->flags) ||
+ get_disk_ro(bdev->bd_disk);
}
bool set_capacity_and_notify(struct gendisk *disk, sector_t size);
--
2.39.2
From: Yu Kuai <[email protected]>
Commit 1b0a151c10a6 ("blk-core: use pr_warn_ratelimited() in
bio_check_ro()") fix message storm by limit the rate, however, there
will still be lots of message in the long term. Fix it better by warn
once for each partition.
Signed-off-by: Yu Kuai <[email protected]>
---
block/blk-core.c | 14 +++++++++++---
include/linux/blk_types.h | 1 +
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 577a693165d8..ccfc80d0302d 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -502,9 +502,17 @@ static inline void bio_check_ro(struct bio *bio)
if (op_is_write(bio_op(bio)) && bdev_read_only(bio->bi_bdev)) {
if (op_is_flush(bio->bi_opf) && !bio_sectors(bio))
return;
- pr_warn_ratelimited("Trying to write to read-only block-device %pg\n",
- bio->bi_bdev);
- /* Older lvm-tools actually trigger this */
+
+ if (test_bit(BD_FLAG_RO_WARNED, &bio->bi_bdev->flags) ||
+ test_and_set_bit(BD_FLAG_RO_WARNED, &bio->bi_bdev->flags))
+ return;
+
+ /*
+ * Use ioctl to set underlying disk of raid/dm to read-only
+ * will trigger this.
+ */
+ pr_warn("Trying to write to read-only block-device %pg\n",
+ bio->bi_bdev);
}
}
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 95bd26c62655..99d32c841c0d 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -41,6 +41,7 @@ struct bio_crypt_ctx;
#define BD_FLAG_WRITE_HOLDER 1
#define BD_FLAG_HAS_SUBMIT_BIO 2
#define BD_FLAG_MAKE_IT_FAIL 3
+#define BD_FLAG_RO_WARNED 4
struct block_device {
sector_t bd_start_sect;
--
2.39.2
On Mon, Nov 20, 2023 at 05:38:46PM +0800, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> There are multiple switches in struct block_device, use seperate bool
> fields for them is not gracefully. Add a new field flags and replace
> swithes to a bit, there are no functional changes, and preapre to add
> a new switch in the next patch.
>
> Signed-off-by: Yu Kuai <[email protected]>
> ---
> block/bdev.c | 15 ++++++++-------
> block/blk-core.c | 7 ++++---
> block/genhd.c | 8 +++++---
> block/ioctl.c | 2 +-
> include/linux/blk_types.h | 12 ++++++------
> include/linux/blkdev.h | 5 +++--
> 6 files changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/block/bdev.c b/block/bdev.c
> index fc8d28d77495..cb849bcf61ae 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -408,10 +408,10 @@ struct block_device *bdev_alloc(struct gendisk *disk, u8 partno)
> bdev->bd_partno = partno;
> bdev->bd_inode = inode;
> bdev->bd_queue = disk->queue;
> - if (partno)
> - bdev->bd_has_submit_bio = disk->part0->bd_has_submit_bio;
> + if (partno && test_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags))
> + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
> else
> - bdev->bd_has_submit_bio = false;
> + clear_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
> bdev->bd_stats = alloc_percpu(struct disk_stats);
> if (!bdev->bd_stats) {
> iput(inode);
> @@ -612,7 +612,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
> bdev->bd_holder = NULL;
> bdev->bd_holder_ops = NULL;
> mutex_unlock(&bdev->bd_holder_lock);
> - if (bdev->bd_write_holder)
> + if (test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags))
> unblock = true;
> }
> if (!whole->bd_holders)
> @@ -625,7 +625,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
> */
> if (unblock) {
> disk_unblock_events(bdev->bd_disk);
> - bdev->bd_write_holder = false;
> + clear_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
> }
> }
>
> @@ -878,9 +878,10 @@ struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
> * writeable reference is too fragile given the way @mode is
> * used in blkdev_get/put().
> */
> - if ((mode & BLK_OPEN_WRITE) && !bdev->bd_write_holder &&
> + if ((mode & BLK_OPEN_WRITE) &&
> + !test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags) &&
> (disk->event_flags & DISK_EVENT_FLAG_BLOCK_ON_EXCL_WRITE)) {
> - bdev->bd_write_holder = true;
> + set_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
> unblock_events = false;
> }
> }
> diff --git a/block/blk-core.c b/block/blk-core.c
> index fdf25b8d6e78..577a693165d8 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -482,7 +482,8 @@ __setup("fail_make_request=", setup_fail_make_request);
>
> bool should_fail_request(struct block_device *part, unsigned int bytes)
> {
> - return part->bd_make_it_fail && should_fail(&fail_make_request, bytes);
> + return test_bit(BD_FLAG_MAKE_IT_FAIL, &part->flags) &&
> + should_fail(&fail_make_request, bytes);
> }
>
> static int __init fail_make_request_debugfs(void)
> @@ -595,7 +596,7 @@ static void __submit_bio(struct bio *bio)
> if (unlikely(!blk_crypto_bio_prep(&bio)))
> return;
>
> - if (!bio->bi_bdev->bd_has_submit_bio) {
> + if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags)) {
> blk_mq_submit_bio(bio);
> } else if (likely(bio_queue_enter(bio) == 0)) {
> struct gendisk *disk = bio->bi_bdev->bd_disk;
> @@ -703,7 +704,7 @@ void submit_bio_noacct_nocheck(struct bio *bio)
> */
> if (current->bio_list)
> bio_list_add(¤t->bio_list[0], bio);
> - else if (!bio->bi_bdev->bd_has_submit_bio)
> + else if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags))
> __submit_bio_noacct_mq(bio);
> else
> __submit_bio_noacct(bio);
> diff --git a/block/genhd.c b/block/genhd.c
> index c9d06f72c587..026f4c6d5b7e 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -413,7 +413,8 @@ 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 */
> - disk->part0->bd_has_submit_bio = disk->fops->submit_bio != NULL;
> + if (disk->fops->submit_bio)
> + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags);
>
> /*
> * If the driver provides an explicit major number it also must provide
> @@ -1062,7 +1063,8 @@ static DEVICE_ATTR(diskseq, 0444, diskseq_show, NULL);
> ssize_t part_fail_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - return sprintf(buf, "%d\n", dev_to_bdev(dev)->bd_make_it_fail);
> + return sprintf(buf, "%d\n",
> + test_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags));
> }
>
> ssize_t part_fail_store(struct device *dev,
> @@ -1072,7 +1074,7 @@ ssize_t part_fail_store(struct device *dev,
> int i;
>
> if (count > 0 && sscanf(buf, "%d", &i) > 0)
> - dev_to_bdev(dev)->bd_make_it_fail = i;
> + set_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags);
>
> return count;
> }
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 4160f4e6bd5b..a548c718a5fb 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -394,7 +394,7 @@ static int blkdev_roset(struct block_device *bdev, unsigned cmd,
> if (ret)
> return ret;
> }
> - bdev->bd_read_only = n;
> + set_bit(BD_FLAG_READ_ONLY, &bdev->flags);
> return 0;
> }
>
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 52e264d5a830..95bd26c62655 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -37,17 +37,20 @@ struct bio_crypt_ctx;
> #define PAGE_SECTORS (1 << PAGE_SECTORS_SHIFT)
> #define SECTOR_MASK (PAGE_SECTORS - 1)
>
> +#define BD_FLAG_READ_ONLY 0 /* read-only-policy */
> +#define BD_FLAG_WRITE_HOLDER 1
> +#define BD_FLAG_HAS_SUBMIT_BIO 2
> +#define BD_FLAG_MAKE_IT_FAIL 3
> +
> 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 flags;
> unsigned long bd_stamp;
> - bool bd_read_only; /* read-only policy */
> u8 bd_partno;
> - bool bd_write_holder;
> - bool bd_has_submit_bio;
The idea looks good, but not necessary to add new 8 bytes, and you may
put all these flags and `bd_partno` into single 'unsigned long', and add
helpers to retrieve part no, since there are not many ->bd_partno
references.
Thanks,
Ming
Hi,
?? 2023/11/20 11:00, Ming Lei д??:
> On Mon, Nov 20, 2023 at 05:38:46PM +0800, Yu Kuai wrote:
>> From: Yu Kuai <[email protected]>
>>
>> There are multiple switches in struct block_device, use seperate bool
>> fields for them is not gracefully. Add a new field flags and replace
>> swithes to a bit, there are no functional changes, and preapre to add
>> a new switch in the next patch.
>>
>> Signed-off-by: Yu Kuai <[email protected]>
>> ---
>> block/bdev.c | 15 ++++++++-------
>> block/blk-core.c | 7 ++++---
>> block/genhd.c | 8 +++++---
>> block/ioctl.c | 2 +-
>> include/linux/blk_types.h | 12 ++++++------
>> include/linux/blkdev.h | 5 +++--
>> 6 files changed, 27 insertions(+), 22 deletions(-)
>>
>> diff --git a/block/bdev.c b/block/bdev.c
>> index fc8d28d77495..cb849bcf61ae 100644
>> --- a/block/bdev.c
>> +++ b/block/bdev.c
>> @@ -408,10 +408,10 @@ struct block_device *bdev_alloc(struct gendisk *disk, u8 partno)
>> bdev->bd_partno = partno;
>> bdev->bd_inode = inode;
>> bdev->bd_queue = disk->queue;
>> - if (partno)
>> - bdev->bd_has_submit_bio = disk->part0->bd_has_submit_bio;
>> + if (partno && test_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags))
>> + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
>> else
>> - bdev->bd_has_submit_bio = false;
>> + clear_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
>> bdev->bd_stats = alloc_percpu(struct disk_stats);
>> if (!bdev->bd_stats) {
>> iput(inode);
>> @@ -612,7 +612,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
>> bdev->bd_holder = NULL;
>> bdev->bd_holder_ops = NULL;
>> mutex_unlock(&bdev->bd_holder_lock);
>> - if (bdev->bd_write_holder)
>> + if (test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags))
>> unblock = true;
>> }
>> if (!whole->bd_holders)
>> @@ -625,7 +625,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
>> */
>> if (unblock) {
>> disk_unblock_events(bdev->bd_disk);
>> - bdev->bd_write_holder = false;
>> + clear_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
>> }
>> }
>>
>> @@ -878,9 +878,10 @@ struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
>> * writeable reference is too fragile given the way @mode is
>> * used in blkdev_get/put().
>> */
>> - if ((mode & BLK_OPEN_WRITE) && !bdev->bd_write_holder &&
>> + if ((mode & BLK_OPEN_WRITE) &&
>> + !test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags) &&
>> (disk->event_flags & DISK_EVENT_FLAG_BLOCK_ON_EXCL_WRITE)) {
>> - bdev->bd_write_holder = true;
>> + set_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
>> unblock_events = false;
>> }
>> }
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index fdf25b8d6e78..577a693165d8 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -482,7 +482,8 @@ __setup("fail_make_request=", setup_fail_make_request);
>>
>> bool should_fail_request(struct block_device *part, unsigned int bytes)
>> {
>> - return part->bd_make_it_fail && should_fail(&fail_make_request, bytes);
>> + return test_bit(BD_FLAG_MAKE_IT_FAIL, &part->flags) &&
>> + should_fail(&fail_make_request, bytes);
>> }
>>
>> static int __init fail_make_request_debugfs(void)
>> @@ -595,7 +596,7 @@ static void __submit_bio(struct bio *bio)
>> if (unlikely(!blk_crypto_bio_prep(&bio)))
>> return;
>>
>> - if (!bio->bi_bdev->bd_has_submit_bio) {
>> + if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags)) {
>> blk_mq_submit_bio(bio);
>> } else if (likely(bio_queue_enter(bio) == 0)) {
>> struct gendisk *disk = bio->bi_bdev->bd_disk;
>> @@ -703,7 +704,7 @@ void submit_bio_noacct_nocheck(struct bio *bio)
>> */
>> if (current->bio_list)
>> bio_list_add(¤t->bio_list[0], bio);
>> - else if (!bio->bi_bdev->bd_has_submit_bio)
>> + else if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags))
>> __submit_bio_noacct_mq(bio);
>> else
>> __submit_bio_noacct(bio);
>> diff --git a/block/genhd.c b/block/genhd.c
>> index c9d06f72c587..026f4c6d5b7e 100644
>> --- a/block/genhd.c
>> +++ b/block/genhd.c
>> @@ -413,7 +413,8 @@ 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 */
>> - disk->part0->bd_has_submit_bio = disk->fops->submit_bio != NULL;
>> + if (disk->fops->submit_bio)
>> + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags);
>>
>> /*
>> * If the driver provides an explicit major number it also must provide
>> @@ -1062,7 +1063,8 @@ static DEVICE_ATTR(diskseq, 0444, diskseq_show, NULL);
>> ssize_t part_fail_show(struct device *dev,
>> struct device_attribute *attr, char *buf)
>> {
>> - return sprintf(buf, "%d\n", dev_to_bdev(dev)->bd_make_it_fail);
>> + return sprintf(buf, "%d\n",
>> + test_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags));
>> }
>>
>> ssize_t part_fail_store(struct device *dev,
>> @@ -1072,7 +1074,7 @@ ssize_t part_fail_store(struct device *dev,
>> int i;
>>
>> if (count > 0 && sscanf(buf, "%d", &i) > 0)
>> - dev_to_bdev(dev)->bd_make_it_fail = i;
>> + set_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags);
>>
>> return count;
>> }
>> diff --git a/block/ioctl.c b/block/ioctl.c
>> index 4160f4e6bd5b..a548c718a5fb 100644
>> --- a/block/ioctl.c
>> +++ b/block/ioctl.c
>> @@ -394,7 +394,7 @@ static int blkdev_roset(struct block_device *bdev, unsigned cmd,
>> if (ret)
>> return ret;
>> }
>> - bdev->bd_read_only = n;
>> + set_bit(BD_FLAG_READ_ONLY, &bdev->flags);
>> return 0;
>> }
>>
>> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
>> index 52e264d5a830..95bd26c62655 100644
>> --- a/include/linux/blk_types.h
>> +++ b/include/linux/blk_types.h
>> @@ -37,17 +37,20 @@ struct bio_crypt_ctx;
>> #define PAGE_SECTORS (1 << PAGE_SECTORS_SHIFT)
>> #define SECTOR_MASK (PAGE_SECTORS - 1)
>>
>> +#define BD_FLAG_READ_ONLY 0 /* read-only-policy */
>> +#define BD_FLAG_WRITE_HOLDER 1
>> +#define BD_FLAG_HAS_SUBMIT_BIO 2
>> +#define BD_FLAG_MAKE_IT_FAIL 3
>> +
>> 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 flags;
>> unsigned long bd_stamp;
>> - bool bd_read_only; /* read-only policy */
>> u8 bd_partno;
>> - bool bd_write_holder;
>> - bool bd_has_submit_bio;
>
> The idea looks good, but not necessary to add new 8 bytes, and you may
> put all these flags and `bd_partno` into single 'unsigned long', and add
> helpers to retrieve part no, since there are not many ->bd_partno
> references.
Yes, it make sense.
By the way, take a look at component of block_device, I think they can
be reorganized to save some space, in this case, new 8 bytes won't make
this struct bigger, in fact, total size will be shrinked by 8 types
after following changes:
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 95bd26c62655..c94242a9ad92 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -50,7 +50,6 @@ struct block_device {
struct disk_stats __percpu *bd_stats;
unsigned long flags;
unsigned long bd_stamp;
- u8 bd_partno;
dev_t bd_dev;
atomic_t bd_openers;
spinlock_t bd_size_lock; /* for bd_inode->i_size
updates */
@@ -60,13 +59,14 @@ struct block_device {
const struct blk_holder_ops *bd_holder_ops;
struct mutex bd_holder_lock;
int bd_holders;
+ u8 bd_partno;
struct kobject *bd_holder_dir;
- atomic_t bd_fsfreeze_count; /* number of freeze
requests */
struct mutex bd_fsfreeze_mutex; /* serialize
freeze/thaw */
+ atomic_t bd_fsfreeze_count; /* number of freeze
requests */
- struct partition_meta_info *bd_meta_info;
int bd_writers;
+ struct partition_meta_info *bd_meta_info;
/*
* keep this out-of-line as it's both big and not needed in the
fast
* path
Please let me know what you think.
Thanks,
Kuai
>
> Thanks,
> Ming
>
>
> .
>
On Mon, Nov 20, 2023 at 02:37:59PM +0800, Yu Kuai wrote:
> Hi,
>
> 在 2023/11/20 11:00, Ming Lei 写道:
> > On Mon, Nov 20, 2023 at 05:38:46PM +0800, Yu Kuai wrote:
> > > From: Yu Kuai <[email protected]>
> > >
> > > There are multiple switches in struct block_device, use seperate bool
> > > fields for them is not gracefully. Add a new field flags and replace
> > > swithes to a bit, there are no functional changes, and preapre to add
> > > a new switch in the next patch.
> > >
> > > Signed-off-by: Yu Kuai <[email protected]>
> > > ---
> > > block/bdev.c | 15 ++++++++-------
> > > block/blk-core.c | 7 ++++---
> > > block/genhd.c | 8 +++++---
> > > block/ioctl.c | 2 +-
> > > include/linux/blk_types.h | 12 ++++++------
> > > include/linux/blkdev.h | 5 +++--
> > > 6 files changed, 27 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/block/bdev.c b/block/bdev.c
> > > index fc8d28d77495..cb849bcf61ae 100644
> > > --- a/block/bdev.c
> > > +++ b/block/bdev.c
> > > @@ -408,10 +408,10 @@ struct block_device *bdev_alloc(struct gendisk *disk, u8 partno)
> > > bdev->bd_partno = partno;
> > > bdev->bd_inode = inode;
> > > bdev->bd_queue = disk->queue;
> > > - if (partno)
> > > - bdev->bd_has_submit_bio = disk->part0->bd_has_submit_bio;
> > > + if (partno && test_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags))
> > > + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
> > > else
> > > - bdev->bd_has_submit_bio = false;
> > > + clear_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
> > > bdev->bd_stats = alloc_percpu(struct disk_stats);
> > > if (!bdev->bd_stats) {
> > > iput(inode);
> > > @@ -612,7 +612,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
> > > bdev->bd_holder = NULL;
> > > bdev->bd_holder_ops = NULL;
> > > mutex_unlock(&bdev->bd_holder_lock);
> > > - if (bdev->bd_write_holder)
> > > + if (test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags))
> > > unblock = true;
> > > }
> > > if (!whole->bd_holders)
> > > @@ -625,7 +625,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
> > > */
> > > if (unblock) {
> > > disk_unblock_events(bdev->bd_disk);
> > > - bdev->bd_write_holder = false;
> > > + clear_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
> > > }
> > > }
> > > @@ -878,9 +878,10 @@ struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
> > > * writeable reference is too fragile given the way @mode is
> > > * used in blkdev_get/put().
> > > */
> > > - if ((mode & BLK_OPEN_WRITE) && !bdev->bd_write_holder &&
> > > + if ((mode & BLK_OPEN_WRITE) &&
> > > + !test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags) &&
> > > (disk->event_flags & DISK_EVENT_FLAG_BLOCK_ON_EXCL_WRITE)) {
> > > - bdev->bd_write_holder = true;
> > > + set_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
> > > unblock_events = false;
> > > }
> > > }
> > > diff --git a/block/blk-core.c b/block/blk-core.c
> > > index fdf25b8d6e78..577a693165d8 100644
> > > --- a/block/blk-core.c
> > > +++ b/block/blk-core.c
> > > @@ -482,7 +482,8 @@ __setup("fail_make_request=", setup_fail_make_request);
> > > bool should_fail_request(struct block_device *part, unsigned int bytes)
> > > {
> > > - return part->bd_make_it_fail && should_fail(&fail_make_request, bytes);
> > > + return test_bit(BD_FLAG_MAKE_IT_FAIL, &part->flags) &&
> > > + should_fail(&fail_make_request, bytes);
> > > }
> > > static int __init fail_make_request_debugfs(void)
> > > @@ -595,7 +596,7 @@ static void __submit_bio(struct bio *bio)
> > > if (unlikely(!blk_crypto_bio_prep(&bio)))
> > > return;
> > > - if (!bio->bi_bdev->bd_has_submit_bio) {
> > > + if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags)) {
> > > blk_mq_submit_bio(bio);
> > > } else if (likely(bio_queue_enter(bio) == 0)) {
> > > struct gendisk *disk = bio->bi_bdev->bd_disk;
> > > @@ -703,7 +704,7 @@ void submit_bio_noacct_nocheck(struct bio *bio)
> > > */
> > > if (current->bio_list)
> > > bio_list_add(¤t->bio_list[0], bio);
> > > - else if (!bio->bi_bdev->bd_has_submit_bio)
> > > + else if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags))
> > > __submit_bio_noacct_mq(bio);
> > > else
> > > __submit_bio_noacct(bio);
> > > diff --git a/block/genhd.c b/block/genhd.c
> > > index c9d06f72c587..026f4c6d5b7e 100644
> > > --- a/block/genhd.c
> > > +++ b/block/genhd.c
> > > @@ -413,7 +413,8 @@ 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 */
> > > - disk->part0->bd_has_submit_bio = disk->fops->submit_bio != NULL;
> > > + if (disk->fops->submit_bio)
> > > + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags);
> > > /*
> > > * If the driver provides an explicit major number it also must provide
> > > @@ -1062,7 +1063,8 @@ static DEVICE_ATTR(diskseq, 0444, diskseq_show, NULL);
> > > ssize_t part_fail_show(struct device *dev,
> > > struct device_attribute *attr, char *buf)
> > > {
> > > - return sprintf(buf, "%d\n", dev_to_bdev(dev)->bd_make_it_fail);
> > > + return sprintf(buf, "%d\n",
> > > + test_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags));
> > > }
> > > ssize_t part_fail_store(struct device *dev,
> > > @@ -1072,7 +1074,7 @@ ssize_t part_fail_store(struct device *dev,
> > > int i;
> > > if (count > 0 && sscanf(buf, "%d", &i) > 0)
> > > - dev_to_bdev(dev)->bd_make_it_fail = i;
> > > + set_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags);
> > > return count;
> > > }
> > > diff --git a/block/ioctl.c b/block/ioctl.c
> > > index 4160f4e6bd5b..a548c718a5fb 100644
> > > --- a/block/ioctl.c
> > > +++ b/block/ioctl.c
> > > @@ -394,7 +394,7 @@ static int blkdev_roset(struct block_device *bdev, unsigned cmd,
> > > if (ret)
> > > return ret;
> > > }
> > > - bdev->bd_read_only = n;
> > > + set_bit(BD_FLAG_READ_ONLY, &bdev->flags);
> > > return 0;
> > > }
> > > diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> > > index 52e264d5a830..95bd26c62655 100644
> > > --- a/include/linux/blk_types.h
> > > +++ b/include/linux/blk_types.h
> > > @@ -37,17 +37,20 @@ struct bio_crypt_ctx;
> > > #define PAGE_SECTORS (1 << PAGE_SECTORS_SHIFT)
> > > #define SECTOR_MASK (PAGE_SECTORS - 1)
> > > +#define BD_FLAG_READ_ONLY 0 /* read-only-policy */
> > > +#define BD_FLAG_WRITE_HOLDER 1
> > > +#define BD_FLAG_HAS_SUBMIT_BIO 2
> > > +#define BD_FLAG_MAKE_IT_FAIL 3
> > > +
> > > 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 flags;
> > > unsigned long bd_stamp;
> > > - bool bd_read_only; /* read-only policy */
> > > u8 bd_partno;
> > > - bool bd_write_holder;
> > > - bool bd_has_submit_bio;
> >
> > The idea looks good, but not necessary to add new 8 bytes, and you may
> > put all these flags and `bd_partno` into single 'unsigned long', and add
> > helpers to retrieve part no, since there are not many ->bd_partno
> > references.
>
> Yes, it make sense.
>
> By the way, take a look at component of block_device, I think they can
> be reorganized to save some space, in this case, new 8 bytes won't make
> this struct bigger, in fact, total size will be shrinked by 8 types
> after following changes:
>
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 95bd26c62655..c94242a9ad92 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -50,7 +50,6 @@ struct block_device {
> struct disk_stats __percpu *bd_stats;
> unsigned long flags;
> unsigned long bd_stamp;
> - u8 bd_partno;
> dev_t bd_dev;
> atomic_t bd_openers;
> spinlock_t bd_size_lock; /* for bd_inode->i_size
> updates */
> @@ -60,13 +59,14 @@ struct block_device {
> const struct blk_holder_ops *bd_holder_ops;
> struct mutex bd_holder_lock;
> int bd_holders;
> + u8 bd_partno;
'bd_partno' needs to be kept in the 1st cacheline, so not good to move
it.
Thanks,
Ming
Hi,
在 2023/11/20 14:58, Ming Lei 写道:
> On Mon, Nov 20, 2023 at 02:37:59PM +0800, Yu Kuai wrote:
>> Hi,
>>
>> 在 2023/11/20 11:00, Ming Lei 写道:
>>> On Mon, Nov 20, 2023 at 05:38:46PM +0800, Yu Kuai wrote:
>>>> From: Yu Kuai <[email protected]>
>>>>
>>>> There are multiple switches in struct block_device, use seperate bool
>>>> fields for them is not gracefully. Add a new field flags and replace
>>>> swithes to a bit, there are no functional changes, and preapre to add
>>>> a new switch in the next patch.
>>>>
>>>> Signed-off-by: Yu Kuai <[email protected]>
>>>> ---
>>>> block/bdev.c | 15 ++++++++-------
>>>> block/blk-core.c | 7 ++++---
>>>> block/genhd.c | 8 +++++---
>>>> block/ioctl.c | 2 +-
>>>> include/linux/blk_types.h | 12 ++++++------
>>>> include/linux/blkdev.h | 5 +++--
>>>> 6 files changed, 27 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/block/bdev.c b/block/bdev.c
>>>> index fc8d28d77495..cb849bcf61ae 100644
>>>> --- a/block/bdev.c
>>>> +++ b/block/bdev.c
>>>> @@ -408,10 +408,10 @@ struct block_device *bdev_alloc(struct gendisk *disk, u8 partno)
>>>> bdev->bd_partno = partno;
>>>> bdev->bd_inode = inode;
>>>> bdev->bd_queue = disk->queue;
>>>> - if (partno)
>>>> - bdev->bd_has_submit_bio = disk->part0->bd_has_submit_bio;
>>>> + if (partno && test_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags))
>>>> + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
>>>> else
>>>> - bdev->bd_has_submit_bio = false;
>>>> + clear_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
>>>> bdev->bd_stats = alloc_percpu(struct disk_stats);
>>>> if (!bdev->bd_stats) {
>>>> iput(inode);
>>>> @@ -612,7 +612,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
>>>> bdev->bd_holder = NULL;
>>>> bdev->bd_holder_ops = NULL;
>>>> mutex_unlock(&bdev->bd_holder_lock);
>>>> - if (bdev->bd_write_holder)
>>>> + if (test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags))
>>>> unblock = true;
>>>> }
>>>> if (!whole->bd_holders)
>>>> @@ -625,7 +625,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
>>>> */
>>>> if (unblock) {
>>>> disk_unblock_events(bdev->bd_disk);
>>>> - bdev->bd_write_holder = false;
>>>> + clear_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
>>>> }
>>>> }
>>>> @@ -878,9 +878,10 @@ struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
>>>> * writeable reference is too fragile given the way @mode is
>>>> * used in blkdev_get/put().
>>>> */
>>>> - if ((mode & BLK_OPEN_WRITE) && !bdev->bd_write_holder &&
>>>> + if ((mode & BLK_OPEN_WRITE) &&
>>>> + !test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags) &&
>>>> (disk->event_flags & DISK_EVENT_FLAG_BLOCK_ON_EXCL_WRITE)) {
>>>> - bdev->bd_write_holder = true;
>>>> + set_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
>>>> unblock_events = false;
>>>> }
>>>> }
>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>> index fdf25b8d6e78..577a693165d8 100644
>>>> --- a/block/blk-core.c
>>>> +++ b/block/blk-core.c
>>>> @@ -482,7 +482,8 @@ __setup("fail_make_request=", setup_fail_make_request);
>>>> bool should_fail_request(struct block_device *part, unsigned int bytes)
>>>> {
>>>> - return part->bd_make_it_fail && should_fail(&fail_make_request, bytes);
>>>> + return test_bit(BD_FLAG_MAKE_IT_FAIL, &part->flags) &&
>>>> + should_fail(&fail_make_request, bytes);
>>>> }
>>>> static int __init fail_make_request_debugfs(void)
>>>> @@ -595,7 +596,7 @@ static void __submit_bio(struct bio *bio)
>>>> if (unlikely(!blk_crypto_bio_prep(&bio)))
>>>> return;
>>>> - if (!bio->bi_bdev->bd_has_submit_bio) {
>>>> + if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags)) {
>>>> blk_mq_submit_bio(bio);
>>>> } else if (likely(bio_queue_enter(bio) == 0)) {
>>>> struct gendisk *disk = bio->bi_bdev->bd_disk;
>>>> @@ -703,7 +704,7 @@ void submit_bio_noacct_nocheck(struct bio *bio)
>>>> */
>>>> if (current->bio_list)
>>>> bio_list_add(¤t->bio_list[0], bio);
>>>> - else if (!bio->bi_bdev->bd_has_submit_bio)
>>>> + else if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags))
>>>> __submit_bio_noacct_mq(bio);
>>>> else
>>>> __submit_bio_noacct(bio);
>>>> diff --git a/block/genhd.c b/block/genhd.c
>>>> index c9d06f72c587..026f4c6d5b7e 100644
>>>> --- a/block/genhd.c
>>>> +++ b/block/genhd.c
>>>> @@ -413,7 +413,8 @@ 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 */
>>>> - disk->part0->bd_has_submit_bio = disk->fops->submit_bio != NULL;
>>>> + if (disk->fops->submit_bio)
>>>> + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags);
>>>> /*
>>>> * If the driver provides an explicit major number it also must provide
>>>> @@ -1062,7 +1063,8 @@ static DEVICE_ATTR(diskseq, 0444, diskseq_show, NULL);
>>>> ssize_t part_fail_show(struct device *dev,
>>>> struct device_attribute *attr, char *buf)
>>>> {
>>>> - return sprintf(buf, "%d\n", dev_to_bdev(dev)->bd_make_it_fail);
>>>> + return sprintf(buf, "%d\n",
>>>> + test_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags));
>>>> }
>>>> ssize_t part_fail_store(struct device *dev,
>>>> @@ -1072,7 +1074,7 @@ ssize_t part_fail_store(struct device *dev,
>>>> int i;
>>>> if (count > 0 && sscanf(buf, "%d", &i) > 0)
>>>> - dev_to_bdev(dev)->bd_make_it_fail = i;
>>>> + set_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags);
>>>> return count;
>>>> }
>>>> diff --git a/block/ioctl.c b/block/ioctl.c
>>>> index 4160f4e6bd5b..a548c718a5fb 100644
>>>> --- a/block/ioctl.c
>>>> +++ b/block/ioctl.c
>>>> @@ -394,7 +394,7 @@ static int blkdev_roset(struct block_device *bdev, unsigned cmd,
>>>> if (ret)
>>>> return ret;
>>>> }
>>>> - bdev->bd_read_only = n;
>>>> + set_bit(BD_FLAG_READ_ONLY, &bdev->flags);
>>>> return 0;
>>>> }
>>>> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
>>>> index 52e264d5a830..95bd26c62655 100644
>>>> --- a/include/linux/blk_types.h
>>>> +++ b/include/linux/blk_types.h
>>>> @@ -37,17 +37,20 @@ struct bio_crypt_ctx;
>>>> #define PAGE_SECTORS (1 << PAGE_SECTORS_SHIFT)
>>>> #define SECTOR_MASK (PAGE_SECTORS - 1)
>>>> +#define BD_FLAG_READ_ONLY 0 /* read-only-policy */
>>>> +#define BD_FLAG_WRITE_HOLDER 1
>>>> +#define BD_FLAG_HAS_SUBMIT_BIO 2
>>>> +#define BD_FLAG_MAKE_IT_FAIL 3
>>>> +
>>>> 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 flags;
>>>> unsigned long bd_stamp;
>>>> - bool bd_read_only; /* read-only policy */
>>>> u8 bd_partno;
>>>> - bool bd_write_holder;
>>>> - bool bd_has_submit_bio;
>>>
>>> The idea looks good, but not necessary to add new 8 bytes, and you may
>>> put all these flags and `bd_partno` into single 'unsigned long', and add
>>> helpers to retrieve part no, since there are not many ->bd_partno
>>> references.
>>
>> Yes, it make sense.
>>
>> By the way, take a look at component of block_device, I think they can
>> be reorganized to save some space, in this case, new 8 bytes won't make
>> this struct bigger, in fact, total size will be shrinked by 8 types
>> after following changes:
>>
>> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
>> index 95bd26c62655..c94242a9ad92 100644
>> --- a/include/linux/blk_types.h
>> +++ b/include/linux/blk_types.h
>> @@ -50,7 +50,6 @@ struct block_device {
>> struct disk_stats __percpu *bd_stats;
>> unsigned long flags;
>> unsigned long bd_stamp;
>> - u8 bd_partno;
>> dev_t bd_dev;
>> atomic_t bd_openers;
>> spinlock_t bd_size_lock; /* for bd_inode->i_size
>> updates */
>> @@ -60,13 +59,14 @@ struct block_device {
>> const struct blk_holder_ops *bd_holder_ops;
>> struct mutex bd_holder_lock;
>> int bd_holders;
>> + u8 bd_partno;
>
> 'bd_partno' needs to be kept in the 1st cacheline, so not good to move
> it.
>
Got it, and same for bd_dev and bd_openers, I'll send a new version
based on your first adivce.
Thanks,
Kuai
>
> Thanks,
> Ming
>
>
> .
>
Hi,
?? 2023/11/20 11:00, Ming Lei д??:
> On Mon, Nov 20, 2023 at 05:38:46PM +0800, Yu Kuai wrote:
>> From: Yu Kuai <[email protected]>
>>
>> There are multiple switches in struct block_device, use seperate bool
>> fields for them is not gracefully. Add a new field flags and replace
>> swithes to a bit, there are no functional changes, and preapre to add
>> a new switch in the next patch.
>>
>> Signed-off-by: Yu Kuai <[email protected]>
>> ---
>> block/bdev.c | 15 ++++++++-------
>> block/blk-core.c | 7 ++++---
>> block/genhd.c | 8 +++++---
>> block/ioctl.c | 2 +-
>> include/linux/blk_types.h | 12 ++++++------
>> include/linux/blkdev.h | 5 +++--
>> 6 files changed, 27 insertions(+), 22 deletions(-)
>>
>> diff --git a/block/bdev.c b/block/bdev.c
>> index fc8d28d77495..cb849bcf61ae 100644
>> --- a/block/bdev.c
>> +++ b/block/bdev.c
>> @@ -408,10 +408,10 @@ struct block_device *bdev_alloc(struct gendisk *disk, u8 partno)
>> bdev->bd_partno = partno;
>> bdev->bd_inode = inode;
>> bdev->bd_queue = disk->queue;
>> - if (partno)
>> - bdev->bd_has_submit_bio = disk->part0->bd_has_submit_bio;
>> + if (partno && test_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags))
>> + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
>> else
>> - bdev->bd_has_submit_bio = false;
>> + clear_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
>> bdev->bd_stats = alloc_percpu(struct disk_stats);
>> if (!bdev->bd_stats) {
>> iput(inode);
>> @@ -612,7 +612,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
>> bdev->bd_holder = NULL;
>> bdev->bd_holder_ops = NULL;
>> mutex_unlock(&bdev->bd_holder_lock);
>> - if (bdev->bd_write_holder)
>> + if (test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags))
>> unblock = true;
>> }
>> if (!whole->bd_holders)
>> @@ -625,7 +625,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
>> */
>> if (unblock) {
>> disk_unblock_events(bdev->bd_disk);
>> - bdev->bd_write_holder = false;
>> + clear_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
>> }
>> }
>>
>> @@ -878,9 +878,10 @@ struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
>> * writeable reference is too fragile given the way @mode is
>> * used in blkdev_get/put().
>> */
>> - if ((mode & BLK_OPEN_WRITE) && !bdev->bd_write_holder &&
>> + if ((mode & BLK_OPEN_WRITE) &&
>> + !test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags) &&
>> (disk->event_flags & DISK_EVENT_FLAG_BLOCK_ON_EXCL_WRITE)) {
>> - bdev->bd_write_holder = true;
>> + set_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
>> unblock_events = false;
>> }
>> }
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index fdf25b8d6e78..577a693165d8 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -482,7 +482,8 @@ __setup("fail_make_request=", setup_fail_make_request);
>>
>> bool should_fail_request(struct block_device *part, unsigned int bytes)
>> {
>> - return part->bd_make_it_fail && should_fail(&fail_make_request, bytes);
>> + return test_bit(BD_FLAG_MAKE_IT_FAIL, &part->flags) &&
>> + should_fail(&fail_make_request, bytes);
>> }
>>
>> static int __init fail_make_request_debugfs(void)
>> @@ -595,7 +596,7 @@ static void __submit_bio(struct bio *bio)
>> if (unlikely(!blk_crypto_bio_prep(&bio)))
>> return;
>>
>> - if (!bio->bi_bdev->bd_has_submit_bio) {
>> + if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags)) {
>> blk_mq_submit_bio(bio);
>> } else if (likely(bio_queue_enter(bio) == 0)) {
>> struct gendisk *disk = bio->bi_bdev->bd_disk;
>> @@ -703,7 +704,7 @@ void submit_bio_noacct_nocheck(struct bio *bio)
>> */
>> if (current->bio_list)
>> bio_list_add(¤t->bio_list[0], bio);
>> - else if (!bio->bi_bdev->bd_has_submit_bio)
>> + else if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags))
>> __submit_bio_noacct_mq(bio);
>> else
>> __submit_bio_noacct(bio);
>> diff --git a/block/genhd.c b/block/genhd.c
>> index c9d06f72c587..026f4c6d5b7e 100644
>> --- a/block/genhd.c
>> +++ b/block/genhd.c
>> @@ -413,7 +413,8 @@ 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 */
>> - disk->part0->bd_has_submit_bio = disk->fops->submit_bio != NULL;
>> + if (disk->fops->submit_bio)
>> + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags);
>>
>> /*
>> * If the driver provides an explicit major number it also must provide
>> @@ -1062,7 +1063,8 @@ static DEVICE_ATTR(diskseq, 0444, diskseq_show, NULL);
>> ssize_t part_fail_show(struct device *dev,
>> struct device_attribute *attr, char *buf)
>> {
>> - return sprintf(buf, "%d\n", dev_to_bdev(dev)->bd_make_it_fail);
>> + return sprintf(buf, "%d\n",
>> + test_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags));
>> }
>>
>> ssize_t part_fail_store(struct device *dev,
>> @@ -1072,7 +1074,7 @@ ssize_t part_fail_store(struct device *dev,
>> int i;
>>
>> if (count > 0 && sscanf(buf, "%d", &i) > 0)
>> - dev_to_bdev(dev)->bd_make_it_fail = i;
>> + set_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags);
>>
>> return count;
>> }
>> diff --git a/block/ioctl.c b/block/ioctl.c
>> index 4160f4e6bd5b..a548c718a5fb 100644
>> --- a/block/ioctl.c
>> +++ b/block/ioctl.c
>> @@ -394,7 +394,7 @@ static int blkdev_roset(struct block_device *bdev, unsigned cmd,
>> if (ret)
>> return ret;
>> }
>> - bdev->bd_read_only = n;
>> + set_bit(BD_FLAG_READ_ONLY, &bdev->flags);
>> return 0;
>> }
>>
>> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
>> index 52e264d5a830..95bd26c62655 100644
>> --- a/include/linux/blk_types.h
>> +++ b/include/linux/blk_types.h
>> @@ -37,17 +37,20 @@ struct bio_crypt_ctx;
>> #define PAGE_SECTORS (1 << PAGE_SECTORS_SHIFT)
>> #define SECTOR_MASK (PAGE_SECTORS - 1)
>>
>> +#define BD_FLAG_READ_ONLY 0 /* read-only-policy */
>> +#define BD_FLAG_WRITE_HOLDER 1
>> +#define BD_FLAG_HAS_SUBMIT_BIO 2
>> +#define BD_FLAG_MAKE_IT_FAIL 3
>> +
>> 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 flags;
>> unsigned long bd_stamp;
>> - bool bd_read_only; /* read-only policy */
>> u8 bd_partno;
>> - bool bd_write_holder;
>> - bool bd_has_submit_bio;
>
> The idea looks good, but not necessary to add new 8 bytes, and you may
> put all these flags and `bd_partno` into single 'unsigned long', and add
> helpers to retrieve part no, since there are not many ->bd_partno
> references.
Hi, I'm not quite sure which is the best way to implement this, anyway,
I come up with following implementation, can you please take a look
before I send a new version?
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 52e264d5a830..4e29774b14fd 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -37,6 +37,12 @@ struct bio_crypt_ctx;
#define PAGE_SECTORS (1 << PAGE_SECTORS_SHIFT)
#define SECTOR_MASK (PAGE_SECTORS - 1)
+#define BD_FLAG_READ_ONLY 0 /* read-only-policy */
+#define BD_FLAG_WRITE_HOLDER 1
+#define BD_FLAG_HAS_SUBMIT_BIO 2
+#define BD_FLAG_MAKE_IT_FAIL 3
+#define BD_FLAGS_MAX 16
+
struct block_device {
sector_t bd_start_sect;
sector_t bd_nr_sectors;
@@ -44,10 +50,20 @@ struct block_device {
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;
- bool bd_has_submit_bio;
+
+ /*
+ * In order to keep both flags and bd_partno in the first cacheline,
+ * first 16 bits are used for flags.
+ */
+ union {
+ unsigned long flags;
+ struct {
+#ifdef __LITTLE_ENDIAN
+ u16 __flags;
+#endif
+ u8 bd_partno;
+ };
+ };
Thanks,
Kuai
>
> Thanks,
> Ming
>
>
> .
>
On 11/20/23 10:38, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> There are multiple switches in struct block_device, use seperate bool
> fields for them is not gracefully. Add a new field flags and replace
> swithes to a bit, there are no functional changes, and preapre to add
> a new switch in the next patch.
>
> Signed-off-by: Yu Kuai <[email protected]>
> ---
> block/bdev.c | 15 ++++++++-------
> block/blk-core.c | 7 ++++---
> block/genhd.c | 8 +++++---
> block/ioctl.c | 2 +-
> include/linux/blk_types.h | 12 ++++++------
> include/linux/blkdev.h | 5 +++--
> 6 files changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/block/bdev.c b/block/bdev.c
> index fc8d28d77495..cb849bcf61ae 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -408,10 +408,10 @@ struct block_device *bdev_alloc(struct gendisk *disk, u8 partno)
> bdev->bd_partno = partno;
> bdev->bd_inode = inode;
> bdev->bd_queue = disk->queue;
> - if (partno)
> - bdev->bd_has_submit_bio = disk->part0->bd_has_submit_bio;
> + if (partno && test_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags))
> + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
> else
> - bdev->bd_has_submit_bio = false;
> + clear_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
> bdev->bd_stats = alloc_percpu(struct disk_stats);
> if (!bdev->bd_stats) {
> iput(inode);
Couldn't you achieve the very same result by using
'READ_ONCE()/WRITE_ONCE()' and keep the structure as-is?
Cheers,
Hannes--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg
Managing Directors: I. Totev, A. McDonald, W. Knoblich
(HRB 36809, AG Nürnberg)
On Mon, Nov 20, 2023 at 04:01:03PM +0800, Yu Kuai wrote:
> Hi,
>
> 在 2023/11/20 11:00, Ming Lei 写道:
> > On Mon, Nov 20, 2023 at 05:38:46PM +0800, Yu Kuai wrote:
> > > From: Yu Kuai <[email protected]>
> > >
> > > There are multiple switches in struct block_device, use seperate bool
> > > fields for them is not gracefully. Add a new field flags and replace
> > > swithes to a bit, there are no functional changes, and preapre to add
> > > a new switch in the next patch.
> > >
> > > Signed-off-by: Yu Kuai <[email protected]>
> > > ---
> > > block/bdev.c | 15 ++++++++-------
> > > block/blk-core.c | 7 ++++---
> > > block/genhd.c | 8 +++++---
> > > block/ioctl.c | 2 +-
> > > include/linux/blk_types.h | 12 ++++++------
> > > include/linux/blkdev.h | 5 +++--
> > > 6 files changed, 27 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/block/bdev.c b/block/bdev.c
> > > index fc8d28d77495..cb849bcf61ae 100644
> > > --- a/block/bdev.c
> > > +++ b/block/bdev.c
> > > @@ -408,10 +408,10 @@ struct block_device *bdev_alloc(struct gendisk *disk, u8 partno)
> > > bdev->bd_partno = partno;
> > > bdev->bd_inode = inode;
> > > bdev->bd_queue = disk->queue;
> > > - if (partno)
> > > - bdev->bd_has_submit_bio = disk->part0->bd_has_submit_bio;
> > > + if (partno && test_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags))
> > > + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
> > > else
> > > - bdev->bd_has_submit_bio = false;
> > > + clear_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
> > > bdev->bd_stats = alloc_percpu(struct disk_stats);
> > > if (!bdev->bd_stats) {
> > > iput(inode);
> > > @@ -612,7 +612,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
> > > bdev->bd_holder = NULL;
> > > bdev->bd_holder_ops = NULL;
> > > mutex_unlock(&bdev->bd_holder_lock);
> > > - if (bdev->bd_write_holder)
> > > + if (test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags))
> > > unblock = true;
> > > }
> > > if (!whole->bd_holders)
> > > @@ -625,7 +625,7 @@ static void bd_end_claim(struct block_device *bdev, void *holder)
> > > */
> > > if (unblock) {
> > > disk_unblock_events(bdev->bd_disk);
> > > - bdev->bd_write_holder = false;
> > > + clear_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
> > > }
> > > }
> > > @@ -878,9 +878,10 @@ struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
> > > * writeable reference is too fragile given the way @mode is
> > > * used in blkdev_get/put().
> > > */
> > > - if ((mode & BLK_OPEN_WRITE) && !bdev->bd_write_holder &&
> > > + if ((mode & BLK_OPEN_WRITE) &&
> > > + !test_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags) &&
> > > (disk->event_flags & DISK_EVENT_FLAG_BLOCK_ON_EXCL_WRITE)) {
> > > - bdev->bd_write_holder = true;
> > > + set_bit(BD_FLAG_WRITE_HOLDER, &bdev->flags);
> > > unblock_events = false;
> > > }
> > > }
> > > diff --git a/block/blk-core.c b/block/blk-core.c
> > > index fdf25b8d6e78..577a693165d8 100644
> > > --- a/block/blk-core.c
> > > +++ b/block/blk-core.c
> > > @@ -482,7 +482,8 @@ __setup("fail_make_request=", setup_fail_make_request);
> > > bool should_fail_request(struct block_device *part, unsigned int bytes)
> > > {
> > > - return part->bd_make_it_fail && should_fail(&fail_make_request, bytes);
> > > + return test_bit(BD_FLAG_MAKE_IT_FAIL, &part->flags) &&
> > > + should_fail(&fail_make_request, bytes);
> > > }
> > > static int __init fail_make_request_debugfs(void)
> > > @@ -595,7 +596,7 @@ static void __submit_bio(struct bio *bio)
> > > if (unlikely(!blk_crypto_bio_prep(&bio)))
> > > return;
> > > - if (!bio->bi_bdev->bd_has_submit_bio) {
> > > + if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags)) {
> > > blk_mq_submit_bio(bio);
> > > } else if (likely(bio_queue_enter(bio) == 0)) {
> > > struct gendisk *disk = bio->bi_bdev->bd_disk;
> > > @@ -703,7 +704,7 @@ void submit_bio_noacct_nocheck(struct bio *bio)
> > > */
> > > if (current->bio_list)
> > > bio_list_add(¤t->bio_list[0], bio);
> > > - else if (!bio->bi_bdev->bd_has_submit_bio)
> > > + else if (!test_bit(BD_FLAG_HAS_SUBMIT_BIO, &bio->bi_bdev->flags))
> > > __submit_bio_noacct_mq(bio);
> > > else
> > > __submit_bio_noacct(bio);
> > > diff --git a/block/genhd.c b/block/genhd.c
> > > index c9d06f72c587..026f4c6d5b7e 100644
> > > --- a/block/genhd.c
> > > +++ b/block/genhd.c
> > > @@ -413,7 +413,8 @@ 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 */
> > > - disk->part0->bd_has_submit_bio = disk->fops->submit_bio != NULL;
> > > + if (disk->fops->submit_bio)
> > > + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags);
> > > /*
> > > * If the driver provides an explicit major number it also must provide
> > > @@ -1062,7 +1063,8 @@ static DEVICE_ATTR(diskseq, 0444, diskseq_show, NULL);
> > > ssize_t part_fail_show(struct device *dev,
> > > struct device_attribute *attr, char *buf)
> > > {
> > > - return sprintf(buf, "%d\n", dev_to_bdev(dev)->bd_make_it_fail);
> > > + return sprintf(buf, "%d\n",
> > > + test_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags));
> > > }
> > > ssize_t part_fail_store(struct device *dev,
> > > @@ -1072,7 +1074,7 @@ ssize_t part_fail_store(struct device *dev,
> > > int i;
> > > if (count > 0 && sscanf(buf, "%d", &i) > 0)
> > > - dev_to_bdev(dev)->bd_make_it_fail = i;
> > > + set_bit(BD_FLAG_MAKE_IT_FAIL, &dev_to_bdev(dev)->flags);
> > > return count;
> > > }
> > > diff --git a/block/ioctl.c b/block/ioctl.c
> > > index 4160f4e6bd5b..a548c718a5fb 100644
> > > --- a/block/ioctl.c
> > > +++ b/block/ioctl.c
> > > @@ -394,7 +394,7 @@ static int blkdev_roset(struct block_device *bdev, unsigned cmd,
> > > if (ret)
> > > return ret;
> > > }
> > > - bdev->bd_read_only = n;
> > > + set_bit(BD_FLAG_READ_ONLY, &bdev->flags);
> > > return 0;
> > > }
> > > diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> > > index 52e264d5a830..95bd26c62655 100644
> > > --- a/include/linux/blk_types.h
> > > +++ b/include/linux/blk_types.h
> > > @@ -37,17 +37,20 @@ struct bio_crypt_ctx;
> > > #define PAGE_SECTORS (1 << PAGE_SECTORS_SHIFT)
> > > #define SECTOR_MASK (PAGE_SECTORS - 1)
> > > +#define BD_FLAG_READ_ONLY 0 /* read-only-policy */
> > > +#define BD_FLAG_WRITE_HOLDER 1
> > > +#define BD_FLAG_HAS_SUBMIT_BIO 2
> > > +#define BD_FLAG_MAKE_IT_FAIL 3
> > > +
> > > 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 flags;
> > > unsigned long bd_stamp;
> > > - bool bd_read_only; /* read-only policy */
> > > u8 bd_partno;
> > > - bool bd_write_holder;
> > > - bool bd_has_submit_bio;
> >
> > The idea looks good, but not necessary to add new 8 bytes, and you may
> > put all these flags and `bd_partno` into single 'unsigned long', and add
> > helpers to retrieve part no, since there are not many ->bd_partno
> > references.
>
> Hi, I'm not quite sure which is the best way to implement this, anyway,
> I come up with following implementation, can you please take a look
> before I send a new version?
>
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 52e264d5a830..4e29774b14fd 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -37,6 +37,12 @@ struct bio_crypt_ctx;
> #define PAGE_SECTORS (1 << PAGE_SECTORS_SHIFT)
> #define SECTOR_MASK (PAGE_SECTORS - 1)
>
> +#define BD_FLAG_READ_ONLY 0 /* read-only-policy */
> +#define BD_FLAG_WRITE_HOLDER 1
> +#define BD_FLAG_HAS_SUBMIT_BIO 2
> +#define BD_FLAG_MAKE_IT_FAIL 3
> +#define BD_FLAGS_MAX 16
> +
> struct block_device {
> sector_t bd_start_sect;
> sector_t bd_nr_sectors;
> @@ -44,10 +50,20 @@ struct block_device {
> 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;
> - bool bd_has_submit_bio;
> +
> + /*
> + * In order to keep both flags and bd_partno in the first cacheline,
> + * first 16 bits are used for flags.
> + */
> + union {
> + unsigned long flags;
> + struct {
> +#ifdef __LITTLE_ENDIAN
> + u16 __flags;
> +#endif
> + u8 bd_partno;
> + };
> + };
You can follow the similar pattern of bio->bi_opf.
Thanks,
Ming
Hi,
在 2023/11/20 18:45, Hannes Reinecke 写道:
> On 11/20/23 10:38, Yu Kuai wrote:
>> From: Yu Kuai <[email protected]>
>>
>> There are multiple switches in struct block_device, use seperate bool
>> fields for them is not gracefully. Add a new field flags and replace
>> swithes to a bit, there are no functional changes, and preapre to add
>> a new switch in the next patch.
>>
>> Signed-off-by: Yu Kuai <[email protected]>
>> ---
>> block/bdev.c | 15 ++++++++-------
>> block/blk-core.c | 7 ++++---
>> block/genhd.c | 8 +++++---
>> block/ioctl.c | 2 +-
>> include/linux/blk_types.h | 12 ++++++------
>> include/linux/blkdev.h | 5 +++--
>> 6 files changed, 27 insertions(+), 22 deletions(-)
>>
>> diff --git a/block/bdev.c b/block/bdev.c
>> index fc8d28d77495..cb849bcf61ae 100644
>> --- a/block/bdev.c
>> +++ b/block/bdev.c
>> @@ -408,10 +408,10 @@ struct block_device *bdev_alloc(struct gendisk
>> *disk, u8 partno)
>> bdev->bd_partno = partno;
>> bdev->bd_inode = inode;
>> bdev->bd_queue = disk->queue;
>> - if (partno)
>> - bdev->bd_has_submit_bio = disk->part0->bd_has_submit_bio;
>> + if (partno && test_bit(BD_FLAG_HAS_SUBMIT_BIO, &disk->part0->flags))
>> + set_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
>> else
>> - bdev->bd_has_submit_bio = false;
>> + clear_bit(BD_FLAG_HAS_SUBMIT_BIO, &bdev->flags);
>> bdev->bd_stats = alloc_percpu(struct disk_stats);
>> if (!bdev->bd_stats) {
>> iput(inode);
>
> Couldn't you achieve the very same result by using
> 'READ_ONCE()/WRITE_ONCE()' and keep the structure as-is?
Yes, remove this patch and add a new bool field in the next patch can
work as well, the idea here is replace multiple fields with a single
flag, and just use the flag in next patch. If people thinks add a new
bool field is better, I'll follow that.
Thanks,
Kuai
>
> Cheers,
>
> Hannes--
> Dr. Hannes Reinecke Kernel Storage Architect
> [email protected] +49 911 74053 688
> SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg
> Managing Directors: I. Totev, A. McDonald, W. Knoblich
> (HRB 36809, AG Nürnberg)
>
> .
>