On Fri, Dec 23, 2005 at 02:53:39PM -0600, James Bottomley wrote:
> On Fri, 2005-12-23 at 23:43 +0300, Sergey Vlasov wrote:
> > So if that GFP_ATOMIC allocation ever fails, the target is leaked
> > forever - does not look nice.
>
> Yes, but it will print a warning.
>
> > 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.
>
> 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.
Ah, I see now... scsi_alloc_target() checks if a target with the same
channel/id combination is on the list and adds new target only if
there was no such one before.
However, what prevents a race between scsi_target_reap_work() and
scsi_alloc_target()? If the worker thread is interrupted/preempted
just after releasing host_lock (when it has already removed the target
from the list, but before it has called device_del()),
scsi_alloc_target() might consider the target as new and get to
device_add() faster.
Attachment:
pgpF0eOrLi65m.pgp
Description: PGP signature
