Re: [PATCH] Separate target visibility from reaped state information

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2016-02-02 at 10:29 -0800, Bart Van Assche wrote:
> On 02/02/2016 03:46 AM, James Bottomley wrote:
> > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> > index 4f18a85..00bc721 100644
> > --- a/drivers/scsi/scsi_sysfs.c
> > +++ b/drivers/scsi/scsi_sysfs.c
> > @@ -1272,16 +1272,18 @@ static void __scsi_remove_target(struct
> > scsi_target *starget)
> >   void scsi_remove_target(struct device *dev)
> >   {
> >       struct Scsi_Host *shost = dev_to_shost(dev->parent);
> > -     struct scsi_target *starget;
> > +     struct scsi_target *starget, *last_target = NULL;
> >       unsigned long flags;
> > 
> >   restart:
> >       spin_lock_irqsave(shost->host_lock, flags);
> >       list_for_each_entry(starget, &shost->__targets, siblings) {
> > -             if (starget->state == STARGET_DEL)
> > +             if (starget->state == STARGET_DEL ||
> > +                 starget == last_target)
> >                       continue;
> >               if (starget->dev.parent == dev || &starget->dev ==
> > dev) {
> >                       kref_get(&starget->reap_ref);
> > +                     last_target = starget;
> >                       spin_unlock_irqrestore(shost->host_lock,
> > flags);
> >                       __scsi_remove_target(starget);
> >                       scsi_target_reap(starget);
> 
> Hello James,
> 
> Do you think it is a robust approach to store the pointer to the last
> removed target in the last_target variable ?

Well, yes, I think it will work, if that's what you mean.

>  What if e.g. scsi_target_reap() frees the memory the last_target 
> pointer points at and another thread reallocates a scsi_target data 
> structure ? Can that last data structure have the same address as the 
> contents of the last_target variable ?

Yes, but it doesn't matter, does it?  Add/Remove has always (and will
always) be racy.  Under current conditions you can still add to the
target after the list_for_each terminates and have scsi_remove_target()
return with attached devices.  The only way to close the race is
basically to forbid scanning as we shut down the host and wait for all
in-progress scans before starting the final removals.

James