fix large I/O regression with iSER in 4.4+

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 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..04c660d 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(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

[Laurence Oberman]

This looks fine to me.
I am pulling this in to my SRP initiator and target testing ongoing at the moment so will be testing.
Up to now this has likely not affected me but I am pulling in all RDMA patches to test.

Reviewed-by: Laurence Oberman <loberman@redhat.com>

Laurence Oberman
Principal Software Maintenance Engineer
Red Hat Global Support Services

----- Original Message -----
From: "Christoph Hellwig" <hch@lst.de>
To: linux-rdma@vger.kernel.org, linux-scsi@vger.kernel.org
Sent: Monday, April 11, 2016 6:47:25 PM
Subject: [PATCH 1/2] scsi: add a max_segment_size limitation to struct Scsi_Host

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@lst.de>
---
 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..04c660d 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(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@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

[Bart Van Assche]

On 04/11/2016 03:47 PM, Christoph Hellwig wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8106515..04c660d 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(shost->max_segment_size, dma_get_max_seg_size(dev)));
>
>   	if (!shost->use_clustering)
>   		q->limits.cluster = 0;

Hello Christoph,

Since Scsi_Host.max_segment_size is initialized to zero, shouldn't min() 
be changed into min_not_zero()?

Bart.

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

[Christoph Hellwig]

On Mon, Apr 11, 2016 at 04:32:16PM -0700, Bart Van Assche wrote:
> Since Scsi_Host.max_segment_size is initialized to zero, shouldn't min() be 
> changed into min_not_zero()?

It probably should.  Thanks for catching this.
--
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

[Laurence Oberman]

Thanks Bart
Good catch, I completely missed it.

Laurence Oberman
Principal Software Maintenance Engineer
Red Hat Global Support Services

----- Original Message -----
From: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
To: "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Sent: Monday, April 11, 2016 7:32:16 PM
Subject: Re: [PATCH 1/2] scsi: add a max_segment_size limitation to struct Scsi_Host

On 04/11/2016 03:47 PM, Christoph Hellwig wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8106515..04c660d 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(shost->max_segment_size, dma_get_max_seg_size(dev)));
>
>   	if (!shost->use_clustering)
>   		q->limits.cluster = 0;

Hello Christoph,

Since Scsi_Host.max_segment_size is initialized to zero, shouldn't min() 
be changed into min_not_zero()?

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
--
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

[Laurence Oberman]

Modified patch to use min_not_zero()
Ran a number of tests overnight on F/C, SCSI/SAS and SRP (RDMA) and no issues found.

Tested-by: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Laurence Oberman
Principal Software Maintenance Engineer
Red Hat Global Support Services

----- Original Message -----
From: "Laurence Oberman" <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
To: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Sent: Monday, April 11, 2016 7:44:24 PM
Subject: Re: [PATCH 1/2] scsi: add a max_segment_size limitation to struct Scsi_Host

Thanks Bart
Good catch, I completely missed it.

Laurence Oberman
Principal Software Maintenance Engineer
Red Hat Global Support Services

----- Original Message -----
From: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
To: "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Sent: Monday, April 11, 2016 7:32:16 PM
Subject: Re: [PATCH 1/2] scsi: add a max_segment_size limitation to struct Scsi_Host

On 04/11/2016 03:47 PM, Christoph Hellwig wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8106515..04c660d 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(shost->max_segment_size, dma_get_max_seg_size(dev)));
>
>   	if (!shost->use_clustering)
>   		q->limits.cluster = 0;

Hello Christoph,

Since Scsi_Host.max_segment_size is initialized to zero, shouldn't min() 
be changed into min_not_zero()?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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