diff mbox series

null_blk: Fix scheduling in atomic with zoned mode

Message ID 20201110073605.296624-1-damien.lemoal@wdc.com
State New
Headers show
Series null_blk: Fix scheduling in atomic with zoned mode | expand

Commit Message

Damien Le Moal Nov. 10, 2020, 7:36 a.m. UTC
commit e1777d099728a76a8f8090f89649aac961e7e530 upstream.

Commit aa1c09cb65e2 ("null_blk: Fix locking in zoned mode") changed
zone locking to using the potentially sleeping wait_on_bit_io()
function. This is acceptable when memory backing is enabled as the
device queue is in that case marked as blocking, but this triggers a
scheduling while in atomic context with memory backing disabled.

Fix this by relying solely on the device zone spinlock for zone
information protection without temporarily releasing this lock around
null_process_cmd() execution in null_zone_write(). This is OK to do
since when memory backing is disabled, command processing does not
block and the memory backing lock nullb->lock is unused. This solution
avoids the overhead of having to mark a zoned null_blk device queue as
blocking when memory backing is unused.

This patch also adds comments to the zone locking code to explain the
unusual locking scheme.

Fixes: aa1c09cb65e2 ("null_blk: Fix locking in zoned mode")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Cc: stable@vger.kernel.org
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/block/null_blk.h       |  1 +
 drivers/block/null_blk_zoned.c | 31 +++++++++++++++++++++++++------
 2 files changed, 26 insertions(+), 6 deletions(-)

Comments

Greg Kroah-Hartman Nov. 17, 2020, 10:29 a.m. UTC | #1
On Tue, Nov 10, 2020 at 04:36:05PM +0900, Damien Le Moal wrote:
> commit e1777d099728a76a8f8090f89649aac961e7e530 upstream.

> 

> Commit aa1c09cb65e2 ("null_blk: Fix locking in zoned mode") changed

> zone locking to using the potentially sleeping wait_on_bit_io()

> function. This is acceptable when memory backing is enabled as the

> device queue is in that case marked as blocking, but this triggers a

> scheduling while in atomic context with memory backing disabled.

> 

> Fix this by relying solely on the device zone spinlock for zone

> information protection without temporarily releasing this lock around

> null_process_cmd() execution in null_zone_write(). This is OK to do

> since when memory backing is disabled, command processing does not

> block and the memory backing lock nullb->lock is unused. This solution

> avoids the overhead of having to mark a zoned null_blk device queue as

> blocking when memory backing is unused.

> 

> This patch also adds comments to the zone locking code to explain the

> unusual locking scheme.

> 

> Fixes: aa1c09cb65e2 ("null_blk: Fix locking in zoned mode")

> Reported-by: kernel test robot <lkp@intel.com>

> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>

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

> Cc: stable@vger.kernel.org

> Signed-off-by: Jens Axboe <axboe@kernel.dk>

> ---

>  drivers/block/null_blk.h       |  1 +

>  drivers/block/null_blk_zoned.c | 31 +++++++++++++++++++++++++------

>  2 files changed, 26 insertions(+), 6 deletions(-)


Now queued up, thanks.

greg k-h
diff mbox series

Patch

diff --git a/drivers/block/null_blk.h b/drivers/block/null_blk.h
index 206309ecc7e4..7562cd6cd681 100644
--- a/drivers/block/null_blk.h
+++ b/drivers/block/null_blk.h
@@ -44,6 +44,7 @@  struct nullb_device {
 	unsigned int nr_zones;
 	struct blk_zone *zones;
 	sector_t zone_size_sects;
+	spinlock_t zone_lock;
 	unsigned long *zone_locks;
 
 	unsigned long size; /* device size in MB */
diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
index 495713d6c989..d9102327357c 100644
--- a/drivers/block/null_blk_zoned.c
+++ b/drivers/block/null_blk_zoned.c
@@ -46,10 +46,20 @@  int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q)
 	if (!dev->zones)
 		return -ENOMEM;
 
-	dev->zone_locks = bitmap_zalloc(dev->nr_zones, GFP_KERNEL);
-	if (!dev->zone_locks) {
-		kvfree(dev->zones);
-		return -ENOMEM;
+	/*
+	 * With memory backing, the zone_lock spinlock needs to be temporarily
+	 * released to avoid scheduling in atomic context. To guarantee zone
+	 * information protection, use a bitmap to lock zones with
+	 * wait_on_bit_lock_io(). Sleeping on the lock is OK as memory backing
+	 * implies that the queue is marked with BLK_MQ_F_BLOCKING.
+	 */
+	spin_lock_init(&dev->zone_lock);
+	if (dev->memory_backed) {
+		dev->zone_locks = bitmap_zalloc(dev->nr_zones, GFP_KERNEL);
+		if (!dev->zone_locks) {
+			kvfree(dev->zones);
+			return -ENOMEM;
+		}
 	}
 
 	if (dev->zone_nr_conv >= dev->nr_zones) {
@@ -118,12 +128,16 @@  void null_free_zoned_dev(struct nullb_device *dev)
 
 static inline void null_lock_zone(struct nullb_device *dev, unsigned int zno)
 {
-	wait_on_bit_lock_io(dev->zone_locks, zno, TASK_UNINTERRUPTIBLE);
+	if (dev->memory_backed)
+		wait_on_bit_lock_io(dev->zone_locks, zno, TASK_UNINTERRUPTIBLE);
+	spin_lock_irq(&dev->zone_lock);
 }
 
 static inline void null_unlock_zone(struct nullb_device *dev, unsigned int zno)
 {
-	clear_and_wake_up_bit(zno, dev->zone_locks);
+	spin_unlock_irq(&dev->zone_lock);
+	if (dev->memory_backed)
+		clear_and_wake_up_bit(zno, dev->zone_locks);
 }
 
 int null_report_zones(struct gendisk *disk, sector_t sector,
@@ -233,7 +247,12 @@  static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,
 		if (zone->cond != BLK_ZONE_COND_EXP_OPEN)
 			zone->cond = BLK_ZONE_COND_IMP_OPEN;
 
+		if (dev->memory_backed)
+			spin_unlock_irq(&dev->zone_lock);
 		ret = null_process_cmd(cmd, REQ_OP_WRITE, sector, nr_sectors);
+		if (dev->memory_backed)
+			spin_lock_irq(&dev->zone_lock);
+
 		if (ret != BLK_STS_OK)
 			break;