[PATCH 0/3] Hold the zoned write plug lock less long

[PATCH 0/3] Hold the zoned write plug lock less long

[Bart Van Assche]

Hi Jens,

The zone write plug lock is taken in the submission and in the completion path.
Hence, it is important that this lock is held no longer than needed. This patch
series reduces how long the zone write plug lock is held. Please consider this
patch series for the next merge window.

Thanks,

Bart.

Bart Van Assche (3):
  blk-zoned: Move locking into disk_zone_wplug_set_wp_offset()
  blk-zoned: Move locking into disk_zone_wplug_abort()
  blk-zoned: Do not lock zwplug->lock recursively

 block/blk-zoned.c | 56 +++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 26 deletions(-)

[PATCH 1/3] blk-zoned: Move locking into disk_zone_wplug_set_wp_offset()

[Bart Van Assche]

Instead of holding zwplug->lock around disk_zone_wplug_set_wp_offset() calls,
move zwplug->lock locking into disk_zone_wplug_set_wp_offset(). Prepare for
reducing the amount of code protected by zwplug->lock. No functionality has
been changed.

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

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 8f15d1aa6eb8..75b01068b877 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -624,14 +624,19 @@ static void disk_zone_wplug_abort(struct blk_zone_wplug *zwplug)
  */
 static void disk_zone_wplug_set_wp_offset(struct gendisk *disk,
 					  struct blk_zone_wplug *zwplug,
-					  unsigned int wp_offset)
+					  unsigned int wp_offset,
+					  bool only_if_update_needed)
 {
-	lockdep_assert_held(&zwplug->lock);
-
-	/* Update the zone write pointer and abort all plugged BIOs. */
-	zwplug->flags &= ~BLK_ZONE_WPLUG_NEED_WP_UPDATE;
-	zwplug->wp_offset = wp_offset;
-	disk_zone_wplug_abort(zwplug);
+	scoped_guard(spinlock_irqsave, &zwplug->lock) {
+		if (only_if_update_needed &&
+		    !(zwplug->flags & BLK_ZONE_WPLUG_NEED_WP_UPDATE))
+			return;
+
+		/* Update the zone write pointer and abort all plugged BIOs. */
+		zwplug->flags &= ~BLK_ZONE_WPLUG_NEED_WP_UPDATE;
+		zwplug->wp_offset = wp_offset;
+		disk_zone_wplug_abort(zwplug);
+	}
 
 	/*
 	 * The zone write plug now has no BIO plugged: remove it from the
@@ -669,18 +674,13 @@ static void disk_zone_wplug_sync_wp_offset(struct gendisk *disk,
 					   struct blk_zone *zone)
 {
 	struct blk_zone_wplug *zwplug;
-	unsigned long flags;
 
 	zwplug = disk_get_zone_wplug(disk, zone->start);
 	if (!zwplug)
 		return;
 
-	spin_lock_irqsave(&zwplug->lock, flags);
-	if (zwplug->flags & BLK_ZONE_WPLUG_NEED_WP_UPDATE)
-		disk_zone_wplug_set_wp_offset(disk, zwplug,
-					      blk_zone_wp_offset(zone));
-	spin_unlock_irqrestore(&zwplug->lock, flags);
-
+	disk_zone_wplug_set_wp_offset(disk, zwplug, blk_zone_wp_offset(zone),
+				      true);
 	disk_put_zone_wplug(zwplug);
 }
 
@@ -700,7 +700,6 @@ static bool blk_zone_wplug_handle_reset_or_finish(struct bio *bio,
 	struct gendisk *disk = bio->bi_bdev->bd_disk;
 	sector_t sector = bio->bi_iter.bi_sector;
 	struct blk_zone_wplug *zwplug;
-	unsigned long flags;
 
 	/* Conventional zones cannot be reset nor finished. */
 	if (!bdev_zone_is_seq(bio->bi_bdev, sector)) {
@@ -726,9 +725,7 @@ static bool blk_zone_wplug_handle_reset_or_finish(struct bio *bio,
 	 */
 	zwplug = disk_get_zone_wplug(disk, sector);
 	if (zwplug) {
-		spin_lock_irqsave(&zwplug->lock, flags);
-		disk_zone_wplug_set_wp_offset(disk, zwplug, wp_offset);
-		spin_unlock_irqrestore(&zwplug->lock, flags);
+		disk_zone_wplug_set_wp_offset(disk, zwplug, wp_offset, false);
 		disk_put_zone_wplug(zwplug);
 	}
 
@@ -739,7 +736,6 @@ static bool blk_zone_wplug_handle_reset_all(struct bio *bio)
 {
 	struct gendisk *disk = bio->bi_bdev->bd_disk;
 	struct blk_zone_wplug *zwplug;
-	unsigned long flags;
 	sector_t sector;
 
 	/*
@@ -751,9 +747,7 @@ static bool blk_zone_wplug_handle_reset_all(struct bio *bio)
 	     sector += disk->queue->limits.chunk_sectors) {
 		zwplug = disk_get_zone_wplug(disk, sector);
 		if (zwplug) {
-			spin_lock_irqsave(&zwplug->lock, flags);
-			disk_zone_wplug_set_wp_offset(disk, zwplug, 0);
-			spin_unlock_irqrestore(&zwplug->lock, flags);
+			disk_zone_wplug_set_wp_offset(disk, zwplug, 0, false);
 			disk_put_zone_wplug(zwplug);
 		}
 	}

[PATCH 2/3] blk-zoned: Move locking into disk_zone_wplug_abort()

[Bart Van Assche]

Instead of holding zwplug->lock around disk_zone_wplug_abort() calls, make
disk_zone_wplug_abort() obtain zwplug->lock. Prepare for reducing the amount
of code protected by zwplug->lock. No functionality has been changed.

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

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 75b01068b877..ce5604c92fea 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -607,13 +607,20 @@ static void disk_zone_wplug_abort(struct blk_zone_wplug *zwplug)
 {
 	struct bio *bio;
 
-	if (bio_list_empty(&zwplug->bio_list))
-		return;
+	scoped_guard(spinlock_irqsave, &zwplug->lock)
+		if (bio_list_empty(&zwplug->bio_list))
+			return;
 
 	pr_warn_ratelimited("%s: zone %u: Aborting plugged BIOs\n",
 			    zwplug->disk->disk_name, zwplug->zone_no);
-	while ((bio = bio_list_pop(&zwplug->bio_list)))
-		blk_zone_wplug_bio_io_error(zwplug, bio);
+	for (;;) {
+		scoped_guard(spinlock_irqsave, &zwplug->lock) {
+			bio = bio_list_pop(&zwplug->bio_list);
+			if (!bio)
+				break;
+			blk_zone_wplug_bio_io_error(zwplug, bio);
+		}
+	}
 }
 
 /*
@@ -635,9 +642,10 @@ static void disk_zone_wplug_set_wp_offset(struct gendisk *disk,
 		/* Update the zone write pointer and abort all plugged BIOs. */
 		zwplug->flags &= ~BLK_ZONE_WPLUG_NEED_WP_UPDATE;
 		zwplug->wp_offset = wp_offset;
-		disk_zone_wplug_abort(zwplug);
 	}
 
+	disk_zone_wplug_abort(zwplug);
+
 	/*
 	 * The zone write plug now has no BIO plugged: remove it from the
 	 * hash table so that it cannot be seen. The plug will be freed
@@ -1086,7 +1094,9 @@ static void blk_zone_wplug_handle_native_zone_append(struct bio *bio)
 	if (!bio_list_empty(&zwplug->bio_list)) {
 		pr_warn_ratelimited("%s: zone %u: Invalid mix of zone append and regular writes\n",
 				    disk->disk_name, zwplug->zone_no);
+		spin_unlock_irqrestore(&zwplug->lock, flags);
 		disk_zone_wplug_abort(zwplug);
+		spin_lock_irqsave(&zwplug->lock, flags);
 	}
 	disk_remove_zone_wplug(disk, zwplug);
 	spin_unlock_irqrestore(&zwplug->lock, flags);

[PATCH 3/3] blk-zoned: Do not lock zwplug->lock recursively

[Bart Van Assche]

If zoned block devices are stacked and if a lower driver reports an I/O
error, this triggers nested locking of spinlocks. Rework the zoned block
device code such that this doesn't happen anymore. This patch fixes the
following kernel warning:

WARNING: possible recursive locking detected
--------------------------------------------
kworker/6:1H/203 is trying to acquire lock:
ffffff8881697130 (&zwplug->lock){-...}-{2:2}, at: blk_zone_write_plug_bio_endio+0x144/0x290

but task is already holding lock:
ffffff884e99d930 (&zwplug->lock){-...}-{2:2}, at: blk_zone_wplug_bio_work+0x30/0x250

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&zwplug->lock);
  lock(&zwplug->lock);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

3 locks held by kworker/6:1H/203:
 #0: ffffff88128ed358 ((wq_completion)sde_zwplugs){+.+.}-{0:0}, at: process_one_work+0x1bc/0x65c
 #1: ffffffc088343d70 ((work_completion)(&zwplug->bio_work)){+.+.}-{0:0}, at: process_one_work+0x1e4/0x65c
 #2: ffffff884e99d930 (&zwplug->lock){-...}-{2:2}, at: blk_zone_wplug_bio_work+0x30/0x250

stack backtrace:
Workqueue: sde_zwplugs blk_zone_wplug_bio_work
Call trace:
 dump_backtrace+0xfc/0x17c
 show_stack+0x18/0x28
 dump_stack_lvl+0x40/0xa0
 dump_stack+0x18/0x24
 print_deadlock_bug+0x38c/0x398
 __lock_acquire+0x13e8/0x2e1c
 lock_acquire+0x134/0x2b4
 _raw_spin_lock_irqsave+0x5c/0x80
 blk_zone_write_plug_bio_endio+0x144/0x290
 bio_endio+0x9c/0x240
 __dm_io_complete+0x210/0x27c
 clone_endio+0xe8/0x214
 bio_endio+0x218/0x240
 blk_crypto_fallback_encrypt_endio+0x78/0x94
 bio_endio+0x218/0x240
 blk_zone_wplug_bio_work+0xa8/0x250
 process_one_work+0x26c/0x65c
 worker_thread+0x33c/0x498
 kthread+0x110/0x134
 ret_from_fork+0x10/0x20

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

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index ce5604c92fea..4bff76a06204 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -614,12 +614,11 @@ static void disk_zone_wplug_abort(struct blk_zone_wplug *zwplug)
 	pr_warn_ratelimited("%s: zone %u: Aborting plugged BIOs\n",
 			    zwplug->disk->disk_name, zwplug->zone_no);
 	for (;;) {
-		scoped_guard(spinlock_irqsave, &zwplug->lock) {
+		scoped_guard(spinlock_irqsave, &zwplug->lock)
 			bio = bio_list_pop(&zwplug->bio_list);
-			if (!bio)
-				break;
-			blk_zone_wplug_bio_io_error(zwplug, bio);
-		}
+		if (!bio)
+			break;
+		blk_zone_wplug_bio_io_error(zwplug, bio);
 	}
 }
 
@@ -1236,8 +1235,9 @@ void blk_zone_write_plug_bio_endio(struct bio *bio)
 	 * needing a write pointer update.
 	 */
 	if (bio->bi_status != BLK_STS_OK) {
-		spin_lock_irqsave(&zwplug->lock, flags);
 		disk_zone_wplug_abort(zwplug);
+
+		spin_lock_irqsave(&zwplug->lock, flags);
 		zwplug->flags |= BLK_ZONE_WPLUG_NEED_WP_UPDATE;
 		spin_unlock_irqrestore(&zwplug->lock, flags);
 	}
@@ -1288,13 +1288,12 @@ static void blk_zone_wplug_bio_work(struct work_struct *work)
 	unsigned long flags;
 	struct bio *bio;
 
+again:
 	/*
 	 * Submit the next plugged BIO. If we do not have any, clear
 	 * the plugged flag.
 	 */
 	spin_lock_irqsave(&zwplug->lock, flags);
-
-again:
 	bio = bio_list_pop(&zwplug->bio_list);
 	if (!bio) {
 		zwplug->flags &= ~BLK_ZONE_WPLUG_PLUGGED;
@@ -1303,6 +1302,7 @@ static void blk_zone_wplug_bio_work(struct work_struct *work)
 	}
 
 	if (!blk_zone_wplug_prepare_bio(zwplug, bio)) {
+		spin_unlock_irqrestore(&zwplug->lock, flags);
 		blk_zone_wplug_bio_io_error(zwplug, bio);
 		goto again;
 	}

Re: [PATCH 1/3] blk-zoned: Move locking into disk_zone_wplug_set_wp_offset()

[Christoph Hellwig]

On Tue, May 20, 2025 at 05:06:24PM -0700, Bart Van Assche wrote:
> Instead of holding zwplug->lock around disk_zone_wplug_set_wp_offset() calls,
> move zwplug->lock locking into disk_zone_wplug_set_wp_offset(). Prepare for
> reducing the amount of code protected by zwplug->lock. No functionality has
> been changed.

Please wrap your commit logs after 73 characters.

> +					  bool only_if_update_needed)

That would probably be a bit better with a bool force, that is inverted.

>  {
> -	lockdep_assert_held(&zwplug->lock);
> -
> -	/* Update the zone write pointer and abort all plugged BIOs. */
> -	zwplug->flags &= ~BLK_ZONE_WPLUG_NEED_WP_UPDATE;
> -	zwplug->wp_offset = wp_offset;
> -	disk_zone_wplug_abort(zwplug);
> +	scoped_guard(spinlock_irqsave, &zwplug->lock) {

And don't randomly mess up the code to new locking methods.  Please
stick to the lock/unlock helpers here instead of starting a grand
reformatting.

Re: [PATCH 2/3] blk-zoned: Move locking into disk_zone_wplug_abort()

[Christoph Hellwig]

On Tue, May 20, 2025 at 05:06:25PM -0700, Bart Van Assche wrote:
> Instead of holding zwplug->lock around disk_zone_wplug_abort() calls, make
> disk_zone_wplug_abort() obtain zwplug->lock. Prepare for reducing the amount
> of code protected by zwplug->lock. No functionality has been changed.

Besides the comments from the last patch that apply here as well,
this does change critical sections withoyut any explanation of why
that is desirable and an explanation of why it is safe.

Re: [PATCH 3/3] blk-zoned: Do not lock zwplug->lock recursively

[Christoph Hellwig]

On Tue, May 20, 2025 at 05:06:26PM -0700, Bart Van Assche wrote:
> If zoned block devices are stacked and if a lower driver reports an I/O
> error, this triggers nested locking of spinlocks. Rework the zoned block
> device code such that this doesn't happen anymore. This patch fixes the
> following kernel warning:

Please explain the issue instead of just dumping lockdep output that
needs a fair amount of effort to decipher.  From a quick looks this
seems to be about the problem that the BIO_ZONE_WRITE_PLUGGING is
interpreted by the lower layer while it is owned by the upper layer,
which we've discussed just yesterday.

We need to fix that instead of working around it.

Also please help creating a reproducer using null_blk or scsi_debug
so that we can verify the fixes and have a reproducer in blktests to
avoid reintroducing issues here.