On Fri, Nov 06, 2020 at 08:01:41PM +0900, Damien Le Moal wrote:
> 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.
>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Fixes: aa1c09cb65e2 ("null_blk: Fix locking in zoned mode")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxx>
> ---
> drivers/block/null_blk.h | 2 +-
> drivers/block/null_blk_zoned.c | 47 ++++++++++++++++++++++------------
> 2 files changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/block/null_blk.h b/drivers/block/null_blk.h
> index cfd00ad40355..c24d9b5ad81a 100644
> --- a/drivers/block/null_blk.h
> +++ b/drivers/block/null_blk.h
> @@ -47,7 +47,7 @@ struct nullb_device {
> unsigned int nr_zones_closed;
> struct blk_zone *zones;
> sector_t zone_size_sects;
> - spinlock_t zone_dev_lock;
> + 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 8775acbb4f8f..beb34b4f76b0 100644
> --- a/drivers/block/null_blk_zoned.c
> +++ b/drivers/block/null_blk_zoned.c
> @@ -46,11 +46,20 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q)
> if (!dev->zones)
> return -ENOMEM;
>
> - spin_lock_init(&dev->zone_dev_lock);
> - 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) {
> @@ -137,12 +146,17 @@ 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,
> @@ -322,7 +336,6 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,
> return null_process_cmd(cmd, REQ_OP_WRITE, sector, nr_sectors);
>
> null_lock_zone(dev, zno);
> - spin_lock(&dev->zone_dev_lock);
>
> switch (zone->cond) {
> case BLK_ZONE_COND_FULL:
> @@ -375,9 +388,17 @@ 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;
>
> - spin_unlock(&dev->zone_dev_lock);
> + /*
> + * Memory backing allocation may sleep: release the zone_lock spinlock
> + * to avoid scheduling in atomic context. Zone operation atomicity is
> + * still guaranteed through the zone_locks bitmap.
> + */
> + if (dev->memory_backed)
> + spin_unlock_irq(&dev->zone_lock);
> ret = null_process_cmd(cmd, REQ_OP_WRITE, sector, nr_sectors);
> - spin_lock(&dev->zone_dev_lock);
> + if (dev->memory_backed)
> + spin_lock_irq(&dev->zone_lock);
Do we actually need to take zone_lock at all for the memory_backed
case? Should the !memory_backed just use a per-zone spinlock?
