On Thu, 22 Dec 2005 22:13:29 -0800 Andrew Morton wrote:
> Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx> wrote:
> > tree d2f74a0351a09e184e124fd6ecf16e02ab768a0b
> > parent 42e33148df38c60b99d984b76b302c64397ebe4c
> > author James Bottomley <James.Bottomley@xxxxxxxxxxxx> 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 <James.Bottomley@xxxxxxxxxxxx>
> >
> > drivers/scsi/scsi_scan.c | 48 +++++++++++++++++++++++++++++++++++++----------
> > 1 files changed, 38 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > index 94e5167..e36c21e 100644
> > --- a/drivers/scsi/scsi_scan.c
> > +++ b/drivers/scsi/scsi_scan.c
[skip]
> > void scsi_target_reap(struct scsi_target *starget)
> > {
> > - 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.
>
> > - 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.
Patch is only compile tested.
------------------------------------------------------------------------
[SCSI] fix scsi_target_reap() device_del from atomic context
scsi_target_reap() may be called from any context, however, it needs to
call device_del(), which requires a process context. Thus we have to
perform device_del() via a workqueue.
Signed-off-by: Sergey Vlasov <vsu@xxxxxxxxxxx>
---
drivers/scsi/scsi_scan.c | 33 +++++++++++++++++++++++++++++----
1 files changed, 29 insertions(+), 4 deletions(-)
28763f31e602e7265b61e676dcc1b536b5442fbf
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 94e5167..ac4c75f 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -400,6 +400,30 @@ static struct scsi_target *scsi_alloc_ta
return found_target;
}
+static LIST_HEAD(scsi_target_reap_list);
+static DEFINE_SPINLOCK(scsi_target_reap_list_lock);
+
+static void scsi_target_reap_work_fn(void *data)
+{
+ struct scsi_target *starget;
+ unsigned long flags;
+
+ spin_lock_irqsave(&scsi_target_reap_list_lock, flags);
+ while (!list_empty(&scsi_target_reap_list)) {
+ starget = list_entry(scsi_target_reap_list.next,
+ struct scsi_target, siblings);
+ list_del_init(&starget->siblings);
+ spin_unlock_irqrestore(&scsi_target_reap_list_lock, flags);
+ device_del(&starget->dev);
+ transport_unregister_device(&starget->dev);
+ put_device(&starget->dev);
+ spin_lock_irqsave(&scsi_target_reap_list_lock, flags);
+ }
+ spin_unlock_irqrestore(&scsi_target_reap_list_lock, flags);
+}
+
+static DECLARE_WORK(scsi_target_reap_work, scsi_target_reap_work_fn, NULL);
+
/**
* scsi_target_reap - check to see if target is in use and destroy if not
*
@@ -416,11 +440,12 @@ void scsi_target_reap(struct scsi_target
spin_lock_irqsave(shost->host_lock, flags);
if (--starget->reap_ref == 0 && list_empty(&starget->devices)) {
- list_del_init(&starget->siblings);
+ list_del(&starget->siblings);
spin_unlock_irqrestore(shost->host_lock, flags);
- device_del(&starget->dev);
- transport_unregister_device(&starget->dev);
- put_device(&starget->dev);
+ spin_lock_irqsave(&scsi_target_reap_list_lock, flags);
+ list_add_tail(&starget->siblings, &scsi_target_reap_list);
+ spin_unlock_irqrestore(&scsi_target_reap_list_lock, flags);
+ schedule_work(&scsi_target_reap_work);
return;
}
spin_unlock_irqrestore(shost->host_lock, flags);
--
1.0.GIT
Attachment:
pgpp5I2Qt0bbF.pgp
Description: PGP signature
