fix scsi_reap_target() device_del from atomic context

tree d2f74a0351a09e184e124fd6ecf16e02ab768a0b
parent 42e33148df38c60b99d984b76b302c64397ebe4c
author James Bottomley <[email protected]> Fri, 16 Dec 2005 12:01:43 -0800 committer James Bottomley <jejb@mulgrave.(none)> Sat, 17 Dec 2005 22:48:08 -0600

[SCSI] fix scsi_reap_target() device_del from atomic context

scsi_reap_target() was desgined to be called from any context. However it must do a device_del() of the target device, which may only be called from user context. Thus we have to reimplement scsi_reap_target() via a workqueue.

Signed-off-by James Bottomley <[email protected]>

 drivers/scsi/scsi_scan.c |   48 +++++++++++++++++++++++++++++++++++++----------
 1 files changed, 38 insertions(+), 10 deletions(-)

drivers/scsi/scsi_scan.c +0 -0

index 94e5167..e36c21e 100644

drivers/scsi/scsi_scan.c +12 -0

+	struct work_queue_wrapper *wqw = (struct work_queue_wrapper *)data;
+	struct scsi_target *starget = wqw->starget;
+	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
+	unsigned long flags;
+
+	kfree(wqw);
+
+	spin_lock_irqsave(shost->host_lock, flags);
+
+	if (--starget->reap_ref == 0 && list_empty(&starget->devices)) {
+		list_del_init(&starget->siblings);
+		spin_unlock_irqrestore(shost->host_lock, flags);

drivers/scsi/scsi_scan.c +7 -7

-	if (--starget->reap_ref == 0 && list_empty(&starget->devices)) {
-		list_del_init(&starget->siblings);
-		spin_unlock_irqrestore(shost->host_lock, flags);
-		device_del(&starget->dev);
-		transport_unregister_device(&starget->dev);
-		put_device(&starget->dev);
+	if (!wqw) {
+		starget_printk(KERN_ERR, starget,
+			       "Failed to allocate memory in scsi_reap_target()\n");
 		return;
 	}
-	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	INIT_WORK(&wqw->work, scsi_target_reap_work, wqw);
+	wqw->starget = starget;
+	schedule_work(&wqw->work);
 }

Quoted 17 lines

(quoted diff) +17 -0

+	struct work_queue_wrapper *wqw = (struct work_queue_wrapper *)data;
+	struct scsi_target *starget = wqw->starget;
+	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
+	unsigned long flags;
+
+	kfree(wqw);
+
+	spin_lock_irqsave(shost->host_lock, flags);
+
+	if (--starget->reap_ref == 0 && list_empty(&starget->devices)) {
+		list_del_init(&starget->siblings);
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		device_del(&starget->dev);
+		transport_unregister_device(&starget->dev);
+		put_device(&starget->dev);
+		return;
+

Review

Quoted 5 lines

(quoted diff) +5 -0

+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	return;
+}

Given that this can run an arbitrary amount of time later on, how do we know that *shost is still live?

Quoted 2 lines · Andrew Morton

Given that this can run an arbitrary amount of time later on, how do we know that *shost is still live?

If there's still an starget, its parent shost must still be around, no?

There is a potential improvement, in that could be done which is only to use the workqueue if we're in atomic context. However, I elected to leave playing with that cleanup until after 2.6.15

There is also the point that I now have two of these allocations of structures containing a workqueue and a pointer in separate instances. It does look like this might be an improvement to the API (i.e. a workqueue use that manages the allocation of the actual work_struct).

Quoted 3 lines · James Bottomley

There is a potential improvement, in that could be done which is only to use the workqueue if we're in atomic context. However, I elected to leave playing with that cleanup until after 2.6.15

We don't have a way of determining whether we're in atomic context (in_atomic() only works with CONFIG_PREEMPT). If scsi internally knows what context things are in then that'll work OK.

Quoted 4 lines

There is also the point that I now have two of these allocations of structures containing a workqueue and a pointer in separate instances. It does look like this might be an improvement to the API (i.e. a workqueue use that manages the allocation of the actual work_struct).

Perhaps you could use work_struct.data for the scsi_target* and get back to the work_struct via container_of().

Quoted 3 lines

Perhaps you could use work_struct.data for the scsi_target* and get back to the work_struct via container_of().

Could you elaborate some more on this? If I simply use the starget pointer as my work_struct.data, how do I get back to the actual work_struct for me to free it? It's not passed in to the function as far as I can tell. But even if I do this, I still have to manage the allocation and deallocation of the work_struct.

Quoted 17 lines

(quoted diff) +17 -0

+	struct work_queue_wrapper *wqw = (struct work_queue_wrapper *)data;
+	struct scsi_target *starget = wqw->starget;
+	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
+	unsigned long flags;
+
+	kfree(wqw);
+
+	spin_lock_irqsave(shost->host_lock, flags);
+
+	if (--starget->reap_ref == 0 && list_empty(&starget->devices)) {
+		list_del_init(&starget->siblings);
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		device_del(&starget->dev);
+		transport_unregister_device(&starget->dev);
+		put_device(&starget->dev);
+		return;
+

Review

Quoted 5 lines

(quoted diff) +5 -0

+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	return;
+}

Given that this can run an arbitrary amount of time later on, how do we know that *shost is still live?

	EIP is at sysfs_hash_and_remove+0x31/0x11f
	eax: 00000000   ebx: 6b6b6b6b   ecx: 00000000   edx: ee6aac68
	esi: 6b6b6b6b   edi: f8839d44   ebp: f8839cc0   esp: c1af7e98
	ds: 007b   es: 007b   ss: 0068
	Process events/2 (pid: 16, threadinfo=c1af7000 task=c1aef030)
	Stack: c03998d2 00000063 eb6a0e2c eb6a0e24 f8839d44 f8839cc0 c025877b e640aac0 
	       ee000f5c ee000f5c 00000000 f8839cc0 f332e178 eb6a0e24 c025ab08 c025ab39 
	       eb6a0e24 c0370606 eb6a0e00 f332e178 c1af7f00 e5a872dc c025a722 f332e178 
	Call Trace:
	 [<c025877b>] class_device_del+0x138/0x163
	 [<c025ab08>] transport_remove_classdev+0x0/0x68

Quoted 11 lines · Andrew Vasquez

	EIP is at sysfs_hash_and_remove+0x31/0x11f
	eax: 00000000   ebx: 6b6b6b6b   ecx: 00000000   edx: ee6aac68
	esi: 6b6b6b6b   edi: f8839d44   ebp: f8839cc0   esp: c1af7e98
	ds: 007b   es: 007b   ss: 0068
	Process events/2 (pid: 16, threadinfo=c1af7000 task=c1aef030)
	Stack: c03998d2 00000063 eb6a0e2c eb6a0e24 f8839d44 f8839cc0 c025877b e640aac0 
	       ee000f5c ee000f5c 00000000 f8839cc0 f332e178 eb6a0e24 c025ab08 c025ab39 
	       eb6a0e24 c0370606 eb6a0e00 f332e178 c1af7f00 e5a872dc c025a722 f332e178 
	Call Trace:
	 [<c025877b>] class_device_del+0x138/0x163
	 [<c025ab08>] transport_remove_classdev+0x0/0x68

This one is the signal for class devices being deleted *after* the underlying device has been deleted, so the class device delete oopses trying to remove the symlink from the real device to the class device.

Strange it should show up now, since that particular piece of code hasn't been altered, just moved into a work queue.

See if the attached fixes it (if it does, I'm mystified as to why you haven't always seen this).

James

Quoted 21 lines · Linux Kernel Mailing List

tree d2f74a0351a09e184e124fd6ecf16e02ab768a0b
parent 42e33148df38c60b99d984b76b302c64397ebe4c
author James Bottomley <[email protected]> Fri, 16 Dec 2005 12:01:43 -0800 committer James Bottomley <jejb@mulgrave.(none)> Sat, 17 Dec 2005 22:48:08 -0600

[SCSI] fix scsi_reap_target() device_del from atomic context

scsi_reap_target() was desgined to be called from any context. However it must do a device_del() of the target device, which may only be called from user context. Thus we have to reimplement scsi_reap_target() via a workqueue.

Signed-off-by James Bottomley <[email protected]>

 drivers/scsi/scsi_scan.c |   48 +++++++++++++++++++++++++++++++++++++----------
 1 files changed, 38 insertions(+), 10 deletions(-)

drivers/scsi/scsi_scan.c +0 -0

index 94e5167..e36c21e 100644

drivers/scsi/scsi_scan.c

Quoted 7 lines

void scsi_target_reap(struct scsi_target *starget) {

drivers/scsi/scsi_scan.c +2 -3

-	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
-	unsigned long flags;
-	spin_lock_irqsave(shost->host_lock, flags);
+	struct work_queue_wrapper *wqw = 
+		kzalloc(sizeof(struct work_queue_wrapper), GFP_ATOMIC);

kmalloc() would suffice.

drivers/scsi/scsi_scan.c

Quoted 10 lines

drivers/scsi/scsi_scan.c +3 -6

-	if (--starget->reap_ref == 0 && list_empty(&starget->devices)) {
-		list_del_init(&starget->siblings);
-		spin_unlock_irqrestore(shost->host_lock, flags);
-		device_del(&starget->dev);
-		transport_unregister_device(&starget->dev);
-		put_device(&starget->dev);
+	if (!wqw) {
+		starget_printk(KERN_ERR, starget,
+			       "Failed to allocate memory in scsi_reap_target()\n");
 		return;

So if that GFP_ATOMIC allocation ever fails, the target is leaked forever - does not look nice.

Does anything depend on starget->siblings being empty after we had removed it from the shost->__targets list it was on? Seems that all other uses of this field are:

drivers/scsi/scsi_scan.c:313:   list_for_each_entry(starget, &shost->__targets, siblings) {
drivers/scsi/scsi_scan.c:365:   INIT_LIST_HEAD(&starget->siblings);
drivers/scsi/scsi_scan.c:373:   list_add_tail(&starget->siblings, &shost->__targets);

So probably we can reuse this field and do deferred reaping without any memory allocation at all. The following patch should be applied _instead_ of the James' patch, not on top of it (I can make a combined patch if it is desired). The difference with the previous patch is that scsi_target_reap() still removes the target from shost->__targets immediately - only device_del() and subsequent actions are deferred to a workqueue.

Quoted 2 lines · Sergey Vlasov

So if that GFP_ATOMIC allocation ever fails, the target is leaked forever - does not look nice.

Yes, but it will print a warning.

Quoted 8 lines

Does anything depend on starget->siblings being empty after we had removed it from the shost->__targets list it was on? Seems that all other uses of this field are:

drivers/scsi/scsi_scan.c:313:   list_for_each_entry(starget, &shost->__targets, siblings) {
drivers/scsi/scsi_scan.c:365:   INIT_LIST_HEAD(&starget->siblings);
drivers/scsi/scsi_scan.c:373:   list_add_tail(&starget->siblings, &shost->__targets);

Quoted 7 lines

So probably we can reuse this field and do deferred reaping without any memory allocation at all. The following patch should be applied _instead_ of the James' patch, not on top of it (I can make a combined patch if it is desired). The difference with the previous patch is that scsi_target_reap() still removes the target from shost->__targets immediately - only device_del() and subsequent actions are deferred to a workqueue.

No, you can't.

If you do this, the target namespace will potentially be in use in sysfs after the system thinks the target is gone. Thus, any reallocation fails because you can't add a new target with the same name as an existing one.