Re: [PATCH] fc_user_scan correction

[Mike Christie <michaelc@xxxxxxxxxxx>]

Mike Christie wrote:
> James Smart wrote:
> > Way back when, when the fc_user_scan routine was created, it kept some
> > of its original logic that walked the rport list and kicked off a scan.
> > Unfortunately, it didn't keep any of the locking around the rport list,
> > nor did it consider the synchronous nature of the scan invoked. The 
> > result,
> > there are some scan requests where the rport list changes, thus a 
> > subsequent
> > scan is called on a bogus rport structure and the system NMI's.
> >
> > The fix is to defer to the midlayer scan routine and not deal with 
> > locking
> > in the user scan function.
> >
> > Patch was cut against scsi-misc-2.6
> >
> > -- james s
> >
> >
> >  Signed-off-by: James Smart <james.smart@xxxxxxxxxx>
> >
> >  ---
> >
> >  scsi_scan.c         |    1 +
> >  scsi_transport_fc.c |   21 +--------------------
> >  2 files changed, 2 insertions(+), 20 deletions(-)
> >
> >
> > diff -upNr a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > --- a/drivers/scsi/scsi_scan.c    2008-04-22 13:09:23.000000000 -0400
> > +++ b/drivers/scsi/scsi_scan.c    2008-04-22 13:10:02.000000000 -0400
> > @@ -1669,6 +1669,7 @@ int scsi_scan_host_selected(struct Scsi_
> >  
> >      return 0;
> >  }
> > +EXPORT_SYMBOL(scsi_scan_host_selected);
> >  
> >  static void scsi_sysfs_add_devices(struct Scsi_Host *shost)
> >  {
> > diff -upNr a/drivers/scsi/scsi_transport_fc.c 
> > b/drivers/scsi/scsi_transport_fc.c
> > --- a/drivers/scsi/scsi_transport_fc.c    2008-04-22 
> > 12:51:22.000000000 -0400
> > +++ b/drivers/scsi/scsi_transport_fc.c    2008-04-22 
> > 12:53:30.000000000 -0400
> > @@ -1929,29 +1929,10 @@ fc_timed_out(struct scsi_cmnd *scmd)
> >      return EH_NOT_HANDLED;
> >  }
> >  
> > -/*
> > - * Must be called with shost->host_lock held
> > - */
> >  static int fc_user_scan(struct Scsi_Host *shost, uint channel,
> >          uint id, uint lun)
> >  {
> > -    struct fc_rport *rport;
> > -
> > -    list_for_each_entry(rport, &fc_host_rports(shost), peers) {
> > -        if (rport->scsi_target_id == -1)
> > -            continue;
> > -
> > -        if (rport->port_state != FC_PORTSTATE_ONLINE)
> > -            continue;
> > -
> > -        if ((channel == SCAN_WILD_CARD || channel == rport->channel) &&
> > -            (id == SCAN_WILD_CARD || id == rport->scsi_target_id)) {
> > -            scsi_scan_target(&rport->dev, rport->channel,
> > -                     rport->scsi_target_id, lun, 1);
> > -        }
> > -    }
> > -
> > -    return 0;
> > +     return scsi_scan_host_selected(shost, channel, id, lun, 1);
> >  }
>
> Is this is the same as if you did not implement the user_scan callout? 
> scsi_sysfs.c will call
>
> scsi_scan_host_selected(shost, channel, id, lun, 1);
>
> I thought we added the user_scan callback because the transport classes 
> had to pass in the device struct between the host and target so we got
>
> .../host/rport/target/scsi_device
>
> instead of
>
> .../host/target/scsi_device
>
> qla4xxx has the same problem. Do not look at it for help :( It added a 
> mutex and does not deadlock because like the FC class it stats the 
> removal of the rport/session then device so the cache sync always fails 
> (the check ready function always returns DID_NO_CONNECT so the cache 
> sync fails). iscsi tcp/iser/bnx2i works because it has userspace helping 
> out with the removal and shutdown and does it in two stages.

I think we need some loop + locking + refcounting similar to how the 
shost_for_each_device loops over devices.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html