| Message ID | 20231120093847.2228127-2-yukuai1@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: warn once for each partition in bio_check_ro() | expand |
On Mon, Nov 20, 2023 at 05:38:46PM +0800, Yu Kuai wrote: > From: Yu Kuai <yukuai3@huawei.com> > > 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 <yukuai3@huawei.com> > --- > 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 <yukuai3@huawei.com> >> >> 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 <yukuai3@huawei.com> >> --- >> 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 <yukuai3@huawei.com> > > > > > > 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 <yukuai3@huawei.com> > > > --- > > > 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 <yukuai3@huawei.com> >>>> >>>> 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 <yukuai3@huawei.com> >>>> --- >>>> 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 <yukuai3@huawei.com> >> >> 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 <yukuai3@huawei.com> >> --- >> 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 <yukuai3@huawei.com> > > 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 <yukuai3@huawei.com> > --- > 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 hare@suse.de +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 <yukuai3@huawei.com> > > > > > > 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 <yukuai3@huawei.com> > > > --- > > > 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 <yukuai3@huawei.com> >> >> 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 <yukuai3@huawei.com> >> --- >> 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 > hare@suse.de +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) > > . >
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);