LOCKDEP warning due to ext_devt_lock

LOCKDEP warning due to ext_devt_lock

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1841 bytes --]


Hi Keith,
 I'm getting lockdep warning around use of ext_devt_lock.

[11428.266019] inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.

 As you say in
    2da78092dda1 ("block: Fix dev_t minor allocation lifetime")
ext_devt_lock can be taken from call_rcu's soft-irq.
I think that means it should always use the _irq verion of spinlock.

I've been using the following patch and the LOCKDEP message are gone.

If you agree, could you forward it or something similar upstream?
Thanks,
NeilBrown


Signed-off-by: NeilBrown <neilb@suse.de>
diff --git a/block/genhd.c b/block/genhd.c
index 666e11b83983..d1338934562e 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -422,9 +422,9 @@ int blk_alloc_devt(struct hd_struct *part, dev_t *devt)
 	/* allocate ext devt */
 	idr_preload(GFP_KERNEL);
 
-	spin_lock(&ext_devt_lock);
+	spin_lock_irq(&ext_devt_lock);
 	idx = idr_alloc(&ext_devt_idr, part, 0, NR_EXT_DEVT, GFP_NOWAIT);
-	spin_unlock(&ext_devt_lock);
+	spin_unlock_irq(&ext_devt_lock);
 
 	idr_preload_end();
 	if (idx < 0)
@@ -449,9 +449,9 @@ void blk_free_devt(dev_t devt)
 		return;
 
 	if (MAJOR(devt) == BLOCK_EXT_MAJOR) {
-		spin_lock(&ext_devt_lock);
+		spin_lock_irq(&ext_devt_lock);
 		idr_remove(&ext_devt_idr, blk_mangle_minor(MINOR(devt)));
-		spin_unlock(&ext_devt_lock);
+		spin_unlock_irq(&ext_devt_lock);
 	}
 }
 
@@ -690,13 +690,13 @@ struct gendisk *get_gendisk(dev_t devt, int *partno)
 	} else {
 		struct hd_struct *part;
 
-		spin_lock(&ext_devt_lock);
+		spin_lock_irq(&ext_devt_lock);
 		part = idr_find(&ext_devt_idr, blk_mangle_minor(MINOR(devt)));
 		if (part && get_disk(part_to_disk(part))) {
 			*partno = part->partno;
 			disk = part_to_disk(part);
 		}
-		spin_unlock(&ext_devt_lock);
+		spin_unlock_irq(&ext_devt_lock);
 	}
 
 	return disk;

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: LOCKDEP warning due to ext_devt_lock

[Jens Axboe]

On 05/27/2015 08:32 PM, NeilBrown wrote:
>
> Hi Keith,
>   I'm getting lockdep warning around use of ext_devt_lock.
>
> [11428.266019] inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
>
>   As you say in
>      2da78092dda1 ("block: Fix dev_t minor allocation lifetime")
> ext_devt_lock can be taken from call_rcu's soft-irq.
> I think that means it should always use the _irq verion of spinlock.

Wouldn't spin_lock_bh() and friends suffice then?

-- 
Jens Axboe

Re: LOCKDEP warning due to ext_devt_lock

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

On Wed, 27 May 2015 21:32:21 -0600 Jens Axboe <axboe@fb.com> wrote:

> On 05/27/2015 08:32 PM, NeilBrown wrote:
> >
> > Hi Keith,
> >   I'm getting lockdep warning around use of ext_devt_lock.
> >
> > [11428.266019] inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
> >
> >   As you say in
> >      2da78092dda1 ("block: Fix dev_t minor allocation lifetime")
> > ext_devt_lock can be taken from call_rcu's soft-irq.
> > I think that means it should always use the _irq verion of spinlock.
> 
> Wouldn't spin_lock_bh() and friends suffice then?
> 

Quite possibly.  I've never actually used those so I don't tend to think
about them.  But only reflection I think they would be exactly right.
I'll modify my patch for future testing.

BTW, did you get my:

   [PATCH v2] block: discard bdi_unregister() in favour of bdi_destroy()

??  I haven't seen it appear in your 'block' tree.

Thanks,
NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: LOCKDEP warning due to ext_devt_lock

[Jens Axboe]

On 05/27/2015 09:45 PM, NeilBrown wrote:
> On Wed, 27 May 2015 21:32:21 -0600 Jens Axboe <axboe@fb.com> wrote:
>
>> On 05/27/2015 08:32 PM, NeilBrown wrote:
>>>
>>> Hi Keith,
>>>    I'm getting lockdep warning around use of ext_devt_lock.
>>>
>>> [11428.266019] inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
>>>
>>>    As you say in
>>>       2da78092dda1 ("block: Fix dev_t minor allocation lifetime")
>>> ext_devt_lock can be taken from call_rcu's soft-irq.
>>> I think that means it should always use the _irq verion of spinlock.
>>
>> Wouldn't spin_lock_bh() and friends suffice then?
>>
>
> Quite possibly.  I've never actually used those so I don't tend to think
> about them.  But only reflection I think they would be exactly right.
> I'll modify my patch for future testing.
>
> BTW, did you get my:
>
>     [PATCH v2] block: discard bdi_unregister() in favour of bdi_destroy()
>
> ??  I haven't seen it appear in your 'block' tree.

I missed that, thanks for the reminder. I've queued it up for this series.

-- 
Jens Axboe