fix large I/O regression with iSER in 4.4+ V2

fix large I/O regression with iSER in 4.4+ V2

[Christoph Hellwig]

Since iSER was converted to use the generic virt_boundary mechanism
(which was called something else in 4.4), it didn't handle the case
where a request is using up the full size of max_hw_segments, but
not actually aligned to the virt boundary.  This series sets the
maximum segment size limit to fix this workload (xfs_repair is a good
reproducer, btw).

Should probably go into 4.4 and 4.5-stable.

Changes since V1:
 - use min_not_zero
 - use SIZE_4K instead of PAGE_SIZE in iser

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

[PATCH 1/2] scsi: add a max_segment_size limitation to struct Scsi_Host

[Christoph Hellwig]

RDMA drivers need segments that aren't larger than a single HCA page
for memory registrations to work properly, so wire up this limitation
in the host.

While we could just call blk_queue_max_segment_size from ->slave_configure,
that would override the global limit based on the DMA device, so let's do
it the traditional way by adding a field to the Scsi_Host structure.

Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/scsi/scsi_lib.c  | 3 ++-
 include/scsi/scsi_host.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8106515..ad79372 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2120,7 +2120,8 @@ static void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
 	blk_queue_segment_boundary(q, shost->dma_boundary);
 	dma_set_seg_boundary(dev, shost->dma_boundary);
 
-	blk_queue_max_segment_size(q, dma_get_max_seg_size(dev));
+	blk_queue_max_segment_size(q, min_not_zero(shost->max_segment_size,
+						   dma_get_max_seg_size(dev)));
 
 	if (!shost->use_clustering)
 		q->limits.cluster = 0;
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index fcfa3d7..f11d3fe 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -621,6 +621,7 @@ struct Scsi_Host {
 	short unsigned int sg_tablesize;
 	short unsigned int sg_prot_tablesize;
 	unsigned int max_sectors;
+	unsigned int max_segment_size;
 	unsigned long dma_boundary;
 	/*
 	 * In scsi-mq mode, the number of hardware queues supported by the LLD.
-- 
2.1.4

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

Re: [PATCH 1/2] scsi: add a max_segment_size limitation to struct Scsi_Host

[Bart Van Assche]

On 04/12/2016 07:13 AM, Christoph Hellwig wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8106515..ad79372 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2120,7 +2120,8 @@ static void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
>   	blk_queue_segment_boundary(q, shost->dma_boundary);
>   	dma_set_seg_boundary(dev, shost->dma_boundary);
>
> -	blk_queue_max_segment_size(q, dma_get_max_seg_size(dev));
> +	blk_queue_max_segment_size(q, min_not_zero(shost->max_segment_size,
> +						   dma_get_max_seg_size(dev)));
>
>   	if (!shost->use_clustering)
>   		q->limits.cluster = 0;
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index fcfa3d7..f11d3fe 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -621,6 +621,7 @@ struct Scsi_Host {
>   	short unsigned int sg_tablesize;
>   	short unsigned int sg_prot_tablesize;
>   	unsigned int max_sectors;
> +	unsigned int max_segment_size;
>   	unsigned long dma_boundary;
>   	/*
>   	 * In scsi-mq mode, the number of hardware queues supported by the LLD.

Hello Christoph,

The value zero has another meaning for Scsi_Host.max_segment_size than 
for queue_limits.max_segment_size. Shouldn't that be documented somewhere?

Thanks,

Bart.

Re: [PATCH 1/2] scsi: add a max_segment_size limitation to struct Scsi_Host

[Laurence Oberman]

Other than adding the patch and rebuilding the kernel and testing regular stuff, which I had to do anyway, that was the extent of testing.
I did not see where it was used to be honest other than adding the structure member.
I wanted to test the simple change because it was in scsi_lib.c which has many dependencies of course.

Laurence Oberman
Principal Software Maintenance Engineer
Red Hat Global Support Services

----- Original Message -----
From: "Bart Van Assche" <bart.vanassche@sandisk.com>
To: "Christoph Hellwig" <hch@lst.de>, linux-rdma@vger.kernel.org, linux-scsi@vger.kernel.org
Sent: Tuesday, April 12, 2016 11:19:20 AM
Subject: Re: [PATCH 1/2] scsi: add a max_segment_size limitation to struct Scsi_Host

On 04/12/2016 07:13 AM, Christoph Hellwig wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8106515..ad79372 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2120,7 +2120,8 @@ static void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
>   	blk_queue_segment_boundary(q, shost->dma_boundary);
>   	dma_set_seg_boundary(dev, shost->dma_boundary);
>
> -	blk_queue_max_segment_size(q, dma_get_max_seg_size(dev));
> +	blk_queue_max_segment_size(q, min_not_zero(shost->max_segment_size,
> +						   dma_get_max_seg_size(dev)));
>
>   	if (!shost->use_clustering)
>   		q->limits.cluster = 0;
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index fcfa3d7..f11d3fe 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -621,6 +621,7 @@ struct Scsi_Host {
>   	short unsigned int sg_tablesize;
>   	short unsigned int sg_prot_tablesize;
>   	unsigned int max_sectors;
> +	unsigned int max_segment_size;
>   	unsigned long dma_boundary;
>   	/*
>   	 * In scsi-mq mode, the number of hardware queues supported by the LLD.

Hello Christoph,

The value zero has another meaning for Scsi_Host.max_segment_size than 
for queue_limits.max_segment_size. Shouldn't that be documented somewhere?

Thanks,

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

Re: [PATCH 1/2] scsi: add a max_segment_size limitation to struct Scsi_Host

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 <hch-jcswGhMUV9g@public.gmane.org>
---
 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-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 <hch@lst.de>
> ---
>   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.

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.

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-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 <sagi@grimberg.me>

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

[Or Gerlitz]

On Tue, Apr 12, 2016 at 5:13 PM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> 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 <hch-jcswGhMUV9g@public.gmane.org>

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-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

fix large I/O regression with iSER in 4.4+

[Christoph Hellwig]

Since iSER was converted to use the generic virt_boundary mechanism
(which was called something else in 4.4), it didn't handle the case
where a request is using up the full size of max_hw_segments, but
not actually aligned to the virt boundary.  This series sets the
maximum segment size limit to fix this workload (xfs_repair is a good
reproducer, btw).

Should probably go into 4.4 and 4.5-stable.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 <hch@lst.de>
---
 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

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 <hch@lst.de>
> ---
>   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).

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 <hch-jcswGhMUV9g@public.gmane.org>
> > ---
> >   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-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html