[PATCH] block: fix bd_size_lock use

Some block device drivers, e.g. the skd driver, call set_capacity() with
IRQ disabled. This results in lockdep ito complain about inconsistent
lock states ("inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage")
because set_capacity takes a block device bd_size_lock using the
functions spin_lock() and spin_unlock(). Ensure a consistent locking
state by replacing these calls with spin_lock_irqsave() and
spin_lock_irqrestore(). The same applies to bdev_set_nr_sectors().
With this fix, all lockdep complaints are resolved.

Signed-off-by: Damien Le Moal <redacted>
---
 block/genhd.c           | 5 +++--
 block/partitions/core.c | 6 ++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index d3ef29fbc536..f795bd56012f 100644
--- a/block/genhd.c
+++ b/block/genhd.c

@@ -45,10 +45,11 @@ static void disk_release_events(struct gendisk *disk);
 void set_capacity(struct gendisk *disk, sector_t sectors)
 {
 	struct block_device *bdev = disk->part0;
+	unsigned long flags;
 
-	spin_lock(&bdev->bd_size_lock);
+	spin_lock_irqsave(&bdev->bd_size_lock, flags);
 	i_size_write(bdev->bd_inode, (loff_t)sectors << SECTOR_SHIFT);
-	spin_unlock(&bdev->bd_size_lock);
+	spin_unlock_irqrestore(&bdev->bd_size_lock, flags);
 }
 EXPORT_SYMBOL(set_capacity);
 

diff --git a/block/partitions/core.c b/block/partitions/core.c
index d6094203116a..301518bdc403 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c

@@ -88,9 +88,11 @@ static int (*check_part[])(struct parsed_partitions *) = {
 
 static void bdev_set_nr_sectors(struct block_device *bdev, sector_t sectors)
 {
-	spin_lock(&bdev->bd_size_lock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bdev->bd_size_lock, flags);
 	i_size_write(bdev->bd_inode, (loff_t)sectors << SECTOR_SHIFT);
-	spin_unlock(&bdev->bd_size_lock);
+	spin_unlock_irqrestore(&bdev->bd_size_lock, flags);
 }
 
 static struct parsed_partitions *allocate_partitions(struct gendisk *hd)

-- 
2.29.2

On 1/27/21 11:36 PM, Damien Le Moal wrote:

Some block device drivers, e.g. the skd driver, call set_capacity() with
IRQ disabled. This results in lockdep ito complain about inconsistent
lock states ("inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage")
because set_capacity takes a block device bd_size_lock using the
functions spin_lock() and spin_unlock(). Ensure a consistent locking
state by replacing these calls with spin_lock_irqsave() and
spin_lock_irqrestore(). The same applies to bdev_set_nr_sectors().
With this fix, all lockdep complaints are resolved.

Applied, thanks.

-- 
Jens Axboe

On Thu, Jan 28, 2021 at 03:36:19PM +0900, Damien Le Moal wrote:

Some block device drivers, e.g. the skd driver, call set_capacity() with
IRQ disabled. This results in lockdep ito complain about inconsistent
lock states ("inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage")
because set_capacity takes a block device bd_size_lock using the
functions spin_lock() and spin_unlock(). Ensure a consistent locking
state by replacing these calls with spin_lock_irqsave() and
spin_lock_irqrestore(). The same applies to bdev_set_nr_sectors().
With this fix, all lockdep complaints are resolved.

I'd much rather fix the driver to not call set_capacity with irqs
disabled..

On 1/28/21 7:37 AM, Christoph Hellwig wrote:

On Thu, Jan 28, 2021 at 03:36:19PM +0900, Damien Le Moal wrote:

quoted

Some block device drivers, e.g. the skd driver, call set_capacity() with
IRQ disabled. This results in lockdep ito complain about inconsistent
lock states ("inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage")
because set_capacity takes a block device bd_size_lock using the
functions spin_lock() and spin_unlock(). Ensure a consistent locking
state by replacing these calls with spin_lock_irqsave() and
spin_lock_irqrestore(). The same applies to bdev_set_nr_sectors().
With this fix, all lockdep complaints are resolved.

I'd much rather fix the driver to not call set_capacity with irqs
disabled..

Agree, but that might be a bit beyond 5.10 at this point..

-- 
Jens Axboe

On Thu, Jan 28, 2021 at 07:39:21AM -0700, Jens Axboe wrote:

On 1/28/21 7:37 AM, Christoph Hellwig wrote:

quoted

On Thu, Jan 28, 2021 at 03:36:19PM +0900, Damien Le Moal wrote:

quoted

Some block device drivers, e.g. the skd driver, call set_capacity() with
IRQ disabled. This results in lockdep ito complain about inconsistent
lock states ("inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage")
because set_capacity takes a block device bd_size_lock using the
functions spin_lock() and spin_unlock(). Ensure a consistent locking
state by replacing these calls with spin_lock_irqsave() and
spin_lock_irqrestore(). The same applies to bdev_set_nr_sectors().
With this fix, all lockdep complaints are resolved.

I'd much rather fix the driver to not call set_capacity with irqs
disabled..

Agree, but that might be a bit beyond 5.10 at this point..

True.

On 2021/01/28 23:43, Christoph Hellwig wrote:

On Thu, Jan 28, 2021 at 07:39:21AM -0700, Jens Axboe wrote:

quoted

On 1/28/21 7:37 AM, Christoph Hellwig wrote:

quoted

On Thu, Jan 28, 2021 at 03:36:19PM +0900, Damien Le Moal wrote:

quoted

Some block device drivers, e.g. the skd driver, call set_capacity() with
IRQ disabled. This results in lockdep ito complain about inconsistent
lock states ("inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage")
because set_capacity takes a block device bd_size_lock using the
functions spin_lock() and spin_unlock(). Ensure a consistent locking
state by replacing these calls with spin_lock_irqsave() and
spin_lock_irqrestore(). The same applies to bdev_set_nr_sectors().
With this fix, all lockdep complaints are resolved.

I'd much rather fix the driver to not call set_capacity with irqs
disabled..

Agree, but that might be a bit beyond 5.10 at this point..

True.

Agree, it was my initial intent to fix the driver itself to fix this problem.
However, the entire completion path code, where the set_capacity() call is, is
executed under a single spin_lock with IRQ disabled. That is pretty horrible and
will need major surgery to fix.

I can work on that for 5.12 if required, but I would rather leave it like this
and deprecate this driver so that we can remove it in a couple of releases or
so. The STEC cards are not sold anymore since many years ago and are not even
supported by the vendor anymore.

Should I start fixing this driver or go with deprecating it ?

-- 
Damien Le Moal
Western Digital Research