PATCH [1/1]: sd_remove() hangs waiting on async_synchronize of unrelated threads

PATCH Michael Reed 2 participants 9 messages Dec 1, 2009 – Dec 2, 2009 SCSI

Share & tools

Michael Reed

author

2009-12-01 21:45 UTC #0 lore raw

Prevent delays and hangs due to sd_remove() waiting for the completion of async threads executing sd_probe_async of disks on unrelated host adapters. This patch executes every sd_probe_async in its own async domain allowing sd_remove() to wait for just the completion of the async thread associated with the scsi_disk being removed.

Found via fault insertion on a large fibre channel fabric.

Applies to 2.6.32-rc8.

Signed-off-by Michael Reed <[email protected]>

drivers/scsi/sd.h +1 -0

@@ -60,6 +60,7 @@ struct scsi_disk {
 	unsigned	RCD : 1;	/* state of disk RCD bit, unused */
 	unsigned	DPOFUA : 1;	/* state of disk DPOFUA bit */
 	unsigned	first_scan : 1;
+	struct list_head async_domain;	/* for sd_probe_async */
 };
 #define to_scsi_disk(obj) container_of(obj,struct scsi_disk,dev)

drivers/scsi/sd.c +4 -2

@@ -2171,7 +2171,8 @@ static int sd_probe(struct device *dev)
 	get_device(&sdp->sdev_gendev);
 
 	get_device(&sdkp->dev);	/* prevent release before async_schedule */
-	async_schedule(sd_probe_async, sdkp);
+	INIT_LIST_HEAD(&sdkp->async_domain);
+	async_schedule_domain(sd_probe_async, sdkp, &sdkp->async_domain);
 
 	return 0;

@@ -2202,8 +2203,9 @@ static int sd_remove(struct device *dev)
 {
 	struct scsi_disk *sdkp;
 
-	async_synchronize_full();
 	sdkp = dev_get_drvdata(dev);
+	async_synchronize_full_domain(&sdkp->async_domain);
+
 	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
 	device_del(&sdkp->dev);
 	del_gendisk(sdkp->disk);

James Bottomley

2009-12-01 21:53 UTC #1 lore raw

Quoted 5 lines · Michael Reed

Prevent delays and hangs due to sd_remove() waiting for the completion of async threads executing sd_probe_async of disks on unrelated host adapters. This patch executes every sd_probe_async in its own async domain allowing sd_remove() to wait for just the completion of the async thread associated with the scsi_disk being removed.

This patch was thought of a while ago. Unfortunately, some of the unrelated threads we end up waiting on are libata ones. you confine sd to only its own probes, we end up unsynchronised with respect to libata probes and we might cause ordering problems amongst the ata devices.

James

Michael Reed

author

2009-12-01 22:28 UTC #2 lore raw

Quoted 7 lines · James Bottomley

Quoted 5 lines · Michael Reed
Prevent delays and hangs due to sd_remove() waiting for the completion of async threads executing sd_probe_async of disks on unrelated host adapters. This patch executes every sd_probe_async in its own async domain allowing sd_remove() to wait for just the completion of the async thread associated with the scsi_disk being removed.

Quoted 5 lines

This patch was thought of a while ago. Unfortunately, some of the unrelated threads we end up waiting on are libata ones. you confine sd to only its own probes, we end up unsynchronised with respect to libata probes and we might cause ordering problems amongst the ata devices.

Isn't sd_remove() only concerned with the removal of a a single scsi_disk? Shouldn't libata use reference counting if it has is an issue with a scsi_disk being prematurely removed? Or is this a concern about "sd" naming? Or something else that I admittedly don't understand (but should)?

I would truly like to better understand the issue. Would someone mind expanding upon the concern about ata ordering issues associated with the removal of a single scsi_disk?

Thanks,
Mike

Quoted 7 lines

James
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

James Bottomley

2009-12-01 22:50 UTC #3 lore raw

Quoted 15 lines · Michael Reed

Quoted 7 lines · James Bottomley
Quoted 5 lines · Michael Reed
Prevent delays and hangs due to sd_remove() waiting for the completion of async threads executing sd_probe_async of disks on unrelated host adapters. This patch executes every sd_probe_async in its own async domain allowing sd_remove() to wait for just the completion of the async thread associated with the scsi_disk being removed.
Quoted 5 lines
This patch was thought of a while ago. Unfortunately, some of the unrelated threads we end up waiting on are libata ones. you confine sd to only its own probes, we end up unsynchronised with respect to libata probes and we might cause ordering problems amongst the ata devices.

Quoted 4 lines

Isn't sd_remove() only concerned with the removal of a a single scsi_disk? Shouldn't libata use reference counting if it has is an issue with a scsi_disk being prematurely removed? Or is this a concern about "sd" naming? Or something else that I admittedly don't understand (but should)?

Well, no ... the sync on remove is preventing us removing a device whose async part is still running. That async part for libata includes pieces kicked off from the sd probe.

Quoted 3 lines

I would truly like to better understand the issue. Would someone mind expanding upon the concern about ata ordering issues associated with the removal of a single scsi_disk?

The problem isn't removal per se ... it's the fact that remove can't complete until any async pieces remaining from probe have run.

James

Michael Reed

author

2009-12-02 14:27 UTC #4 lore raw

Quoted 13 lines · James Bottomley

Quoted 12 lines · Michael Reed
Quoted 11 lines · James Bottomley
Quoted 5 lines · Michael Reed
Prevent delays and hangs due to sd_remove() waiting for the completion of async threads executing sd_probe_async of disks on unrelated host adapters. This patch executes every sd_probe_async in its own async domain allowing sd_remove() to wait for just the completion of the async thread associated with the scsi_disk being removed.
This patch was thought of a while ago. Unfortunately, some of the unrelated threads we end up waiting on are libata ones. you confine sd to only its own probes, we end up unsynchronised with respect to libata probes and we might cause ordering problems amongst the ata devices.

Quoted 8 lines

Quoted 4 lines
Isn't sd_remove() only concerned with the removal of a a single scsi_disk? Shouldn't libata use reference counting if it has is an issue with a scsi_disk being prematurely removed? Or is this a concern about "sd" naming? Or something else that I admittedly don't understand (but should)?
Well, no ... the sync on remove is preventing us removing a device whose async part is still running. That async part for libata includes pieces kicked off from the sd probe.

What does the call stack look like that spawns the async part of libata?

What kind of hardware do I need to demonstrate this?

Quoted 7 lines

Quoted 4 lines
I would truly like to better understand the issue. Would someone mind expanding upon the concern about ata ordering issues associated with the removal of a single scsi_disk?
The problem isn't removal per se ... it's the fact that remove can't complete until any async pieces remaining from probe have run.

Yes, I understand that. I didn't realize that the sd_probe resulted in any async work other than sd_probe_async(). It does complicate serialization at removal.

I'll try to capture the "motivation" from within my fibre channel centric world for the change and see if anyone's got some ideas on how to resolve the issue.

Mike

Quoted 2 lines

James

James Bottomley

2009-12-02 14:43 UTC #5 lore raw

Quoted 19 lines · Michael Reed

Quoted 18 lines · James Bottomley
Quoted 16 lines · Michael Reed
Quoted 11 lines · James Bottomley
Quoted 5 lines · Michael Reed
Prevent delays and hangs due to sd_remove() waiting for the completion of async threads executing sd_probe_async of disks on unrelated host adapters. This patch executes every sd_probe_async in its own async domain allowing sd_remove() to wait for just the completion of the async thread associated with the scsi_disk being removed.
This patch was thought of a while ago. Unfortunately, some of the unrelated threads we end up waiting on are libata ones. you confine sd to only its own probes, we end up unsynchronised with respect to libata probes and we might cause ordering problems amongst the ata devices.
Isn't sd_remove() only concerned with the removal of a a single scsi_disk? Shouldn't libata use reference counting if it has is an issue with a scsi_disk being prematurely removed? Or is this a concern about "sd" naming? Or something else that I admittedly don't understand (but should)?

Quoted 5 lines

Quoted 3 lines
Well, no ... the sync on remove is preventing us removing a device whose async part is still running. That async part for libata includes pieces kicked off from the sd probe.
What does the call stack look like that spawns the async part of libata?

it's in libata-core (and some in ahci) just do a git grep async in drivers/ata

Quoted 1 line

What kind of hardware do I need to demonstrate this?

To demonstrate what? The out of order sequencing that can arise? any ata based system with > 1 device should do.

There's actually another problem with the previous patch ... scsi_wait_scan() also needs to synchronise with the sd probes.

Quoted 7 lines

Quoted 6 lines
Quoted 3 lines
I would truly like to better understand the issue. Would someone mind expanding upon the concern about ata ordering issues associated with the removal of a single scsi_disk?
The problem isn't removal per se ... it's the fact that remove can't complete until any async pieces remaining from probe have run.

Quoted 6 lines

Yes, I understand that. I didn't realize that the sd_probe resulted in any async work other than sd_probe_async(). It does complicate serialization at removal.
I'll try to capture the "motivation" from within my fibre channel centric world for the change and see if anyone's got some ideas on how to resolve the issue.

OK ... actually describing the problem would be helpful. The async schedule is only in sd_remove() to guard against add/remove races ... usually when we do removal, the async probing parts should be long finished so I don't understand why you think we would be waiting for stuff.

James

Michael Reed

author

2009-12-02 15:18 UTC #6 lore raw

Quoted 27 lines · James Bottomley

Quoted 14 lines · Michael Reed
Quoted 13 lines · James Bottomley
Quoted 12 lines · Michael Reed
Quoted 11 lines · James Bottomley
Quoted 5 lines · Michael Reed
Prevent delays and hangs due to sd_remove() waiting for the completion of async threads executing sd_probe_async of disks on unrelated host adapters. This patch executes every sd_probe_async in its own async domain allowing sd_remove() to wait for just the completion of the async thread associated with the scsi_disk being removed.
This patch was thought of a while ago. Unfortunately, some of the unrelated threads we end up waiting on are libata ones. you confine sd to only its own probes, we end up unsynchronised with respect to libata probes and we might cause ordering problems amongst the ata devices.
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Isn't sd_remove() only concerned with the removal of a a single scsi_disk? Shouldn't libata use reference counting if it has is an issue with a scsi_disk being prematurely removed? Or is this a concern about "sd" naming? Or something else that I admittedly don't understand (but should)?
Well, no ... the sync on remove is preventing us removing a device whose async part is still running. That async part for libata includes pieces kicked off from the sd probe.
What does the call stack look like that spawns the async part of libata?
it's in libata-core (and some in ahci) just do a git grep async in drivers/ata

Quoted 7 lines

Quoted 1 line
What kind of hardware do I need to demonstrate this?
To demonstrate what? The out of order sequencing that can arise? any ata based system with > 1 device should do.
There's actually another problem with the previous patch ... scsi_wait_scan() also needs to synchronise with the sd probes.

Thank you for pointing this out.

Quoted 10 lines

Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
I would truly like to better understand the issue. Would someone mind expanding upon the concern about ata ordering issues associated with the removal of a single scsi_disk?
The problem isn't removal per se ... it's the fact that remove can't complete until any async pieces remaining from probe have run.
Yes, I understand that. I didn't realize that the sd_probe resulted in any async work other than sd_probe_async(). It does complicate serialization at removal.

Quoted 8 lines

Quoted 2 lines
I'll try to capture the "motivation" from within my fibre channel centric world for the change and see if anyone's got some ideas on how to resolve the issue.
OK ... actually describing the problem would be helpful. The async schedule is only in sd_remove() to guard against add/remove races ... usually when we do removal, the async probing parts should be long finished so I don't understand why you think we would be waiting for stuff.

It appears that the async_synchronize_full() waits for all async threads from whatever source. The issue arises when there is sd_probe_async() work active for other luns.

Lots and LOTS of luns can delay the sd_probe_async work. I've got over 7000 total luns on my test system via 13 different FC host adapters. What runs quickly on a small system can take a while on this config. Add fibre channel link up / down events or fabric changes can cause fc_remote_port_delete() and fc_remote_port_add() calls (and associated target scans), the time needed to process these events can exceed the device removal timeout resulting in removes while scan is still running and there's lots of async work pending.

I'll try to provide a more detailed problem description later today. (I knew I should have saved those backtraces....)

Thanks for your help in explaining the issues associated with the patch.

Mike

Quoted 4 lines

James

James Bottomley

2009-12-02 15:35 UTC #7 lore raw

Quoted 18 lines · Michael Reed

Quoted 8 lines · James Bottomley
Quoted 7 lines · Michael Reed
Quoted 2 lines · James Bottomley
The problem isn't removal per se ... it's the fact that remove can't complete until any async pieces remaining from probe have run.
Yes, I understand that. I didn't realize that the sd_probe resulted in any async work other than sd_probe_async(). It does complicate serialization at removal.
Quoted 8 lines
Quoted 2 lines
I'll try to capture the "motivation" from within my fibre channel centric world for the change and see if anyone's got some ideas on how to resolve the issue.
OK ... actually describing the problem would be helpful. The async schedule is only in sd_remove() to guard against add/remove races ... usually when we do removal, the async probing parts should be long finished so I don't understand why you think we would be waiting for stuff.

Quoted 3 lines

It appears that the async_synchronize_full() waits for all async threads from whatever source. The issue arises when there is sd_probe_async() work active for other luns.

Ah ... I'm afraid trying to fix that currently can't be done. The reason why we do a global synchronise is so that even with the async pieces, everything shows up in-order (so the luns get sequentially lettered in the sd<X> space).

Unfortunately, we're not yet to the point where we can expect devices to show up entirely randomly on each boot and have the user cope. Even though udev can help with this, there are still too many non-udev or simply just /dev/sd<X> using systems out there to dump the sequential scan order just yet.

Quoted 8 lines

Lots and LOTS of luns can delay the sd_probe_async work. I've got over 7000 total luns on my test system via 13 different FC host adapters. What runs quickly on a small system can take a while on this config. Add fibre channel link up / down events or fabric changes can cause fc_remote_port_delete() and fc_remote_port_add() calls (and associated target scans), the time needed to process these events can exceed the device removal timeout resulting in removes while scan is still running and there's lots of async work pending.

Is the problem one of thread/work scheduling? As in we just have too much work to do and the scheduler isn't coping? ... figures comparing what goes on with the fully sync case would be helpful.

Quoted 4 lines

I'll try to provide a more detailed problem description later today. (I knew I should have saved those backtraces....)
Thanks for your help in explaining the issues associated with the patch.

You're welcome,

James

Michael Reed

author

2009-12-02 17:06 UTC #8 lore raw

In reply to James Bottomley

Quoted 44 lines · James Bottomley

Quoted 14 lines · Michael Reed
Quoted 13 lines · James Bottomley
Quoted 12 lines · Michael Reed
Quoted 11 lines · James Bottomley
Quoted 5 lines · Michael Reed
Prevent delays and hangs due to sd_remove() waiting for the completion of async threads executing sd_probe_async of disks on unrelated host adapters. This patch executes every sd_probe_async in its own async domain allowing sd_remove() to wait for just the completion of the async thread associated with the scsi_disk being removed.
This patch was thought of a while ago. Unfortunately, some of the unrelated threads we end up waiting on are libata ones. you confine sd to only its own probes, we end up unsynchronised with respect to libata probes and we might cause ordering problems amongst the ata devices.
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Isn't sd_remove() only concerned with the removal of a a single scsi_disk? Shouldn't libata use reference counting if it has is an issue with a scsi_disk being prematurely removed? Or is this a concern about "sd" naming? Or something else that I admittedly don't understand (but should)?
Well, no ... the sync on remove is preventing us removing a device whose async part is still running. That async part for libata includes pieces kicked off from the sd probe.
What does the call stack look like that spawns the async part of libata?
it's in libata-core (and some in ahci) just do a git grep async in drivers/ata
Quoted 1 line
What kind of hardware do I need to demonstrate this?
To demonstrate what? The out of order sequencing that can arise? any ata based system with > 1 device should do.
There's actually another problem with the previous patch ... scsi_wait_scan() also needs to synchronise with the sd probes.
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
I would truly like to better understand the issue. Would someone mind expanding upon the concern about ata ordering issues associated with the removal of a single scsi_disk?
The problem isn't removal per se ... it's the fact that remove can't complete until any async pieces remaining from probe have run.
Yes, I understand that. I didn't realize that the sd_probe resulted in any async work other than sd_probe_async(). It does complicate serialization at removal.

Quoted 8 lines

Quoted 2 lines
I'll try to capture the "motivation" from within my fibre channel centric world for the change and see if anyone's got some ideas on how to resolve the issue.
OK ... actually describing the problem would be helpful. The async schedule is only in sd_remove() to guard against add/remove races ... usually when we do removal, the async probing parts should be long finished so I don't understand why you think we would be waiting for stuff.

As I've been working out of 2.6.31 and things have changed pretty dramatically in scsi-misc, I'm going to update my test environment to scsi-misc and see if I continue to observe the same behavior.

Moving targets are good. Moving targets are good. Moving targets are good.

Mike

Quoted 4 lines

James