Skip to contentSkip to conversation

Introduce bdev_zone_is_seq()

PATCH Superseded Damien Le Moal 4 participants 9 messages Nov 6, 2024 – Nov 7, 2024

Subsystems (2)
Block layer Zoned block device (block layer)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Damien Le Moal

author maintainer
#0 lore raw

cover Introduce bdev_zone_is_seq()

Allow file systems to safely access a block device gendisk bitmap of conventional zones to determine a zone type by:
1) Patch 1 - changing the gendisk conv_zones_bitmap to be RCU protected
2) Patch 2 - Introducing the helper function bdev_zone_is_seq()

This is in preparation for use in btrfs to remove the btrfs-managed bitmap of conventional zones and in zoned support for xfs.

Damien Le Moal (2):
block: RCU protect disk->conv_zones_bitmap
block: Add a public bdev_zone_is_seq() helper

 block/blk-zoned.c      | 43 ++++++++++++++++++++++++------------------
 include/linux/blkdev.h | 28 ++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 19 deletions(-)
Signature · 1 line
2.47.0
author maintainer
#1 lore raw

PATCH 1/2 block: RCU protect disk->conv_zones_bitmap

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 <[email protected]>
---
 block/blk-zoned.c      | 43 ++++++++++++++++++++++++++++++------------
 include/linux/blkdev.h |  2 +-
 2 files changed, 32 insertions(+), 13 deletions(-)
block/blk-zoned.c +31 -12
index a287577d1ad6..7a7855555d6d 100644
@@ -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);
include/linux/blkdev.h +1 -1
index 6d1413bd69a5..5687eb2a019c 100644
@@ -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;
Signature · 1 line
2.47.0
#2 lore raw

Re: [PATCH 1/2] block: RCU protect disk->conv_zones_bitmap

Quoted 10 lines · Damien Le Moal
block/blk-zoned.c +0 -2
index a287577d1ad6..7a7855555d6d 100644
@@ -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;

block/blk-zoned.c int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode, · line 361

Quoted 10 lines
block/blk-zoned.c +8 -1
-	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.

author maintainer
#3 lore raw

Re: [PATCH 1/2] block: RCU protect disk->conv_zones_bitmap

Quoted 7 lines · Bart Van Assche
Quoted 6 lines · Damien Le Moal
block/blk-zoned.c +0 -0
index a287577d1ad6..7a7855555d6d 100644
@@ -350,9 +350,14 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode,
Quoted 16 lines
Quoted 4 lines
block/blk-zoned.c +0 -2
  static inline bool disk_zone_is_conv(struct gendisk *disk, sector_t sector)
  {
-	if (!disk->conv_zones_bitmap)
-		return false;

block/blk-zoned.c int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode, · line 361

Quoted 10 lines
block/blk-zoned.c +8 -1
-	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.

Quoted 5 lines

Otherwise the two patches in this series look good to me.

Thanks,

Bart.

Signature · 2 lines
Damien Le Moal
Western Digital Research
maintainer
#4 lore raw

Re: [PATCH 1/2] block: RCU protect disk->conv_zones_bitmap

Quoted 25 lines · Damien Le Moal
Quoted 7 lines · Bart Van Assche
Quoted 6 lines · Damien Le Moal
block/blk-zoned.c +0 -0
index a287577d1ad6..7a7855555d6d 100644
@@ -350,9 +350,14 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode,
Quoted 16 lines
Quoted 4 lines
block/blk-zoned.c +0 -2
  static inline bool disk_zone_is_conv(struct gendisk *disk, sector_t sector)
  {
-	if (!disk->conv_zones_bitmap)
-		return false;

block/blk-zoned.c int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode, · line 361

Quoted 10 lines
block/blk-zoned.c +8 -1
-	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).

Quoted 3 lines

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.

Signature · 1 line
Jens Axboe
author maintainer
#5 lore raw

Re: [PATCH 1/2] block: RCU protect disk->conv_zones_bitmap

Quoted 26 lines · Jens Axboe
Quoted 25 lines · Damien Le Moal
Quoted 23 lines · Bart Van Assche
Quoted 10 lines · Damien Le Moal
block/blk-zoned.c +0 -2
index a287577d1ad6..7a7855555d6d 100644
@@ -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;

block/blk-zoned.c int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode, · line 361

Quoted 10 lines
block/blk-zoned.c +8 -1
-	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).

Quoted 7 lines
Quoted 3 lines

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 :)

Signature · 2 lines
Damien Le Moal
Western Digital Research
#6 lore raw

Re: [PATCH 1/2] block: RCU protect disk->conv_zones_bitmap

In reply to Damien Le Moal

As sparse rcu warnings got in my radar for something else, I did run sparse over this and it complains. It will need the little gem below to fix (and a rebase of the second patch). Otherwise the series looks great and way better than my initial hack, thanks!

block/blk-zoned.c +5 -5
index 7a7855555d6d..bf4458b11720 100644
@@ -350,11 +350,12 @@ 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)
 {
+	unsigned long *bitmap;
 	bool is_conv;
 
 	rcu_read_lock();
-	is_conv = disk->conv_zones_bitmap &&
-		test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap);
+	bitmap = rcu_dereference(disk->conv_zones_bitmap);
+	is_conv = bitmap && test_bit(disk_zone_no(disk, sector), bitmap);
 	rcu_read_unlock();
 
 	return is_conv;

@@ -1467,11 +1468,10 @@ static unsigned int disk_set_conv_zones_bitmap(struct gendisk *disk,
 	unsigned long flags;
 
 	spin_lock_irqsave(&disk->zone_wplugs_lock, flags);
+	if (bitmap)
+		nr_conv_zones = bitmap_weight(bitmap, disk->nr_zones);
 	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);
author maintainer
#7 lore raw

Re: [PATCH 1/2] block: RCU protect disk->conv_zones_bitmap

Quoted 4 lines · Christoph Hellwig

As sparse rcu warnings got in my radar for something else, I did run sparse over this and it complains. It will need the little gem below to fix (and a rebase of the second patch). Otherwise the series looks great and way better than my initial hack, thanks!

OK. I will integrate this and send v2.

Quoted 34 lines
block/blk-zoned.c +5 -5
index 7a7855555d6d..bf4458b11720 100644
@@ -350,11 +350,12 @@ 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)
 {
+	unsigned long *bitmap;
 	bool is_conv;
 
 	rcu_read_lock();
-	is_conv = disk->conv_zones_bitmap &&
-		test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap);
+	bitmap = rcu_dereference(disk->conv_zones_bitmap);
+	is_conv = bitmap && test_bit(disk_zone_no(disk, sector), bitmap);
 	rcu_read_unlock();
 
 	return is_conv;

@@ -1467,11 +1468,10 @@ static unsigned int disk_set_conv_zones_bitmap(struct gendisk *disk,
 	unsigned long flags;
 
 	spin_lock_irqsave(&disk->zone_wplugs_lock, flags);
+	if (bitmap)
+		nr_conv_zones = bitmap_weight(bitmap, disk->nr_zones);
 	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);
Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer
#8 lore raw

PATCH 2/2 block: Add a public bdev_zone_is_seq() helper

In reply to Damien Le Moal

Turn the private disk_zone_is_conv() function in blk-zoned.c into a public and documented bdev_zone_is_seq() helper with the inverse polarity of the original function, also adding a check for non-zoned devices so that all file systems can use the helper, even with a regular block device.

Suggested-by Christoph Hellwig <[email protected]>
Signed-off-by Damien Le Moal <[email protected]>
---
 block/blk-zoned.c      | 16 ++--------------
 include/linux/blkdev.h | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 14 deletions(-)
block/blk-zoned.c +2 -14
index 7a7855555d6d..a8c1e4106d6d 100644
@@ -348,18 +348,6 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode,
 	return ret;
 }
 
-static inline bool disk_zone_is_conv(struct gendisk *disk, sector_t sector)
-{
-	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)
 {
 	return zone->start + zone->len >= get_capacity(disk);

@@ -714,7 +702,7 @@ static bool blk_zone_wplug_handle_reset_or_finish(struct bio *bio,
 	struct blk_zone_wplug *zwplug;
 
 	/* Conventional zones cannot be reset nor finished. */
-	if (disk_zone_is_conv(disk, sector)) {
+	if (!bdev_zone_is_seq(bio->bi_bdev, sector)) {
 		bio_io_error(bio);
 		return true;
 	}

@@ -968,7 +956,7 @@ static bool blk_zone_wplug_handle_write(struct bio *bio, unsigned int nr_segs)
 	}
 
 	/* Conventional zones do not need write plugging. */
-	if (disk_zone_is_conv(disk, sector)) {
+	if (!bdev_zone_is_seq(bio->bi_bdev, sector)) {
 		/* Zone append to conventional zones is not allowed. */
 		if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
 			bio_io_error(bio);
include/linux/blkdev.h +26 -0
index 5687eb2a019c..24fef307d594 100644
@@ -1380,6 +1380,32 @@ static inline bool bdev_is_zone_start(struct block_device *bdev,
 	return bdev_offset_from_zone_start(bdev, sector) == 0;
 }
 
+/**
+ * bdev_zone_is_seq - check if a sector belongs to a sequential write zone
+ * @bdev:	block device to check
+ * @sector:	sector number
+ *
+ * Check if @sector on @bdev is contained in a sequential write required zone.
+ */
+static inline bool bdev_zone_is_seq(struct block_device *bdev, sector_t sector)
+{
+	bool is_seq = false;
+
+#if IS_ENABLED(CONFIG_BLK_DEV_ZONED)
+	if (bdev_is_zoned(bdev)) {
+		struct gendisk *disk = bdev->bd_disk;
+
+		rcu_read_lock();
+		is_seq = !disk->conv_zones_bitmap ||
+			!test_bit(disk_zone_no(disk, sector),
+				  disk->conv_zones_bitmap);
+		rcu_read_unlock();
+	}
+#endif
+
+	return is_seq;
+}
+
 static inline int queue_dma_alignment(const struct request_queue *q)
 {
 	return q->limits.dma_alignment;
Signature · 1 line
2.47.0