[Announce] Emulex lpfcdriver v8.0.20 available

[Announce] Emulex lpfcdriver v8.0.20 available

[James.Smart]

Christoph and Community,

The 8.0.20 release of the Emulex driver has been recently posted
to SourceForge:
http://prdownloads.sourceforge.net/lpfcxxxx/lpfcdriver-2.6-8.0.20.tar.gz?download


Emulex has been working hard to complete the outstanding items for upstream
acceptance:

- Streamline I/O submission path.
- Removal of GFP_ATOMIC allocations in I/O path. Consider locations
  as well.
- Movement of Consistent Bindings to the FC Transport.
- Discovery changes:
   Target discovery via scsi_scan_single_target. Reduce bookkeeping
     in lpfc_find_target().
   Cleanse target removal. Ensure no more __devices references.
- Removal of ifdefs for older kernel support and different patches

And from our point of view :
- Add management interfaces in support of HBAAPI.


The 8.0.20 release addresses all of the above per the following comments:

- We have significantly streamlined our i/o path, removing all
    GFP_ATOMIC allocations in this path and in most eh_xxxx paths.
    Atomic allocations still exist in our discovery thread, but
    we are looking into removing them as well.
- With the proposed Remote Port additions to the FC transport, we've
    integrated remote ports (rport's) into our node state machine.
    This has reduced much of the target-level housekeeping, moved
    all consistent (semi-persistent) target id bindings into the
    fc transport, and coordinated device discovery/removal with the
    transport.
- Discovery has been reworked such that:
    There is no initial host-level scan.
    Driver registers FC remote ports with FC transport. FC transport
      assigns SCSI target ids for the ports (and implements any
      persistence).
    Driver invokes SCSI bus scans on a per-target level, after
      noticing target id assignments and preparing proper calling
      context.
    If cable pull/device loss, driver calls transport with port-level
      block call (which in turn makes the scsi target block call).
      transport starts target block timer.
    If port returns prior to timeout - driver calls transport
      port-level unblock call, which restarts i/o to devices.
    If port does not return prior to timeout - in timeout handler,
      transport will remove all attached scsi devices.
    When driver decides to release all knowledge of remote port, it
      notifies transport. Transport will remove any attached scsi
      devices.
- We've posted FC Transport patches for host attributes/statistics,
  and remote port attributes, sufficient for HBAAPI.  We've added
  support for these attributes in the driver.
- The ifdefs for older-kernel support and to select kernel patches
  has been removed. The driver expects the latest and greatest
  kernel and patches.


We believe this driver is ready for final reviews for upstream acceptance.
Please return any review comments.

-- James S

Re: [Announce] Emulex lpfcdriver v8.0.20 available

[Christoph Hellwig]

On Tue, Jan 11, 2005 at 01:34:37PM -0500, James.Smart@Emulex.Com wrote:
> Christoph and Community,
> 
> The 8.0.20 release of the Emulex driver has been recently posted
> to SourceForge:
> http://prdownloads.sourceforge.net/lpfcxxxx/lpfcdriver-2.6-8.0.20.tar.gz?download
> 
> 
> Emulex has been working hard to complete the outstanding items for upstream
> acceptance:
> 
> - Streamline I/O submission path.
> - Removal of GFP_ATOMIC allocations in I/O path. Consider locations
>   as well.
> - Movement of Consistent Bindings to the FC Transport.
> - Discovery changes:
>    Target discovery via scsi_scan_single_target. Reduce bookkeeping
>      in lpfc_find_target().
>    Cleanse target removal. Ensure no more __devices references.
> - Removal of ifdefs for older kernel support and different patches
> 
> And from our point of view :
> - Add management interfaces in support of HBAAPI.


I started to look through it, here's some thing I found so far:

 o there's still a ->proc_info method which we don't want for new drivers
 o the host removal is still quite hackish and probably racy:
    - everything what's you're ding with FC_UNLOADING should be done in
      the midlayer where we already have a state machine for the
      Scsi_Host
    - target removal shouldn't happen from your remove method but via
      scsi_remove_host
 o the driver would benefit a _lot_ from driver private data in
   scsi_target, I think we should do that and basically get rid of
   phba->device_queue_hash
 o what about lpfc_target->rport?  This should probably better be in
   the fc_target structure, no?
 o lpfc_target vs lpfc_nodelist.  I'd see these merged into a single
   structure.  In fact large parts of lpfc_nodelist should probably
   move into the transport class.
 o the memory allocation for non-I/O path stilled looked total overkill
   to me, but it seems you're doing an awfull lot from the interrupt handler
   still.  Wouldn't it be easier to do much of it from process context
   via the kernel thread?

RE: [Announce] Emulex lpfcdriver v8.0.20 available

[James.Smart]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Monday, January 17, 2005 5:23 PM
> To: Smart, James
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: [Announce] Emulex lpfcdriver v8.0.20 available
> 
> 
> I started to look through it, here's some thing I found so far:
> 
>  o there's still a ->proc_info method which we don't want for 
> new drivers

OK.

>  o the host removal is still quite hackish and probably racy:
>     - everything what's you're ding with FC_UNLOADING should 
> be done in
>       the midlayer where we already have a state machine for the
>       Scsi_Host

There's only 2 uses for this flag...

- In the node state machine, which prevents upcalls to the midlayer
  for port state changes and/or invoking scsi scan functions.

  As the driver must deal with FC nodes (remote ports) independently
  of whether the SCSI layer (via scsi_host) is active on the adapter,
  we need the functionality to block the scsi midlayer upcalls.
  I don't see how this belongs in the midlayer....

- Stops new i/o issued via queuecommand.

  The check here can probably be removed as long as there's a guarantee
  that there will be no queuecommand calls after scsi_remove_host
  completes. I believe this to be true. The only question I had
  concerns any in-progress scans when scsi_remove_host is called. Does
  remove host cancel the scans (vs just depending on reference counts) ?

>     - target removal shouldn't happen from your remove method but via
>       scsi_remove_host

Ok. Originally, this was thought to be more graceful than
scsi_remove_host (less console messages). However, testing shows no
discernable difference so we can easily do this.

>  o the driver would benefit a _lot_ from driver private data in
>    scsi_target, I think we should do that and basically get rid of
>    phba->device_queue_hash

Doable - although this now requires yet another midlayer patch to allow
host-private data to be allocated along with the target, and the 
addition of target alloc/destroy callback functions. Note that the
midlayer calling sequences for the scsi target, vs the scsi device,
are rather interesting right now (slave alloc called before a target
alloc would be called).

To be honest - I don't think the gain will be that large. There are
really only 5 elements that are target specific (e.g. lpfc_target),
3 of them stats counters. (Note: this has the assumption that nodes
are not combined with scsi targets).

device_queue_hash is used whenever we have to look up entries by
target id. In general, we used this mainly in the eh_xxx routines,
but I guess they can take a little longer to search for the node
associated to the target id.

>  o what about lpfc_target->rport?  This should probably better be in
>    the fc_target structure, no?

Anything in lpfc_target would go into the scsi_target hostdata
(not fc_target).

>  o lpfc_target vs lpfc_nodelist.  I'd see these merged into a single
>    structure.  In fact large parts of lpfc_nodelist should probably
>    move into the transport class.

?? lpfc_target would merge with scsi_target. lpfc_nodelist stays as is
(and independent from the scsi_target).

I disagree with moving the nodelist code (e.g. FC discovery state
machines) into the transport. My opinion is this where you cross into
an area that can be very hardware-dependent (some adapters do this
completely within the adapter). I know that in our case, almost 1/2 of
the states are induced by the way our driver must interact with our
hardware. At this point, I don't see any benefits to taking this step.

>  o the memory allocation for non-I/O path stilled looked 
> total overkill
>    to me, but it seems you're doing an awfull lot from the 
> interrupt handler
>    still.  Wouldn't it be easier to do much of it from process context
>    via the kernel thread?

Yes - we're still looking at the memory allocations outside of the i/o
path and expected to resolve it while addressing new comments (such as
these :).  There's a couple of issues we're addressing relative to
calls made in the interrupt handler, so - yes, well move much of this
over to the kernel thread.

-- James S

Re: [Announce] Emulex lpfcdriver v8.0.20 available

[Christoph Hellwig]

> >  o the host removal is still quite hackish and probably racy:
> >     - everything what's you're ding with FC_UNLOADING should 
> > be done in
> >       the midlayer where we already have a state machine for the
> >       Scsi_Host
> 
> There's only 2 uses for this flag...
> 
> - In the node state machine, which prevents upcalls to the midlayer
>   for port state changes and/or invoking scsi scan functions.
> 
>   As the driver must deal with FC nodes (remote ports) independently
>   of whether the SCSI layer (via scsi_host) is active on the adapter,
>   we need the functionality to block the scsi midlayer upcalls.
>   I don't see how this belongs in the midlayer....

The unloading is adapter-level, and Scsi_Host is a midlayer-managed
object.  So the scsi_scan_single_target you protected by FC_UNLOADING
will do the right thing called on a host that scsi_remove_host
has been called on.

For fc_remote_port_block/fc_remote_port_unblock/fc_remote_port_add
that should be true aswell, but as that's new code you've written
you should know it in more detail then me.

> - Stops new i/o issued via queuecommand.
> 
>   The check here can probably be removed as long as there's a guarantee
>   that there will be no queuecommand calls after scsi_remove_host
>   completes. I believe this to be true. The only question I had
>   concerns any in-progress scans when scsi_remove_host is called. Does
>   remove host cancel the scans (vs just depending on reference counts) ?

Currently it fails them, but doesn't cancel them in the sense of exiting
early.


> >     - target removal shouldn't happen from your remove method but via
> >       scsi_remove_host
> 
> Ok. Originally, this was thought to be more graceful than
> scsi_remove_host (less console messages). However, testing shows no
> discernable difference so we can easily do this.

What console messages do you see?  Looks like we'd have to add another
allowed state transition from BLOCKED?

> >  o the driver would benefit a _lot_ from driver private data in
> >    scsi_target, I think we should do that and basically get rid of
> >    phba->device_queue_hash
> 
> Doable - although this now requires yet another midlayer patch to allow
> host-private data to be allocated along with the target, and the 
> addition of target alloc/destroy callback functions. Note that the
> midlayer calling sequences for the scsi target, vs the scsi device,
> are rather interesting right now (slave alloc called before a target
> alloc would be called).

Yes, the allocation would need to be changed a little.  But hey, I don't
think we should block merging the driver on this one.  We'll probably
introduce it soon and I'll sign you up for testing that patch on the
lpfc drivewr when it's ready.

> >  o what about lpfc_target->rport?  This should probably better be in
> >    the fc_target structure, no?
> 
> Anything in lpfc_target would go into the scsi_target hostdata
> (not fc_target).

I meant fc_target_attrs actually, which is scsi_target transport-data.

The point here is that the rport is a transport-class transport object
that every driver fully converted to fc_transport needs, so it's better
in transport data aswell.

Now looking at lpfc_target given that the rport goes away we only
have the I/O counters left and the nodlist pointer.  The I/O counters
aren't really a driver thing and should probably move to the midlayer
which only leaves the nodelist as private data in the scsi_device.

Sounds like the way to go?

> >  o lpfc_target vs lpfc_nodelist.  I'd see these merged into a single
> >    structure.  In fact large parts of lpfc_nodelist should probably
> >    move into the transport class.
> 
> ?? lpfc_target would merge with scsi_target. lpfc_nodelist stays as is
> (and independent from the scsi_target).
 
> I disagree with moving the nodelist code (e.g. FC discovery state
> machines) into the transport. My opinion is this where you cross into
> an area that can be very hardware-dependent (some adapters do this
> completely within the adapter). I know that in our case, almost 1/2 of
> the states are induced by the way our driver must interact with our
> hardware. At this point, I don't see any benefits to taking this step.

OK.  I've just taken a short look at the state machine and those bits
I looked at looked generic enough.

RE: [Announce] Emulex lpfcdriver v8.0.20 available

[James.Smart]

> What console messages do you see?  Looks like we'd have to add another
> allowed state transition from BLOCKED?

At one point we were seeing 
   Synchronizing SCSI cache for disk sdc:
    FAILED
      status = 0, message = 00, host = 1, driver = 00
      <5>
messages. However, at this point, the sequencing in the driver for unload,
scsi_host_removal, and datastructure teardown is such that we are avoiding
this condition and removals are clean.

No changes are needed.

> Yes, the allocation would need to be changed a little.  But 
> hey, I don't
> think we should block merging the driver on this one.  We'll probably
> introduce it soon and I'll sign you up for testing that patch on the
> lpfc drivewr when it's ready.

Reasonable.

> > >  o what about lpfc_target->rport?  This should probably 
> better be in
> > >    the fc_target structure, no?
> > 
> > Anything in lpfc_target would go into the scsi_target hostdata
> > (not fc_target).
> 
> I meant fc_target_attrs actually, which is scsi_target transport-data.
> 
> The point here is that the rport is a transport-class transport object
> that every driver fully converted to fc_transport needs, so 
> it's better
> in transport data as well.

If I understand you - it's the recommendation that the remote port be
part of the fc_target transport data.  I disagree. If anything, it would
be the other way around. The port exists before any scsi target (thus it
can't be part of the target allocation). The port can also exist without
being a scsi target, so there's no scsi target structure at all.

> 
> Now looking at lpfc_target given that the rport goes away we only
> have the I/O counters left and the nodlist pointer.  The I/O counters
> aren't really a driver thing and should probably move to the midlayer
> which only leaves the nodelist as private data in the scsi_device.
> 
> Sounds like the way to go?

Nope. The counters are a scsi-thing (not fc-thing), so they make sense
to go into the scsi_target. If you want to make them more general for
all drivers, we can do this. Right now, I wouldn't bother.

-- james s

Re: [Announce] Emulex lpfcdriver v8.0.20 available

[Christoph Hellwig]

On Thu, Jan 20, 2005 at 10:27:54AM -0500, James.Smart@Emulex.Com wrote:
> > What console messages do you see?  Looks like we'd have to add another
> > allowed state transition from BLOCKED?
> 
> At one point we were seeing 
>    Synchronizing SCSI cache for disk sdc:
>     FAILED
>       status = 0, message = 00, host = 1, driver = 00
>       <5>
> messages. However, at this point, the sequencing in the driver for unload,
> scsi_host_removal, and datastructure teardown is such that we are avoiding
> this condition and removals are clean.

That was a bug in the scsi device statemachine and showed up on all hosts,
it has indeed been fixed for while.

> > I meant fc_target_attrs actually, which is scsi_target transport-data.
> > 
> > The point here is that the rport is a transport-class transport object
> > that every driver fully converted to fc_transport needs, so 
> > it's better
> > in transport data as well.
> 
> If I understand you - it's the recommendation that the remote port be
> part of the fc_target transport data.  I disagree. If anything, it would
> be the other way around. The port exists before any scsi target (thus it
> can't be part of the target allocation). The port can also exist without
> being a scsi target, so there's no scsi target structure at all.

No, sorry for have been unclear.  I want a pointer to the remote port
structure in the fc_target transport data.  Every driver needs the
linkage of these two, and instead of everyone having it in their
private data we'd better move it to the fc_transport data structure.

> > Now looking at lpfc_target given that the rport goes away we only
> > have the I/O counters left and the nodlist pointer.  The I/O counters
> > aren't really a driver thing and should probably move to the midlayer
> > which only leaves the nodelist as private data in the scsi_device.
> > 
> > Sounds like the way to go?
> 
> Nope. The counters are a scsi-thing (not fc-thing), so they make sense
> to go into the scsi_target.

That's what I meant.  In fact that way it's exposed in a driver-private
attributes isn't good at all.  What about dropping the counters temporarily
for submission and submit a patch later that adds them (and their sysfs
exposure) back at the midlayer?  In fact the actual accounting is better
done at the midlayer-level anyway.

RE: [Announce] Emulex lpfcdriver v8.0.20 available

[James.Smart]

> No, sorry for have been unclear.  I want a pointer to the remote port
> structure in the fc_target transport data.  Every driver needs the
> linkage of these two, and instead of everyone having it in their
> private data we'd better move it to the fc_transport data structure.

Ok. It's already there. What this boils down do is exactly your last
statement (and our driver doesn't do it - we'll fix it).

Note: there's a flipside here... with the remote port stuff, we had
removed almost all the reasons for a fc driver to reference a scsi_target.
The only place we really had to was in the case the older fc_transport/xxx
sysfs attributes (which all but one exist only for compatibility).

We could move this into the remote port structure (already set up for this)
which will always exist for a scsi_target. Even so, I think extending the
scsi target and fixing the calling sequence is the correct thing to do.

>  What about dropping the 
> counters temporarily
> for submission and submit a patch later that adds them (and 
> their sysfs
> exposure) back at the midlayer?  In fact the actual 
> accounting is better
> done at the midlayer-level anyway.

OK.

-- james s

Re: [Announce] Emulex lpfcdriver v8.0.20 available

[Christoph Hellwig]

On Thu, Jan 20, 2005 at 10:54:47AM -0500, James.Smart@Emulex.Com wrote:
> > No, sorry for have been unclear.  I want a pointer to the remote port
> > structure in the fc_target transport data.  Every driver needs the
> > linkage of these two, and instead of everyone having it in their
> > private data we'd better move it to the fc_transport data structure.
> 
> Ok. It's already there. What this boils down do is exactly your last
> statement (and our driver doesn't do it - we'll fix it).
> 
> Note: there's a flipside here... with the remote port stuff, we had
> removed almost all the reasons for a fc driver to reference a scsi_target.
> The only place we really had to was in the case the older fc_transport/xxx
> sysfs attributes (which all but one exist only for compatibility).
> 
> We could move this into the remote port structure (already set up for this)
> which will always exist for a scsi_target. Even so, I think extending the
> scsi target and fixing the calling sequence is the correct thing to do.

This sounds like a good idea, but I wonder if we have userland relying
on those attributes already.  But given that the fc transport class
was mostly a joke before you encehanced it it might be worth breaking
it..

RE: [Announce] Emulex lpfcdriver v8.0.20 available

[James.Smart]

> This sounds like a good idea, but I wonder if we have userland relying
> on those attributes already.  But given that the fc transport class
> was mostly a joke before you encehanced it it might be worth breaking
> it..

You and James would have to give the approval for this one. I already
encountered issues with these attributes when then went from scsi_device
to scsi_target. The most significant hiccup was to udev and the projects
depending on it... Also, given the potential amount of change may be
needed to move to rports, I wouldn't want to pull the rug out from under
the other drivers until they are ready...

-- james s

Re: [Announce] Emulex lpfcdriver v8.0.20 available

[Christoph Hellwig]

On Thu, Jan 20, 2005 at 11:26:00AM -0500, James.Smart@Emulex.Com wrote:
> > This sounds like a good idea, but I wonder if we have userland relying
> > on those attributes already.  But given that the fc transport class
> > was mostly a joke before you encehanced it it might be worth breaking
> > it..
> 
> You and James would have to give the approval for this one. I already
> encountered issues with these attributes when then went from scsi_device
> to scsi_target. The most significant hiccup was to udev and the projects
> depending on it... Also, given the potential amount of change may be
> needed to move to rports, I wouldn't want to pull the rug out from under
> the other drivers until they are ready...

James just returned from a long journey and he hasn't read the thread yet,
but after a short discussion we agree that it's better to move the attributes
over to the rnode.  If we're seeing compatiblity problems we'll have to
add symlinks.

> 
> 
> -- james s
---end quoted text---

RE: [Announce] Emulex lpfcdriver v8.0.20 available

[James.Smart]

I can easily disable the them. However, I won't sign up for dealing
with this change in the other FC drivers as it could be rather
extensive. A patch to back out the attributes would disable FC
transport support in the other drivers. Their maintainers would need
to re-add support.

-- james s


Christoph Hellwig wrote:
> On Thu, Jan 20, 2005 at 11:26:00AM -0500, 
> James.Smart@Emulex.Com wrote:
> > > This sounds like a good idea, but I wonder if we have 
> userland relying
> > > on those attributes already.  But given that the fc 
> transport class
> > > was mostly a joke before you encehanced it it might be 
> worth breaking
> > > it..
> > 
> > You and James would have to give the approval for this one. 
> I already
> > encountered issues with these attributes when then went 
> from scsi_device
> > to scsi_target. The most significant hiccup was to udev and 
> the projects
> > depending on it... Also, given the potential amount of change may be
> > needed to move to rports, I wouldn't want to pull the rug 
> out from under
> > the other drivers until they are ready...
> 
> James just returned from a long journey and he hasn't read 
> the thread yet,
> but after a short discussion we agree that it's better to 
> move the attributes
> over to the rnode.  If we're seeing compatiblity problems 
> we'll have to
> add symlinks.