| Message ID | 20241106231323.8008-2-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce bdev_zone_is_seq() | expand |
On 11/6/24 3:13 PM, Damien Le Moal wrote: > diff --git a/block/blk-zoned.c b/block/blk-zoned.c > index a287577d1ad6..7a7855555d6d 100644 > --- a/block/blk-zoned.c > +++ b/block/blk-zoned.c > @@ -350,9 +350,14 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode, > > static inline bool disk_zone_is_conv(struct gendisk *disk, sector_t sector) > { > - if (!disk->conv_zones_bitmap) > - return false; > - return test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap); > + bool is_conv; > + > + rcu_read_lock(); > + is_conv = disk->conv_zones_bitmap && > + test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap); > + rcu_read_unlock(); > + > + return is_conv; > } The above code can be simplified significantly by using guard(rcu). Otherwise the two patches in this series look good to me. Thanks, Bart.
On 11/7/24 08:20, Bart Van Assche wrote: > On 11/6/24 3:13 PM, Damien Le Moal wrote: >> diff --git a/block/blk-zoned.c b/block/blk-zoned.c >> index a287577d1ad6..7a7855555d6d 100644 >> --- a/block/blk-zoned.c >> +++ b/block/blk-zoned.c >> @@ -350,9 +350,14 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode, >> >> static inline bool disk_zone_is_conv(struct gendisk *disk, sector_t sector) >> { >> - if (!disk->conv_zones_bitmap) >> - return false; >> - return test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap); >> + bool is_conv; >> + >> + rcu_read_lock(); >> + is_conv = disk->conv_zones_bitmap && >> + test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap); >> + rcu_read_unlock(); >> + >> + return is_conv; >> } > > The above code can be simplified significantly by using guard(rcu). I personally dislike very much annotations that hide code. So unless Jens prefers using guard(rcu), I would prefer leaving the code as it is. > Otherwise the two patches in this series look good to me. > > Thanks, > > Bart.
On 11/6/24 4:44 PM, Damien Le Moal wrote: > On 11/7/24 08:20, Bart Van Assche wrote: >> On 11/6/24 3:13 PM, Damien Le Moal wrote: >>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c >>> index a287577d1ad6..7a7855555d6d 100644 >>> --- a/block/blk-zoned.c >>> +++ b/block/blk-zoned.c >>> @@ -350,9 +350,14 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode, >>> >>> static inline bool disk_zone_is_conv(struct gendisk *disk, sector_t sector) >>> { >>> - if (!disk->conv_zones_bitmap) >>> - return false; >>> - return test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap); >>> + bool is_conv; >>> + >>> + rcu_read_lock(); >>> + is_conv = disk->conv_zones_bitmap && >>> + test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap); >>> + rcu_read_unlock(); >>> + >>> + return is_conv; >>> } >> >> The above code can be simplified significantly by using guard(rcu). > > I personally dislike very much annotations that hide code. So unless > Jens prefers using guard(rcu), I would prefer leaving the code as it > is. I don't mind it, and I do use it myself when it makes sense - but imho it's up to the person writing the code, particularly when it's their code in the first place.
On 11/7/24 09:02, Jens Axboe wrote: > On 11/6/24 4:44 PM, Damien Le Moal wrote: >> On 11/7/24 08:20, Bart Van Assche wrote: >>> On 11/6/24 3:13 PM, Damien Le Moal wrote: >>>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c >>>> index a287577d1ad6..7a7855555d6d 100644 >>>> --- a/block/blk-zoned.c >>>> +++ b/block/blk-zoned.c >>>> @@ -350,9 +350,14 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode, >>>> >>>> static inline bool disk_zone_is_conv(struct gendisk *disk, sector_t sector) >>>> { >>>> - if (!disk->conv_zones_bitmap) >>>> - return false; >>>> - return test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap); >>>> + bool is_conv; >>>> + >>>> + rcu_read_lock(); >>>> + is_conv = disk->conv_zones_bitmap && >>>> + test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap); >>>> + rcu_read_unlock(); >>>> + >>>> + return is_conv; >>>> } >>> >>> The above code can be simplified significantly by using guard(rcu). >> >> I personally dislike very much annotations that hide code. So unless >> Jens prefers using guard(rcu), I would prefer leaving the code as it >> is. > > I don't mind it, and I do use it myself when it makes sense - but imho > it's up to the person writing the code, particularly when it's their > code in the first place. OK, then I would prefer leaving the code as is :)
diff --git a/block/blk-zoned.c b/block/blk-zoned.c index a287577d1ad6..7a7855555d6d 100644 --- a/block/blk-zoned.c +++ b/block/blk-zoned.c @@ -350,9 +350,14 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode, static inline bool disk_zone_is_conv(struct gendisk *disk, sector_t sector) { - if (!disk->conv_zones_bitmap) - return false; - return test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap); + bool is_conv; + + rcu_read_lock(); + is_conv = disk->conv_zones_bitmap && + test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap); + rcu_read_unlock(); + + return is_conv; } static bool disk_zone_is_last(struct gendisk *disk, struct blk_zone *zone) @@ -1455,6 +1460,25 @@ static void disk_destroy_zone_wplugs_hash_table(struct gendisk *disk) disk->zone_wplugs_hash_bits = 0; } +static unsigned int disk_set_conv_zones_bitmap(struct gendisk *disk, + unsigned long *bitmap) +{ + unsigned int nr_conv_zones = 0; + unsigned long flags; + + spin_lock_irqsave(&disk->zone_wplugs_lock, flags); + bitmap = rcu_replace_pointer(disk->conv_zones_bitmap, bitmap, + lockdep_is_held(&disk->zone_wplugs_lock)); + if (disk->conv_zones_bitmap) + nr_conv_zones = bitmap_weight(disk->conv_zones_bitmap, + disk->nr_zones); + spin_unlock_irqrestore(&disk->zone_wplugs_lock, flags); + + kfree_rcu_mightsleep(bitmap); + + return nr_conv_zones; +} + void disk_free_zone_resources(struct gendisk *disk) { if (!disk->zone_wplugs_pool) @@ -1478,8 +1502,7 @@ void disk_free_zone_resources(struct gendisk *disk) mempool_destroy(disk->zone_wplugs_pool); disk->zone_wplugs_pool = NULL; - bitmap_free(disk->conv_zones_bitmap); - disk->conv_zones_bitmap = NULL; + disk_set_conv_zones_bitmap(disk, NULL); disk->zone_capacity = 0; disk->last_zone_capacity = 0; disk->nr_zones = 0; @@ -1538,17 +1561,15 @@ static int disk_update_zone_resources(struct gendisk *disk, struct blk_revalidate_zone_args *args) { struct request_queue *q = disk->queue; - unsigned int nr_seq_zones, nr_conv_zones = 0; + unsigned int nr_seq_zones, nr_conv_zones; unsigned int pool_size; struct queue_limits lim; disk->nr_zones = args->nr_zones; disk->zone_capacity = args->zone_capacity; disk->last_zone_capacity = args->last_zone_capacity; - swap(disk->conv_zones_bitmap, args->conv_zones_bitmap); - if (disk->conv_zones_bitmap) - nr_conv_zones = bitmap_weight(disk->conv_zones_bitmap, - disk->nr_zones); + nr_conv_zones = + disk_set_conv_zones_bitmap(disk, args->conv_zones_bitmap); if (nr_conv_zones >= disk->nr_zones) { pr_warn("%s: Invalid number of conventional zones %u / %u\n", disk->disk_name, nr_conv_zones, disk->nr_zones); @@ -1817,8 +1838,6 @@ int blk_revalidate_disk_zones(struct gendisk *disk) disk_free_zone_resources(disk); blk_mq_unfreeze_queue(q); - kfree(args.conv_zones_bitmap); - return ret; } EXPORT_SYMBOL_GPL(blk_revalidate_disk_zones); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 6d1413bd69a5..5687eb2a019c 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -195,7 +195,7 @@ struct gendisk { unsigned int nr_zones; unsigned int zone_capacity; unsigned int last_zone_capacity; - unsigned long *conv_zones_bitmap; + unsigned long __rcu *conv_zones_bitmap; unsigned int zone_wplugs_hash_bits; spinlock_t zone_wplugs_lock; struct mempool_s *zone_wplugs_pool;
Ensure that a disk revalidation changing the conventional zones bitmap of a disk does not cause invalid memory references when using the disk_zone_is_conv() helper by RCU protecting the disk->conv_zones_bitmap pointer. disk_zone_is_conv() is modified to operate under the RCU read lock and the function disk_set_conv_zones_bitmap() is added to update a disk conv_zones_bitmap pointer using rcu_replace_pointer() with the disk zone_wplugs_lock spinlock held. disk_free_zone_resources() is modified to call disk_update_zone_resources() with a NULL bitmap pointer to free the disk conv_zones_bitmap. disk_set_conv_zones_bitmap() is also used in disk_update_zone_resources() to set the new (revalidated) bitmap and free the old one. Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- block/blk-zoned.c | 43 ++++++++++++++++++++++++++++++------------ include/linux/blkdev.h | 2 +- 2 files changed, 32 insertions(+), 13 deletions(-)