[PATCH 0/6] Minor improvements for the zoned block storage code

[PATCH 0/6] Minor improvements for the zoned block storage code

[Bart Van Assche]

Hi Jens,

This patch series includes several minor improvements for the zoned block
storage code. None of these patches changes the behavior of that code.

Please consider these patches for the next merge window.

Thanks,

Bart.

Bart Van Assche (6):
  blk-zoned: Minimize #include directives
  blk-zoned: Document the locking order
  blk-zoned: Document locking assumptions
  blk-zoned: Improve the queue reference count strategy documentation
  blk-zoned: Uninline functions that are not in the hot path
  blk-zoned: Split queue_zone_wplugs_show()

 block/blk-zoned.c | 67 +++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 32 deletions(-)

[PATCH 1/6] blk-zoned: Minimize #include directives

[Bart Van Assche]

Only include those header files that are necessary.

Cc: Damien Le Moal <dlemoal@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-zoned.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 84da1eadff64..1575b887fa38 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -11,12 +11,8 @@
  */
 
 #include <linux/kernel.h>
-#include <linux/module.h>
 #include <linux/blkdev.h>
 #include <linux/blk-mq.h>
-#include <linux/mm.h>
-#include <linux/vmalloc.h>
-#include <linux/sched/mm.h>
 #include <linux/spinlock.h>
 #include <linux/refcount.h>
 #include <linux/mempool.h>

Re: [PATCH 1/6] blk-zoned: Minimize #include directives

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 1/6] blk-zoned: Minimize #include directives

[Damien Le Moal]

On 2024/12/16 13:02, Bart Van Assche wrote:
> Only include those header files that are necessary.
> 
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 2/6] blk-zoned: Document the locking order

[Bart Van Assche]

Document that zwplug->lock is the outer lock relative to
disk->zone_wplugs_lock.

Cc: Damien Le Moal <dlemoal@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-zoned.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 1575b887fa38..3e42372fa832 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -46,7 +46,8 @@ static const char *const zone_cond_name[] = {
  *       reference is dropped whenever the zone of the zone write plug is reset,
  *       finished and when the zone becomes full (last write BIO to the zone
  *       completes).
- * @lock: Spinlock to atomically manipulate the plug.
+ * @lock: Spinlock to atomically manipulate the plug. Outer lock relative to
+ *	disk->zone_wplugs_lock.
  * @flags: Flags indicating the plug state.
  * @zone_no: The number of the zone the plug is managing.
  * @wp_offset: The zone write pointer location relative to the start of the zone

Re: [PATCH 2/6] blk-zoned: Document the locking order

[Christoph Hellwig]

On Mon, Dec 16, 2024 at 01:02:40PM -0800, Bart Van Assche wrote:
> Document that zwplug->lock is the outer lock relative to
> disk->zone_wplugs_lock.
> 
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  block/blk-zoned.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index 1575b887fa38..3e42372fa832 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
> @@ -46,7 +46,8 @@ static const char *const zone_cond_name[] = {
>   *       reference is dropped whenever the zone of the zone write plug is reset,
>   *       finished and when the zone becomes full (last write BIO to the zone
>   *       completes).
> - * @lock: Spinlock to atomically manipulate the plug.
> + * @lock: Spinlock to atomically manipulate the plug. Outer lock relative to
> + *	disk->zone_wplugs_lock.

That's pretty odd wording.  If you think this information is important
we should probably have a comment at the top of the file explaining the
lock order like in many MM or VFS source files.

[PATCH 3/6] blk-zoned: Document locking assumptions

[Bart Van Assche]

Document which functions expect that their callers must hold a lock.

Cc: Damien Le Moal <dlemoal@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-zoned.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 3e42372fa832..ca44b2d6727c 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -460,6 +460,8 @@ static inline void disk_put_zone_wplug(struct blk_zone_wplug *zwplug)
 static inline bool disk_should_remove_zone_wplug(struct gendisk *disk,
 						 struct blk_zone_wplug *zwplug)
 {
+	lockdep_assert_held(&zwplug->lock);
+
 	/* If the zone write plug was already removed, we are done. */
 	if (zwplug->flags & BLK_ZONE_WPLUG_UNHASHED)
 		return false;
@@ -914,6 +916,8 @@ static bool blk_zone_wplug_prepare_bio(struct blk_zone_wplug *zwplug,
 {
 	struct gendisk *disk = bio->bi_bdev->bd_disk;
 
+	lockdep_assert_held(&zwplug->lock);
+
 	/*
 	 * If we lost track of the zone write pointer due to a write error,
 	 * the user must either execute a report zones, reset the zone or finish

Re: [PATCH 3/6] blk-zoned: Document locking assumptions

[Christoph Hellwig]

On Mon, Dec 16, 2024 at 01:02:41PM -0800, Bart Van Assche wrote:
> Document which functions expect that their callers must hold a lock.

Looks good:

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

Re: [PATCH 3/6] blk-zoned: Document locking assumptions

[Damien Le Moal]

On 2024/12/16 13:02, Bart Van Assche wrote:
> Document which functions expect that their callers must hold a lock.
> 
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 4/6] blk-zoned: Improve the queue reference count strategy documentation

[Bart Van Assche]

For the blk_queue_exit() calls, document where the corresponding code can
be found that increases q->q_usage_counter.

Cc: Damien Le Moal <dlemoal@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-zoned.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index ca44b2d6727c..0f7666441fe1 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -583,6 +583,7 @@ static inline void blk_zone_wplug_bio_io_error(struct blk_zone_wplug *zwplug,
 	bio_clear_flag(bio, BIO_ZONE_WRITE_PLUGGING);
 	bio_io_error(bio);
 	disk_put_zone_wplug(zwplug);
+	/* Drop the reference taken by disk_zone_wplug_add_bio(() */
 	blk_queue_exit(q);
 }
 
@@ -894,10 +895,7 @@ void blk_zone_write_plug_init_request(struct request *req)
 			break;
 		}
 
-		/*
-		 * Drop the extra reference on the queue usage we got when
-		 * plugging the BIO and advance the write pointer offset.
-		 */
+		/* Drop the reference taken by disk_zone_wplug_add_bio(). */
 		blk_queue_exit(q);
 		zwplug->wp_offset += bio_sectors(bio);
 

Re: [PATCH 4/6] blk-zoned: Improve the queue reference count strategy documentation

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 4/6] blk-zoned: Improve the queue reference count strategy documentation

[Damien Le Moal]

On 2024/12/16 13:02, Bart Van Assche wrote:
> For the blk_queue_exit() calls, document where the corresponding code can
> be found that increases q->q_usage_counter.
> 
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 5/6] blk-zoned: Uninline functions that are not in the hot path

[Bart Van Assche]

Apply the convention that is followed elsewhere in the block layer: only
declare functions inline if these are in the hot path. This patch makes it
easier to debug the code in blk-zoned.c with trace_printk(). trace_printk()
only displays the function name correctly for functions that are not
inlined.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-zoned.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 0f7666441fe1..046903fc6030 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -575,8 +575,8 @@ static struct blk_zone_wplug *disk_get_and_lock_zone_wplug(struct gendisk *disk,
 	return zwplug;
 }
 
-static inline void blk_zone_wplug_bio_io_error(struct blk_zone_wplug *zwplug,
-					       struct bio *bio)
+static void blk_zone_wplug_bio_io_error(struct blk_zone_wplug *zwplug,
+					struct bio *bio)
 {
 	struct request_queue *q = zwplug->disk->queue;
 
@@ -1389,7 +1389,7 @@ void disk_free_zone_resources(struct gendisk *disk)
 	disk->nr_zones = 0;
 }
 
-static inline bool disk_need_zone_resources(struct gendisk *disk)
+static bool disk_need_zone_resources(struct gendisk *disk)
 {
 	/*
 	 * All mq zoned devices need zone resources so that the block layer

Re: [PATCH 5/6] blk-zoned: Uninline functions that are not in the hot path

[Christoph Hellwig]

On Mon, Dec 16, 2024 at 01:02:43PM -0800, Bart Van Assche wrote:
> Apply the convention that is followed elsewhere in the block layer: only
> declare functions inline if these are in the hot path. This patch makes it
> easier to debug the code in blk-zoned.c with trace_printk(). trace_printk()
> only displays the function name correctly for functions that are not
> inlined.

The other convention is to mark them as inline if they are so tiny
that force inlinining is guaranteed to generate smaller code.
disk_need_zone_resources seems a pretty clear case of that, while for
the blk_zone_wplug_bio_io_error the forced inlining is indeed a bit
questionable.  I'm still not sure what the sudden urge to change it is?

[PATCH 6/6] blk-zoned: Split queue_zone_wplugs_show()

[Bart Van Assche]

Reduce the indentation level of the code in queue_zone_wplugs_show() by
moving the body of the loop in that function into a new function.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-zoned.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 046903fc6030..775ad7016659 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -1775,37 +1775,41 @@ int blk_zone_issue_zeroout(struct block_device *bdev, sector_t sector,
 EXPORT_SYMBOL_GPL(blk_zone_issue_zeroout);
 
 #ifdef CONFIG_BLK_DEBUG_FS
+static void queue_zone_wplug_show(struct blk_zone_wplug *zwplug,
+				  struct seq_file *m)
+{
+	unsigned int zwp_wp_offset, zwp_flags;
+	unsigned int zwp_zone_no, zwp_ref;
+	unsigned int zwp_bio_list_size;
+	unsigned long flags;
+
+	spin_lock_irqsave(&zwplug->lock, flags);
+	zwp_zone_no = zwplug->zone_no;
+	zwp_flags = zwplug->flags;
+	zwp_ref = refcount_read(&zwplug->ref);
+	zwp_wp_offset = zwplug->wp_offset;
+	zwp_bio_list_size = bio_list_size(&zwplug->bio_list);
+	spin_unlock_irqrestore(&zwplug->lock, flags);
+
+	seq_printf(m, "%u 0x%x %u %u %u\n", zwp_zone_no, zwp_flags, zwp_ref,
+		   zwp_wp_offset, zwp_bio_list_size);
+}
 
 int queue_zone_wplugs_show(void *data, struct seq_file *m)
 {
 	struct request_queue *q = data;
 	struct gendisk *disk = q->disk;
 	struct blk_zone_wplug *zwplug;
-	unsigned int zwp_wp_offset, zwp_flags;
-	unsigned int zwp_zone_no, zwp_ref;
-	unsigned int zwp_bio_list_size, i;
-	unsigned long flags;
+	unsigned int i;
 
 	if (!disk->zone_wplugs_hash)
 		return 0;
 
 	rcu_read_lock();
-	for (i = 0; i < disk_zone_wplugs_hash_size(disk); i++) {
-		hlist_for_each_entry_rcu(zwplug,
-					 &disk->zone_wplugs_hash[i], node) {
-			spin_lock_irqsave(&zwplug->lock, flags);
-			zwp_zone_no = zwplug->zone_no;
-			zwp_flags = zwplug->flags;
-			zwp_ref = refcount_read(&zwplug->ref);
-			zwp_wp_offset = zwplug->wp_offset;
-			zwp_bio_list_size = bio_list_size(&zwplug->bio_list);
-			spin_unlock_irqrestore(&zwplug->lock, flags);
-
-			seq_printf(m, "%u 0x%x %u %u %u\n",
-				   zwp_zone_no, zwp_flags, zwp_ref,
-				   zwp_wp_offset, zwp_bio_list_size);
-		}
-	}
+	for (i = 0; i < disk_zone_wplugs_hash_size(disk); i++)
+		hlist_for_each_entry_rcu(zwplug, &disk->zone_wplugs_hash[i],
+					 node)
+			queue_zone_wplug_show(zwplug, m);
 	rcu_read_unlock();
 
 	return 0;

Re: [PATCH 6/6] blk-zoned: Split queue_zone_wplugs_show()

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 6/6] blk-zoned: Split queue_zone_wplugs_show()

[Damien Le Moal]

On 2024/12/16 13:02, Bart Van Assche wrote:
> Reduce the indentation level of the code in queue_zone_wplugs_show() by
> moving the body of the loop in that function into a new function.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research