Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[Jeff Garzik]

Brian King wrote:
> Currently, ipr does not support HDIO_GET_IDENTITY to SATA devices.
> An oops occurs if userspace attempts to send the command. Since hald
> issues the command, ensure we fail the ioctl in ipr. This is a
> temporary solution to the oops. Once the ipr libata EH conversion
> is upstream, ipr will fully support HDIO_GET_IDENTITY.
> 
> Tested-by: Milton Miller <miltonm@bga.com>
> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
> ---
> 
>  linux-2.6-bjking1/drivers/scsi/ipr.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

I was pretty happy with the rough draft of your EH conversion...  where 
does that stand now?

	Jeff

Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[Brian King]

Jeff Garzik wrote:
> Brian King wrote:
>> Currently, ipr does not support HDIO_GET_IDENTITY to SATA devices.
>> An oops occurs if userspace attempts to send the command. Since hald
>> issues the command, ensure we fail the ioctl in ipr. This is a
>> temporary solution to the oops. Once the ipr libata EH conversion
>> is upstream, ipr will fully support HDIO_GET_IDENTITY.
>>
>> Tested-by: Milton Miller <miltonm@bga.com>
>> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
>> ---
>>
>>  linux-2.6-bjking1/drivers/scsi/ipr.c |    6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> I was pretty happy with the rough draft of your EH conversion...  where 
> does that stand now?

Unfortunately, it sits in the pile of things I haven't had time to
work on... There are two issues that I need to resolve before it can
replace what is currently in mainline.

1. Since freeze() cannot be implemented like on a SATA HBA, this causes
   problems on port removal. Current removal procedure assumes it can just
   freeze the port and free up all command/DMA resources. Need to abort
   outstanding ops for SAS. This should be solvable with some libata-eh
   changes.

2. dev_res allocated memory (ata_port) gets freed on rphy_device_del,
   rather than on the last put, which causes ata_port to get freed
   too soon. Possible solution would be to free dev_res allocated memory
   on the last put rather than on device_del.

The second problem was the one I got stuck on. Perhaps Tejun can chime in
here... Would it be possible to change dev_res to free allocated memory
on release rather than delete time?

There is also the issue of HBA queue limits. Since each SATA rphy uses its
own SCSI host, we lost host queue limit enforcement. I've implemented a
request_limit in ipr in order to solve this for ipr, but this really
does not scale well to lots of SATA devices, so we would probably need
queue groups at the block level before moving libsas to the new API.

-Brian

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[James Bottomley]

On Mon, 2008-06-02 at 11:27 -0500, Brian King wrote:
> There is also the issue of HBA queue limits. Since each SATA rphy uses
> its
> own SCSI host, we lost host queue limit enforcement. I've implemented
> a
> request_limit in ipr in order to solve this for ipr, but this really
> does not scale well to lots of SATA devices, so we would probably need
> queue groups at the block level before moving libsas to the new API.

But this is the point that principally illustrates the problems.  We
really need one SCSI host per physical device, not per phy because most
multi-phy devices have a hard per device queue limit.

The problems seem to come at us from the 1:1 relationship between a
'port' and a host in libata.  Is there no way we can perhaps recast what
we think of as a phy as what libata thinks of as a link and thus keep us
at one host per actual physical device?

James

Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[Jeff Garzik]

James Bottomley wrote:
> The problems seem to come at us from the 1:1 relationship between a
> 'port' and a host in libata.  Is there no way we can perhaps recast what
> we think of as a phy as what libata thinks of as a link and thus keep us
> at one host per actual physical device?


It's difficult to do that universally, but OTOH it would be quite 
reasonable to do that for modern SATA controllers, leaving the older 
master/slave controllers as one-host-per-port.

I've been wanting to do that for a while, even.

The main thing holding me back is not any technical issue, but desire to 
avoid breakage caused by abrupt topology change.

	Jeff

Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[James Bottomley]

On Mon, 2008-06-02 at 13:43 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > The problems seem to come at us from the 1:1 relationship between a
> > 'port' and a host in libata.  Is there no way we can perhaps recast what
> > we think of as a phy as what libata thinks of as a link and thus keep us
> > at one host per actual physical device?
> 
> 
> It's difficult to do that universally, but OTOH it would be quite 
> reasonable to do that for modern SATA controllers, leaving the older 
> master/slave controllers as one-host-per-port.
> 
> I've been wanting to do that for a while, even.
> 
> The main thing holding me back is not any technical issue, but desire to 
> avoid breakage caused by abrupt topology change.

Actually, that's not quite what I was suggesting, but actually it would
be better since then the ata topology will map easily into what sas
wants.

The quickest way seems to be to break the scsi_host <-> ata_port link by
mapping scsi_host <-> ata_host instead.  Legacy controllers with only a
single port can keep the apparent 1:1 mapping (we can even keep the
hostdata stuff).  Unfortunately, the standard way of doing this is via
the transport classes, but as long as you have a pointer from the port
to the host and from the device to the port (which you do) it should be
possible.

The downside I can see is that qc_defer handling changes non trivially
because of this, but there don't seem to be many other issues.

The ideal (for us at least) would be to completely separate port
operations from host operations, because libsas really wants to control
the host and attach the port only to SATA devices.

James

Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[Jeff Garzik]

James Bottomley wrote:
> The quickest way seems to be to break the scsi_host <-> ata_port link by
> mapping scsi_host <-> ata_host instead.

Correct for newer controllers... not desirable for older master/slave


> Legacy controllers with only a
> single port can keep the apparent 1:1 mapping (we can even keep the
> hostdata stuff).

Not sure what you mean by this


> Unfortunately, the standard way of doing this is via
> the transport classes, but as long as you have a pointer from the port
> to the host and from the device to the port (which you do) it should be
> possible.

I've always been open to an ATA transport class and thought that was the 
best way to go long term.  But I confess to not knowing the best way to 
implement that goal in concrete terms.


> The downside I can see is that qc_defer handling changes non trivially
> because of this, but there don't seem to be many other issues.

There are a lot of little details that are easily fixable.

The main detail is not breaking queueing for master/slave. 
Master/slave, if you recall, _requires_ one-shost-per-port because it 
relies on scsi queueing to handle the balancing between master/slave. 
We set can_queue to 1, and for the 2-device case -- master and slave 
present -- SCSI does the heavy lifting to ensure proper arbitration.

Adding support for NCQ and qc_defer made it a bit easier to change the 
master/slave setup, though.

The other reason why we use one-short-per-port on master/slave is that, 
in many respects, each legacy IDE port really can behave like a 
completely separate controller, each port with its own interrupts and 
independent reset logic.

Thus ATA really has two models:  1-port-1-host and N-ports-1-host.

[meta:  I'm not disagreeing with you here, just explaining how all this 
came about...]


> The ideal (for us at least) would be to completely separate port
> operations from host operations, because libsas really wants to control
> the host and attach the port only to SATA devices.

Agreed, I've wanted to move in that direction for a while.

	Jeff

Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[Brian King]

Jeff Garzik wrote:
> The other reason why we use one-short-per-port on master/slave is that, 
> in many respects, each legacy IDE port really can behave like a 
> completely separate controller, each port with its own interrupts and 
> independent reset logic.
> 
> Thus ATA really has two models:  1-port-1-host and N-ports-1-host.

One thing that having a per-port SCSI host buys us is that it isolates
EH to the port rather than the entire SCSI host. The way that the new
libata EH is designed, it can end up spending a fair amount of time
in EH and may even sleep for seconds at a time, waiting for the device
to come ready. Quiescing all devices on a SAS host due to a single
SATA device gets *very* painful. The new libata EH does much more than
just EH. It wakes up for every ATAPI check condition...

Before we can move to a single SCSI host model, we will
have to make some changes regarding how EH is done in libata...

-Brian

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[Jeff Garzik]

Brian King wrote:
> One thing that having a per-port SCSI host buys us is that it isolates
> EH to the port rather than the entire SCSI host. The way that the new
> libata EH is designed, it can end up spending a fair amount of time
> in EH and may even sleep for seconds at a time, waiting for the device
> to come ready. Quiescing all devices on a SAS host due to a single
> SATA device gets *very* painful. The new libata EH does much more than
> just EH. It wakes up for every ATAPI check condition...
> 
> Before we can move to a single SCSI host model, we will
> have to make some changes regarding how EH is done in libata...


Good points.  There definitely needs to be a discussion about how to 
integrate libata EH with libsas model and ipr model (which are distinct, 
if similar).

libata EH was coded to largely be "in the driver's seat", which isn't 
true in a world where it is a peer to SAS devices.  Also libata EH 
duplicates some of the link EH stuff that SAS deals with, and must 
subsume (since libsas knows about both SAS and SATA phys, and an ideal 
world doesn't have the two as separate as they are now).

	Jeff

Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[James Bottomley]

On Mon, 2008-06-02 at 14:54 -0400, Jeff Garzik wrote:
> Brian King wrote:
> > One thing that having a per-port SCSI host buys us is that it isolates
> > EH to the port rather than the entire SCSI host. The way that the new
> > libata EH is designed, it can end up spending a fair amount of time
> > in EH and may even sleep for seconds at a time, waiting for the device
> > to come ready. Quiescing all devices on a SAS host due to a single
> > SATA device gets *very* painful. The new libata EH does much more than
> > just EH. It wakes up for every ATAPI check condition...
> > 
> > Before we can move to a single SCSI host model, we will
> > have to make some changes regarding how EH is done in libata...
> 
> 
> Good points.  There definitely needs to be a discussion about how to 
> integrate libata EH with libsas model and ipr model (which are distinct, 
> if similar).

Actually, libsas completely hijacks the strategy handler (as I believe
libata does), so it really no longer has a host dependency, except for
the one thread per host thing.  Most of what it does (unless it thinks
the actual host chip is wedged) is port (as in SAS port, not phy) based,
so I think we have the capability for doing this already.

> libata EH was coded to largely be "in the driver's seat", which isn't 
> true in a world where it is a peer to SAS devices.  Also libata EH 
> duplicates some of the link EH stuff that SAS deals with, and must 
> subsume (since libsas knows about both SAS and SATA phys, and an ideal 
> world doesn't have the two as separate as they are now).

Yes ... there's even nastier scenarios.  If we're in a STP (SATA Tunnel
Protocol) situation, the device at the end is SATA, but the pathway to
get to it through the port is SAS via expanders.  In that situation the
two have to co-operate on the recovery ... of course, this isn't
incredibly dissimilar from port multipliers now.

James

Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[Brian King]

James Bottomley wrote:
> On Mon, 2008-06-02 at 14:54 -0400, Jeff Garzik wrote:
>> Brian King wrote:
>>> One thing that having a per-port SCSI host buys us is that it isolates
>>> EH to the port rather than the entire SCSI host. The way that the new
>>> libata EH is designed, it can end up spending a fair amount of time
>>> in EH and may even sleep for seconds at a time, waiting for the device
>>> to come ready. Quiescing all devices on a SAS host due to a single
>>> SATA device gets *very* painful. The new libata EH does much more than
>>> just EH. It wakes up for every ATAPI check condition...
>>>
>>> Before we can move to a single SCSI host model, we will
>>> have to make some changes regarding how EH is done in libata...
>>
>> Good points.  There definitely needs to be a discussion about how to 
>> integrate libata EH with libsas model and ipr model (which are distinct, 
>> if similar).
> 
> Actually, libsas completely hijacks the strategy handler (as I believe
> libata does), so it really no longer has a host dependency, except for
> the one thread per host thing.  Most of what it does (unless it thinks
> the actual host chip is wedged) is port (as in SAS port, not phy) based,
> so I think we have the capability for doing this already.

libsas currently does hijack the strategy handler today, but if we want
to move it to the new libata EH/attach model, that's not going to work.
libata EH does much more than EH. Its really more of an exception handler
than an error handler. Anything that happens out of the ordinary wakes
the EH thread.

Ideally, I think we really want to have stacked EH as I believe we have
discussed in the past. If we hit an error, we quiesce the device, do device
level EH. If needed, quiesce the device group / port, perform EH. If 
necessary, quiesce the entire HBA and perform EH on it.

-Brian

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[James Bottomley]

On Mon, 2008-06-02 at 14:11 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > The quickest way seems to be to break the scsi_host <-> ata_port link by
> > mapping scsi_host <-> ata_host instead.
> 
> Correct for newer controllers... not desirable for older master/slave
> 
> 
> > Legacy controllers with only a
> > single port can keep the apparent 1:1 mapping (we can even keep the
> > hostdata stuff).
> 
> Not sure what you mean by this

I think it's possible to do the shift while still making it look to
single port hosts that nothing has changed

> > Unfortunately, the standard way of doing this is via
> > the transport classes, but as long as you have a pointer from the port
> > to the host and from the device to the port (which you do) it should be
> > possible.
> 
> I've always been open to an ATA transport class and thought that was the 
> best way to go long term.  But I confess to not knowing the best way to 
> implement that goal in concrete terms.

Perhaps it is best to approach it from what libsas needs, that way the
structures can be made to map to each other.

> > The downside I can see is that qc_defer handling changes non trivially
> > because of this, but there don't seem to be many other issues.
> 
> There are a lot of little details that are easily fixable.
> 
> The main detail is not breaking queueing for master/slave. 
> Master/slave, if you recall, _requires_ one-shost-per-port because it 
> relies on scsi queueing to handle the balancing between master/slave. 
> We set can_queue to 1, and for the 2-device case -- master and slave 
> present -- SCSI does the heavy lifting to ensure proper arbitration.

Well, we can be elastic on this.  The common case is a two port
master/slave, isn't it.  This particular one could just be treated as
two separate interfaces and hence two hosts (it's reasonably analagous
to the 53c896---which is a single chip that presents two SCSI channels
as two PCI functions).

> Adding support for NCQ and qc_defer made it a bit easier to change the 
> master/slave setup, though.
> 
> The other reason why we use one-short-per-port on master/slave is that, 
> in many respects, each legacy IDE port really can behave like a 
> completely separate controller, each port with its own interrupts and 
> independent reset logic.
> 
> Thus ATA really has two models:  1-port-1-host and N-ports-1-host.
> 
> [meta:  I'm not disagreeing with you here, just explaining how all this 
> came about...]

OK ... so how bad would it be to maintain two attachment models for the
two cases?  The architect in me says yuk because special casing like
this always leads to complexity, but I can't see a simpler way of doing
it.

James

Re: [PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[Alan Cox]

> I think it's possible to do the shift while still making it look to
> single port hosts that nothing has changed

There are very few of those. Your generic PC controller is single host,
dual channel two devices per channel.

> Well, we can be elastic on this.  The common case is a two port
> master/slave, isn't it.  This particular one could just be treated as
> two separate interfaces and hence two hosts (it's reasonably analagous
> to the 53c896---which is a single chip that presents two SCSI channels
> as two PCI functions).

Not as trivial as you might hope. In some cases the two are closely
linked, they often share timing rules. In other cases they have shared or
single assignable DMA fifos.

In practical terms that means you must reconfigure *both* channels when
you do an EH.

Alan

[PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[Brian King]

Currently, ipr does not support HDIO_GET_IDENTITY to SATA devices.
An oops occurs if userspace attempts to send the command. Since hald
issues the command, ensure we fail the ioctl in ipr. This is a
temporary solution to the oops. Once the ipr libata EH conversion
is upstream, ipr will fully support HDIO_GET_IDENTITY.

Tested-by: Milton Miller <miltonm@bga.com>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

 linux-2.6-bjking1/drivers/scsi/ipr.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff -puN drivers/scsi/ipr.c~ipr_sata_no_identify drivers/scsi/ipr.c
--- linux-2.6/drivers/scsi/ipr.c~ipr_sata_no_identify	2008-06-02 08:46:19.000000000 -0500
+++ linux-2.6-bjking1/drivers/scsi/ipr.c	2008-06-02 08:46:19.000000000 -0500
@@ -71,6 +71,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/libata.h>
+#include <linux/hdreg.h>
 #include <asm/io.h>
 #include <asm/irq.h>
 #include <asm/processor.h>
@@ -4913,8 +4914,11 @@ static int ipr_ioctl(struct scsi_device 
 	struct ipr_resource_entry *res;
 
 	res = (struct ipr_resource_entry *)sdev->hostdata;
-	if (res && ipr_is_gata(res))
+	if (res && ipr_is_gata(res)) {
+		if (cmd == HDIO_GET_IDENTITY)
+			return -ENOTTY;
 		return ata_scsi_ioctl(sdev, cmd, arg);
+	}
 
 	return -EINVAL;
 }
_

[PATCH 1/1] ipr: Fix HDIO_GET_IDENTITY oops for SATA devices

[Brian King]

Currently, ipr does not support HDIO_GET_IDENTITY to SATA devices.
An oops occurs if userspace attempts to send the command. Since hald
issues the command, ensure we fail the ioctl in ipr. This is a
temporary solution to the oops. Once the ipr libata EH conversion
is upstream, ipr will fully support HDIO_GET_IDENTITY.

Tested-by: Milton Miller <miltonm@bga.com>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

 linux-2.6-bjking1/drivers/scsi/ipr.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff -puN drivers/scsi/ipr.c~ipr_sata_no_identify drivers/scsi/ipr.c
--- linux-2.6/drivers/scsi/ipr.c~ipr_sata_no_identify	2008-06-02 08:46:19.000000000 -0500
+++ linux-2.6-bjking1/drivers/scsi/ipr.c	2008-06-02 08:46:19.000000000 -0500
@@ -71,6 +71,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/libata.h>
+#include <linux/hdreg.h>
 #include <asm/io.h>
 #include <asm/irq.h>
 #include <asm/processor.h>
@@ -4913,8 +4914,11 @@ static int ipr_ioctl(struct scsi_device 
 	struct ipr_resource_entry *res;
 
 	res = (struct ipr_resource_entry *)sdev->hostdata;
-	if (res && ipr_is_gata(res))
+	if (res && ipr_is_gata(res)) {
+		if (cmd == HDIO_GET_IDENTITY)
+			return -ENOTTY;
 		return ata_scsi_ioctl(sdev, cmd, arg);
+	}
 
 	return -EINVAL;
 }
_