Re: [PATCH 2/2] IB/iser: set max_segment_size

[Or Gerlitz]

On Tue, Apr 12, 2016 at 5:13 PM, Christoph Hellwig  wrote:
> So that we don't overflow the number of MR segments allocated because
> we have to split on SGL segment into multiple MR segments.
>
> Signed-off-by: Christoph Hellwig 

nit, but please fix


IB/iser: set [...] --> IB/iser: Set max segment size in the SCSI host template

I prefer to avoid names of variables and functions in commit titles
for the iser initiator driver.

I find the usage of higher language very beneficial for maintenance  and such.

Sagi, I would appreciate if you avoid acking patches lacking this
practice for iser-I,
specifically we want people to use capital letter in the 1st word that
follows that IB/iser: prefix - ok?

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] IB/iser: set max_segment_size

[Christoph Hellwig]

On Tue, Apr 12, 2016 at 11:43:09AM -0700, Christoph Hellwig wrote:
> > I think this means that there is a mismatch between the current block layer
> > limits and what NVMe / RDMA drivers need ...
> 
> If we tell the block layer that we can only handle page sized comments
> using max_segent_size it should do the right thing.
> 
> Now one thing that might be useful is to force the max_segent_size
> when setting the virt boundary, as they seem to be closely related.

I looked into this, and found something that also means the the patches
in this series unfortunately won't work as expected:

blk_queue_max_segment_size checks that we at least set the maximum
segment size to the systen page size.  This means we can't actually
set the segment size to the virt boundary for the case where it's fixed
4k (iSER, NVMe).  So I think we'll have to stick to setting max_sectors
to

((max_segments - 1) * page_size) >> 9

for all these drivers.  I'll retract this patch and will send a new
one implementing this in iSER, and also research if a common helper
for it makes sense.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] IB/iser: set max_segment_size

[Sagi Grimberg]

Acked-by: Sagi Grimberg 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] IB/iser: set max_segment_size

[Sagi Grimberg]

In iser we sorta rely on 4k pages so we avoid
PAGE_SIZE but rather set SIZE_4K for these sort
of things (like we did in the virt_boundary).


So you still want only 4k segments even on PPC where the PAGE_SIZE is
16k?


Yes, iSER has the "no-gaps" constraint (like nvme) and some
applications in the past were known to issue vectored IO that doesn't
necessarily align with the system PAGE_SIZE. These sort of workloads
resulted in suboptimal performance on ppc, ia64 etc (almost every I/O
had gaps).

Before the block layer was able to enforce scatterlists without gaps,
iSER used bounce buffering in order to service "gappy" I/O which was
probably a lot worse than bio splitting like the block layer is doing
today, but still we felt that having 4k segments even on larger
PAGE_SIZE systems was a decent compromise.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] IB/iser: set max_segment_size

[Christoph Hellwig]

On Tue, Apr 12, 2016 at 11:13:52AM -0700, Bart Van Assche wrote:
> >That's what NVMe does, but I don't think it's a good idea.  Because
> >of the unaligned start into the page this means you have to set the limit
> >to one lower than the actual hardware limit.
> 
> I think this means that there is a mismatch between the current block layer
> limits and what NVMe / RDMA drivers need ...

If we tell the block layer that we can only handle page sized comments
using max_segent_size it should do the right thing.

Now one thing that might be useful is to force the max_segent_size
when setting the virt boundary, as they seem to be closely related.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] IB/iser: set max_segment_size

[Bart Van Assche]

On 04/12/2016 09:51 AM, Christoph Hellwig wrote:

On Tue, Apr 12, 2016 at 08:34:03AM -0700, Bart Van Assche wrote:

ib_sg_to_pages() can handle segments that are larger than mr->page_size.


The interface handles it fine, but we'll still end up using a segment
per (MR) page.


Have you considered to set queue_limits.max_hw_sectors instead of
max_segment_size?


That's what NVMe does, but I don't think it's a good idea.  Because
of the unaligned start into the page this means you have to set the limit
to one lower than the actual hardware limit.


I think this means that there is a mismatch between the current block 
layer limits and what NVMe / RDMA drivers need ...


Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] IB/iser: set max_segment_size

[Christoph Hellwig]

On Tue, Apr 12, 2016 at 08:34:03AM -0700, Bart Van Assche wrote:
> Hello Christoph,
>
> ib_sg_to_pages() can handle segments that are larger than mr->page_size. 

The interface handles it fine, but we'll still end up using a segment
per (MR) page.

> Have you considered to set queue_limits.max_hw_sectors instead of 
> max_segment_size?

That's what NVMe does, but I don't think it's a good idea.  Because
of the unaligned start into the page this means you have to set the limit
to one lower than the actual hardware limit.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] IB/iser: set max_segment_size

[Bart Van Assche]

On 04/12/2016 07:13 AM, Christoph Hellwig wrote:

So that we don't overflow the number of MR segments allocated because
we have to split on SGL segment into multiple MR segments.

Signed-off-by: Christoph Hellwig 
---
  drivers/infiniband/ulp/iser/iscsi_iser.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c 
b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 80b6bed..dc55950 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -623,6 +623,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
shost->max_id = 0;
shost->max_channel = 0;
shost->max_cmd_len = 16;
+   shost->max_segment_size = SIZE_4K;

/*
 * older userspace tools (before 2.0-870) did not pass us


Hello Christoph,

ib_sg_to_pages() can handle segments that are larger than mr->page_size. 
Have you considered to set queue_limits.max_hw_sectors instead of 
max_segment_size?


Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] IB/iser: set max_segment_size

[Christoph Hellwig]

On Tue, Apr 12, 2016 at 07:27:36AM -0700, James Bottomley wrote:
> > In iser we sorta rely on 4k pages so we avoid
> > PAGE_SIZE but rather set SIZE_4K for these sort
> > of things (like we did in the virt_boundary).
> 
> So you still want only 4k segments even on PPC where the PAGE_SIZE is
> 16k?

I'll leave the high level question to Sagi and friends, but if virt_boundary
and the MR page size are set to 4k we need to set the segment size
to the same, so this patch needs to be SIZE_4K indeed unless the
other two are changed.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] IB/iser: set max_segment_size

[James Bottomley]

On Tue, 2016-04-12 at 13:48 +0300, Sagi Grimberg wrote:
> > So that we don't overflow the number of MR segments allocated
> > because
> > we have to split on SGL segment into multiple MR segments.
> > 
> > Signed-off-by: Christoph Hellwig 
> > ---
> >   drivers/infiniband/ulp/iser/iscsi_iser.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c
> > b/drivers/infiniband/ulp/iser/iscsi_iser.c
> > index 80b6bed..784504a 100644
> > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
> > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
> > @@ -623,6 +623,7 @@ iscsi_iser_session_create(struct iscsi_endpoint
> > *ep,
> > shost->max_id = 0;
> > shost->max_channel = 0;
> > shost->max_cmd_len = 16;
> > +   shost->max_segment_size = PAGE_SIZE;
> 
> In iser we sorta rely on 4k pages so we avoid
> PAGE_SIZE but rather set SIZE_4K for these sort
> of things (like we did in the virt_boundary).

So you still want only 4k segments even on PPC where the PAGE_SIZE is
16k?

James


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

[PATCH 2/2] IB/iser: set max_segment_size

[Christoph Hellwig]

So that we don't overflow the number of MR segments allocated because
we have to split on SGL segment into multiple MR segments.

Signed-off-by: Christoph Hellwig 
---
 drivers/infiniband/ulp/iser/iscsi_iser.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c 
b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 80b6bed..dc55950 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -623,6 +623,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
shost->max_id = 0;
shost->max_channel = 0;
shost->max_cmd_len = 16;
+   shost->max_segment_size = SIZE_4K;
 
/*
 * older userspace tools (before 2.0-870) did not pass us
-- 
2.1.4

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

Re: [PATCH 2/2] IB/iser: set max_segment_size

[Sagi Grimberg]

So that we don't overflow the number of MR segments allocated because
we have to split on SGL segment into multiple MR segments.

Signed-off-by: Christoph Hellwig 
---
  drivers/infiniband/ulp/iser/iscsi_iser.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c 
b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 80b6bed..784504a 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -623,6 +623,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
shost->max_id = 0;
shost->max_channel = 0;
shost->max_cmd_len = 16;
+   shost->max_segment_size = PAGE_SIZE;


In iser we sorta rely on 4k pages so we avoid
PAGE_SIZE but rather set SIZE_4K for these sort
of things (like we did in the virt_boundary).
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] IB/iser: set max_segment_size

[Christoph Hellwig]

So that we don't overflow the number of MR segments allocated because
we have to split on SGL segment into multiple MR segments.

Signed-off-by: Christoph Hellwig 
---
 drivers/infiniband/ulp/iser/iscsi_iser.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c 
b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 80b6bed..784504a 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -623,6 +623,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
shost->max_id = 0;
shost->max_channel = 0;
shost->max_cmd_len = 16;
+   shost->max_segment_size = PAGE_SIZE;
 
/*
 * older userspace tools (before 2.0-870) did not pass us
-- 
2.1.4

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