Re: [ata_scsi_offline_dev] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238

Re: [ata_scsi_offline_dev] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238

[Linus Torvalds]

On Mon, Nov 6, 2017 at 2:53 PM, Fengguang Wu <fengguang.wu@intel.com> wrote:
>
> The same dmesg happen to contain another libata related bug. Attached again.
> It's rare and in the error handling path, so unlikely a new regression.
>
> [   49.608280] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
> [   49.647821]  mutex_lock+0x20/0x50
> [   49.651443]  kernfs_find_and_get_ns+0x23/0x60
> [   49.656104]  sysfs_notify+0x77/0x90
> [   49.659900]  scsi_device_set_state+0x63/0x150
> [   49.664559]  ata_scsi_offline_dev+0x1c/0x30 [libata]
> [   49.669817]  ata_eh_detach_dev+0x3b/0xb0 [libata]

ata_eh_detach_dev() does

        spin_lock_irqsave(ap->lock, flags);

and then does

        if (ata_scsi_offline_dev(dev)) {
                dev->flags |= ATA_DFLAG_DETACHED;
                ap->pflags |= ATA_PFLAG_SCSI_HOTPLUG;
        }

inside that spinlock. And this code is not new - it has done it since
2006 or so.

But it does seem to be a new regression in 4.14, caused by commit
8a97712e5314 ("scsi: make 'state' device attribute pollable"), because
that's what added the sysfs_notify() call to scsi_device_set_state(),
which made that spinlock be a problem.

That commit came in through the SCSI merge this merge window, and it
seems to still revert cleanly.

So I do suspect that by now we should just revert that commit. It's
not clear why that state attribute should be pollable, and the new
code is clearly very much buggy.

Hannes, Martin?

                Linus

Re: [ata_scsi_offline_dev] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238

[Tejun Heo]

Hello,

On Mon, Nov 06, 2017 at 03:12:31PM -0800, Linus Torvalds wrote:
> But it does seem to be a new regression in 4.14, caused by commit
> 8a97712e5314 ("scsi: make 'state' device attribute pollable"), because
> that's what added the sysfs_notify() call to scsi_device_set_state(),
> which made that spinlock be a problem.

Yeah, pinged Hannes about it a couple of weeks ago.

> That commit came in through the SCSI merge this merge window, and it
> seems to still revert cleanly.
> 
> So I do suspect that by now we should just revert that commit. It's
> not clear why that state attribute should be pollable, and the new
> code is clearly very much buggy.

I think reverting is the right thing to do right now.  If necessary,
we can make kernfs_notify() safe to be called from atomic contexts.

Thanks.

-- 
tejun

Re: [ata_scsi_offline_dev] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238

[Martin K. Petersen]

Linus,

> But it does seem to be a new regression in 4.14, caused by commit
> 8a97712e5314 ("scsi: make 'state' device attribute pollable"), because
> that's what added the sysfs_notify() call to scsi_device_set_state(),
> which made that spinlock be a problem.
>
> That commit came in through the SCSI merge this merge window, and it
> seems to still revert cleanly.
>
> So I do suspect that by now we should just revert that commit. It's
> not clear why that state attribute should be pollable, and the new
> code is clearly very much buggy.

Yeah, let's revert it for now.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [ata_scsi_offline_dev] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238

[Hannes Reinecke]

On 11/07/2017 12:12 AM, Linus Torvalds wrote:
> On Mon, Nov 6, 2017 at 2:53 PM, Fengguang Wu <fengguang.wu@intel.com> wrote:
>>
>> The same dmesg happen to contain another libata related bug. Attached again.
>> It's rare and in the error handling path, so unlikely a new regression.
>>
>> [   49.608280] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
>> [   49.647821]  mutex_lock+0x20/0x50
>> [   49.651443]  kernfs_find_and_get_ns+0x23/0x60
>> [   49.656104]  sysfs_notify+0x77/0x90
>> [   49.659900]  scsi_device_set_state+0x63/0x150
>> [   49.664559]  ata_scsi_offline_dev+0x1c/0x30 [libata]
>> [   49.669817]  ata_eh_detach_dev+0x3b/0xb0 [libata]
> 
> ata_eh_detach_dev() does
> 
>         spin_lock_irqsave(ap->lock, flags);
> 
> and then does
> 
>         if (ata_scsi_offline_dev(dev)) {
>                 dev->flags |= ATA_DFLAG_DETACHED;
>                 ap->pflags |= ATA_PFLAG_SCSI_HOTPLUG;
>         }
> 
> inside that spinlock. And this code is not new - it has done it since
> 2006 or so.
> 
> But it does seem to be a new regression in 4.14, caused by commit
> 8a97712e5314 ("scsi: make 'state' device attribute pollable"), because
> that's what added the sysfs_notify() call to scsi_device_set_state(),
> which made that spinlock be a problem.
> 
> That commit came in through the SCSI merge this merge window, and it
> seems to still revert cleanly.
> 
> So I do suspect that by now we should just revert that commit. It's
> not clear why that state attribute should be pollable, and the new
> code is clearly very much buggy.
> 
> Hannes, Martin?
> 
Seeing the complexity involved, yes, please revert that.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)