| Message ID | 20210128063619.570177-1-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: fix bd_size_lock use | expand |
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.
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: >> 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..
On Thu, Jan 28, 2021 at 07:39:21AM -0700, Jens Axboe wrote: > On 1/28/21 7:37 AM, Christoph Hellwig wrote: > > 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.. > > 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: >> On 1/28/21 7:37 AM, Christoph Hellwig wrote: >>> 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.. >> >> 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 ?
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)
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 <damien.lemoal@wdc.com> --- block/genhd.c | 5 +++-- block/partitions/core.c | 6 ++++-- 2 files changed, 7 insertions(+), 4 deletions(-)