Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

2008-02-14 Thread Mike Christie

James Bottomley wrote:

On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote:

iscsi_iser does not have any hardware DMA restrictions.  Add a
slave_configure function to remove any DMA alignment restriction,
allowing the use of direct IO from arbitrary offsets within a page.
Also disable page bouncing; iser has no restrictions on which pages it
can address.

Signed-off-by: Pete Wyckoff [EMAIL PROTECTED]
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |8 
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c 
b/drivers/infiniband/ulp/iser/iscsi_iser.c
index be1b9fb..1b272a6 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
iser_conn_terminate(ib_conn);
 }
 
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)

+{
+   blk_queue_bounce_limit(sdev-request_queue, BLK_BOUNCE_ANY);


You really don't want to do this.  That signals to the block layer that
we have an iommu, although it's practically the same thing as a 64 bit
DMA mask ... but I'd just leave it to the DMA mask to set this up
correctly.  Anything else is asking for a subtle bug to turn up years
from now when something causes the mask and the limit to be mismatched.



I thought BLK_BOUNCE_ANY just meant don't bounce anything (that was 
from the blkdev.h comments). We used it for iscsi_tcp because the 
network layer can take any type of page and will do the right thing for 
the hardware it eventually gets sent to.

-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

2008-02-14 Thread Mike Christie

Mike Christie wrote:

James Bottomley wrote:

On Thu, 2008-02-14 at 11:56 -0600, Mike Christie wrote:

You really don't want to do this.  That signals to the block layer that
we have an iommu, although it's practically the same thing as a 64 bit
DMA mask ... but I'd just leave it to the DMA mask to set this up
correctly.  Anything else is asking for a subtle bug to turn up years
from now when something causes the mask and the limit to be mismatched.

I thought BLK_BOUNCE_ANY just meant don't bounce anything (that was 
from the blkdev.h comments). 


It does ... that's why it's used in the IOMMU case ... and why it's
practically the same as a 64 bit mask.


We used it for iscsi_tcp because the network layer can take any type
of page and will do the right thing for the hardware it eventually
gets sent to.


Right, to you it means never bounce because net wants to do it instead.
However, I don't think that's the case for iSER, is it? ... as in, if


I will leave that to Roland and them :)

My only concern with a simple patch to use the ib interface's dma values 
would be, if there is some event that causes us to start using inteface 
ib0 then switch to ib1 (maybe like some sort of route table change if 
that is possible), then we will have to loop over all the scsi devices's 
queues and update the dma values. And if there is IO already queued then 
would we have to possible rebuild those commands if something like the 
dma_mask changed?




I guess we can just handle this like how FC handles the case when the 
dev loss tmo expires, but then the rport comes back later.

-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

2008-02-14 Thread James Bottomley
On Thu, 2008-02-14 at 11:56 -0600, Mike Christie wrote:
  You really don't want to do this.  That signals to the block layer that
  we have an iommu, although it's practically the same thing as a 64 bit
  DMA mask ... but I'd just leave it to the DMA mask to set this up
  correctly.  Anything else is asking for a subtle bug to turn up years
  from now when something causes the mask and the limit to be mismatched.
  
 
 I thought BLK_BOUNCE_ANY just meant don't bounce anything (that was 
 from the blkdev.h comments). 

It does ... that's why it's used in the IOMMU case ... and why it's
practically the same as a 64 bit mask.

 We used it for iscsi_tcp because the network layer can take any type
 of page and will do the right thing for the hardware it eventually
 gets sent to.

Right, to you it means never bounce because net wants to do it instead.
However, I don't think that's the case for iSER, is it? ... as in, if
I've got the pathway tracing correct, it just goes to the infiniband
device and gets mapped there.  If there's a mask mismatch (very
unlikely, I know) we get a very subtle and hard to trace error.

James


-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

2008-02-14 Thread Mike Christie

James Bottomley wrote:

On Thu, 2008-02-14 at 11:56 -0600, Mike Christie wrote:

You really don't want to do this.  That signals to the block layer that
we have an iommu, although it's practically the same thing as a 64 bit
DMA mask ... but I'd just leave it to the DMA mask to set this up
correctly.  Anything else is asking for a subtle bug to turn up years
from now when something causes the mask and the limit to be mismatched.

I thought BLK_BOUNCE_ANY just meant don't bounce anything (that was 
from the blkdev.h comments). 


It does ... that's why it's used in the IOMMU case ... and why it's
practically the same as a 64 bit mask.


We used it for iscsi_tcp because the network layer can take any type
of page and will do the right thing for the hardware it eventually
gets sent to.


Right, to you it means never bounce because net wants to do it instead.
However, I don't think that's the case for iSER, is it? ... as in, if


I will leave that to Roland and them :)

My only concern with a simple patch to use the ib interface's dma values 
would be, if there is some event that causes us to start using inteface 
ib0 then switch to ib1 (maybe like some sort of route table change if 
that is possible), then we will have to loop over all the scsi devices's 
queues and update the dma values. And if there is IO already queued then 
would we have to possible rebuild those commands if something like the 
dma_mask changed?




I've got the pathway tracing correct, it just goes to the infiniband
device and gets mapped there.  If there's a mask mismatch (very
unlikely, I know) we get a very subtle and hard to trace error.


-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

2008-02-14 Thread Mike Christie

Mike Christie wrote:

Mike Christie wrote:

James Bottomley wrote:

On Thu, 2008-02-14 at 11:56 -0600, Mike Christie wrote:
You really don't want to do this.  That signals to the block layer 
that

we have an iommu, although it's practically the same thing as a 64 bit
DMA mask ... but I'd just leave it to the DMA mask to set this up
correctly.  Anything else is asking for a subtle bug to turn up years
from now when something causes the mask and the limit to be 
mismatched.


I thought BLK_BOUNCE_ANY just meant don't bounce anything (that 
was from the blkdev.h comments). 


It does ... that's why it's used in the IOMMU case ... and why it's
practically the same as a 64 bit mask.


We used it for iscsi_tcp because the network layer can take any type
of page and will do the right thing for the hardware it eventually
gets sent to.


Right, to you it means never bounce because net wants to do it instead.
However, I don't think that's the case for iSER, is it? ... as in, if


I will leave that to Roland and them :)


Ooops, I misread your mail. I think you are right and for iser we need 
to use the ib devices dma values.


For some reason I was thinking you were asking if there was a different 
interface which abstracted all that away for us like with iscsi_tcp.




My only concern with a simple patch to use the ib interface's dma 
values would be, if there is some event that causes us to start using 
inteface ib0 then switch to ib1 (maybe like some sort of route table 
change if that is possible), then we will have to loop over all the 
scsi devices's queues and update the dma values. And if there is IO 
already queued then would we have to possible rebuild those commands 
if something like the dma_mask changed?




I guess we can just handle this like how FC handles the case when the 
dev loss tmo expires, but then the rport comes back later.


Yuck, I guess we would have to do something like this if ib0 and ib1 had 
different dma restrictions.

-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

2008-02-13 Thread Pete Wyckoff
[EMAIL PROTECTED] wrote on Tue, 12 Feb 2008 15:57 -0600:
 On Tue, 2008-02-12 at 16:46 -0500, Pete Wyckoff wrote:
  [EMAIL PROTECTED] wrote on Tue, 12 Feb 2008 15:10 -0600:
   On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote:
iscsi_iser does not have any hardware DMA restrictions.  Add a
slave_configure function to remove any DMA alignment restriction,
allowing the use of direct IO from arbitrary offsets within a page.
Also disable page bouncing; iser has no restrictions on which pages it
can address.

Signed-off-by: Pete Wyckoff [EMAIL PROTECTED]
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |8 
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c 
b/drivers/infiniband/ulp/iser/iscsi_iser.c
index be1b9fb..1b272a6 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
iser_conn_terminate(ib_conn);
 }
 
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+   blk_queue_bounce_limit(sdev-request_queue, BLK_BOUNCE_ANY);
   
   You really don't want to do this.  That signals to the block layer that
   we have an iommu, although it's practically the same thing as a 64 bit
   DMA mask ... but I'd just leave it to the DMA mask to set this up
   correctly.  Anything else is asking for a subtle bug to turn up years
   from now when something causes the mask and the limit to be mismatched.
  
  Oh.  I decided to add that line for symmetry with TCP, and was
  convinced by the arguments here:
  
  commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56
  Author: Mike Christie [EMAIL PROTECTED]
  Date:   Thu Jul 26 12:46:47 2007 -0500
  
  [SCSI] iscsi_tcp: Turn off bounce buffers
  
  It was found by LSI that on setups with large amounts of memory
  we were bouncing buffers when we did not need to. If the iscsi tcp
  code touches the data buffer (or a helper does),
  it will kmap the buffer. iscsi_tcp also does not interact with hardware,
  so it does not have any hw dma restrictions. This patch sets the bounce
  buffer settings for our device queue so buffers should not be bounced
  because of a driver limit.
  
  I don't see a convenient place to callback into particular iscsi
  devices to set the DMA mask per-host.  It has to go on the
  shost_gendev, right?, but only for TCP and iSER, not qla4xxx, which
  handles its DMA mask during device probe.
 
 You should be taking your mask from the underlying infiniband device as
 part of the setup, shouldn't you?

I think you're right about this.  All the existing IB HW tries to
set a 64-bit dma mask, but that's no reason to disable the mechanism
entirely in iser.  I'll remove that line that disables bouncing in
my patch.  Perhaps Mike will know if the iscsi_tcp usage is still
appropriate.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

2008-02-12 Thread Pete Wyckoff
[EMAIL PROTECTED] wrote on Tue, 12 Feb 2008 15:10 -0600:
 On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote:
  iscsi_iser does not have any hardware DMA restrictions.  Add a
  slave_configure function to remove any DMA alignment restriction,
  allowing the use of direct IO from arbitrary offsets within a page.
  Also disable page bouncing; iser has no restrictions on which pages it
  can address.
  
  Signed-off-by: Pete Wyckoff [EMAIL PROTECTED]
  ---
   drivers/infiniband/ulp/iser/iscsi_iser.c |8 
   1 files changed, 8 insertions(+), 0 deletions(-)
  
  diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c 
  b/drivers/infiniband/ulp/iser/iscsi_iser.c
  index be1b9fb..1b272a6 100644
  --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
  +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
  @@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
  iser_conn_terminate(ib_conn);
   }
   
  +static int iscsi_iser_slave_configure(struct scsi_device *sdev)
  +{
  +   blk_queue_bounce_limit(sdev-request_queue, BLK_BOUNCE_ANY);
 
 You really don't want to do this.  That signals to the block layer that
 we have an iommu, although it's practically the same thing as a 64 bit
 DMA mask ... but I'd just leave it to the DMA mask to set this up
 correctly.  Anything else is asking for a subtle bug to turn up years
 from now when something causes the mask and the limit to be mismatched.

Oh.  I decided to add that line for symmetry with TCP, and was
convinced by the arguments here:

commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56
Author: Mike Christie [EMAIL PROTECTED]
Date:   Thu Jul 26 12:46:47 2007 -0500

[SCSI] iscsi_tcp: Turn off bounce buffers

It was found by LSI that on setups with large amounts of memory
we were bouncing buffers when we did not need to. If the iscsi tcp
code touches the data buffer (or a helper does),
it will kmap the buffer. iscsi_tcp also does not interact with hardware,
so it does not have any hw dma restrictions. This patch sets the bounce
buffer settings for our device queue so buffers should not be bounced
because of a driver limit.

I don't see a convenient place to callback into particular iscsi
devices to set the DMA mask per-host.  It has to go on the
shost_gendev, right?, but only for TCP and iSER, not qla4xxx, which
handles its DMA mask during device probe.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

2008-02-12 Thread James Bottomley
On Tue, 2008-02-12 at 16:46 -0500, Pete Wyckoff wrote:
 [EMAIL PROTECTED] wrote on Tue, 12 Feb 2008 15:10 -0600:
  On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote:
   iscsi_iser does not have any hardware DMA restrictions.  Add a
   slave_configure function to remove any DMA alignment restriction,
   allowing the use of direct IO from arbitrary offsets within a page.
   Also disable page bouncing; iser has no restrictions on which pages it
   can address.
   
   Signed-off-by: Pete Wyckoff [EMAIL PROTECTED]
   ---
drivers/infiniband/ulp/iser/iscsi_iser.c |8 
1 files changed, 8 insertions(+), 0 deletions(-)
   
   diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c 
   b/drivers/infiniband/ulp/iser/iscsi_iser.c
   index be1b9fb..1b272a6 100644
   --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
   +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
   @@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
 iser_conn_terminate(ib_conn);
}

   +static int iscsi_iser_slave_configure(struct scsi_device *sdev)
   +{
   + blk_queue_bounce_limit(sdev-request_queue, BLK_BOUNCE_ANY);
  
  You really don't want to do this.  That signals to the block layer that
  we have an iommu, although it's practically the same thing as a 64 bit
  DMA mask ... but I'd just leave it to the DMA mask to set this up
  correctly.  Anything else is asking for a subtle bug to turn up years
  from now when something causes the mask and the limit to be mismatched.
 
 Oh.  I decided to add that line for symmetry with TCP, and was
 convinced by the arguments here:
 
 commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56
 Author: Mike Christie [EMAIL PROTECTED]
 Date:   Thu Jul 26 12:46:47 2007 -0500
 
 [SCSI] iscsi_tcp: Turn off bounce buffers
 
 It was found by LSI that on setups with large amounts of memory
 we were bouncing buffers when we did not need to. If the iscsi tcp
 code touches the data buffer (or a helper does),
 it will kmap the buffer. iscsi_tcp also does not interact with hardware,
 so it does not have any hw dma restrictions. This patch sets the bounce
 buffer settings for our device queue so buffers should not be bounced
 because of a driver limit.
 
 I don't see a convenient place to callback into particular iscsi
 devices to set the DMA mask per-host.  It has to go on the
 shost_gendev, right?, but only for TCP and iSER, not qla4xxx, which
 handles its DMA mask during device probe.

You should be taking your mask from the underlying infiniband device as
part of the setup, shouldn't you?

James


-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

2008-02-12 Thread James Bottomley
On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote:
 iscsi_iser does not have any hardware DMA restrictions.  Add a
 slave_configure function to remove any DMA alignment restriction,
 allowing the use of direct IO from arbitrary offsets within a page.
 Also disable page bouncing; iser has no restrictions on which pages it
 can address.
 
 Signed-off-by: Pete Wyckoff [EMAIL PROTECTED]
 ---
  drivers/infiniband/ulp/iser/iscsi_iser.c |8 
  1 files changed, 8 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c 
 b/drivers/infiniband/ulp/iser/iscsi_iser.c
 index be1b9fb..1b272a6 100644
 --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
 +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
 @@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
   iser_conn_terminate(ib_conn);
  }
  
 +static int iscsi_iser_slave_configure(struct scsi_device *sdev)
 +{
 + blk_queue_bounce_limit(sdev-request_queue, BLK_BOUNCE_ANY);

You really don't want to do this.  That signals to the block layer that
we have an iommu, although it's practically the same thing as a 64 bit
DMA mask ... but I'd just leave it to the DMA mask to set this up
correctly.  Anything else is asking for a subtle bug to turn up years
from now when something causes the mask and the limit to be mismatched.

James


-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html