diff mbox series

RDMA/srp: Increase max_segment_size

Message ID 20190122182520.59519-1-bvanassche@acm.org (mailing list archive)
State Mainlined
Commit 0b5cb3300ae59ed7e93b465dfa2384a6a4df8eb4
Delegated to: Jason Gunthorpe
Headers show
Series RDMA/srp: Increase max_segment_size | expand

Commit Message

Bart Van Assche Jan. 22, 2019, 6:25 p.m. UTC
The default behavior of the SCSI core is to set the block layer request
queue parameter max_segment_size to 64 KB. That means that elements of
scatterlists are limited to 64 KB. Since RDMA adapters support larger
sizes, increase max_segment_size for the SRP initiator.

Notes:
- The SCSI max_segment_size parameter was introduced in kernel v5.0. See
  also commit 50c2e9107f17 ("scsi: introduce a max_segment_size
  host_template parameters").
- Some other block drivers already set max_segment_size to UINT_MAX,
  e.g. nbd and rbd.

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |  1 +
 include/rdma/ib_verbs.h             | 13 +++++++++++++
 2 files changed, 14 insertions(+)

Comments

Douglas Gilbert Jan. 22, 2019, 8:47 p.m. UTC | #1
On 2019-01-22 1:25 p.m., Bart Van Assche wrote:
> The default behavior of the SCSI core is to set the block layer request
> queue parameter max_segment_size to 64 KB. That means that elements of
> scatterlists are limited to 64 KB. Since RDMA adapters support larger
> sizes, increase max_segment_size for the SRP initiator.
> 
> Notes:
> - The SCSI max_segment_size parameter was introduced in kernel v5.0. See
>    also commit 50c2e9107f17 ("scsi: introduce a max_segment_size
>    host_template parameters").
> - Some other block drivers already set max_segment_size to UINT_MAX,
>    e.g. nbd and rbd.

In my sg v4 driver rewrite one of options within the SG_SET_GET_EXTENDED
ioctl is to allow the user to modify this value. It is a #define to
32 KB in the production sg v3 driver.

Doug Gilbert

> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/infiniband/ulp/srp/ib_srp.c |  1 +
>   include/rdma/ib_verbs.h             | 13 +++++++++++++
>   2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 085dba075651..a1173e0992e6 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -3819,6 +3819,7 @@ static ssize_t srp_create_target(struct device *dev,
>   	target_host->max_id      = 1;
>   	target_host->max_lun     = -1LL;
>   	target_host->max_cmd_len = sizeof ((struct srp_cmd *) (void *) 0L)->cdb;
> +	target_host->max_segment_size = ib_dma_max_seg_size(ibdev);
>   
>   	target = host_to_target(target_host);
>   
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 94b6e1dd4dab..71ea144ec823 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -3715,6 +3715,19 @@ static inline unsigned int ib_sg_dma_len(struct ib_device *dev,
>   	return sg_dma_len(sg);
>   }
>   
> +/**
> + * ib_dma_max_seg_size - Return the size limit of a single DMA transfer
> + * @dev: The device to query
> + *
> + * The returned value represents a size in bytes.
> + */
> +static inline unsigned int ib_dma_max_seg_size(struct ib_device *dev)
> +{
> +	struct device_dma_parameters *p = dev->dma_device->dma_parms;
> +
> +	return p ? p->max_segment_size : UINT_MAX;
> +}
> +
>   /**
>    * ib_dma_sync_single_for_cpu - Prepare DMA region to be accessed by CPU
>    * @dev: The device for which the DMA address was created
>
Bart Van Assche Jan. 30, 2019, 6:59 p.m. UTC | #2
On Tue, 2019-01-22 at 10:25 -0800, Bart Van Assche wrote:
> The default behavior of the SCSI core is to set the block layer request
> queue parameter max_segment_size to 64 KB. That means that elements of
> scatterlists are limited to 64 KB. Since RDMA adapters support larger
> sizes, increase max_segment_size for the SRP initiator.
> 
> Notes:
> - The SCSI max_segment_size parameter was introduced in kernel v5.0. See
>   also commit 50c2e9107f17 ("scsi: introduce a max_segment_size
>   host_template parameters").
> - Some other block drivers already set max_segment_size to UINT_MAX,
>   e.g. nbd and rbd.

Hi Jason and Doug,

Can one of you have a look at this patch?

Thanks,

Bart.
Jason Gunthorpe Jan. 30, 2019, 7:21 p.m. UTC | #3
On Wed, Jan 30, 2019 at 10:59:34AM -0800, Bart Van Assche wrote:
> On Tue, 2019-01-22 at 10:25 -0800, Bart Van Assche wrote:
> > The default behavior of the SCSI core is to set the block layer request
> > queue parameter max_segment_size to 64 KB. That means that elements of
> > scatterlists are limited to 64 KB. Since RDMA adapters support larger
> > sizes, increase max_segment_size for the SRP initiator.
> > 
> > Notes:
> > - The SCSI max_segment_size parameter was introduced in kernel v5.0. See
> >   also commit 50c2e9107f17 ("scsi: introduce a max_segment_size
> >   host_template parameters").
> > - Some other block drivers already set max_segment_size to UINT_MAX,
> >   e.g. nbd and rbd.
> 
> Hi Jason and Doug,
> 
> Can one of you have a look at this patch?

I was confused by the unanswered remark from Doug Gilbert - what that
a request to change something?

Jason
Bart Van Assche Jan. 30, 2019, 8:22 p.m. UTC | #4
On Wed, 2019-01-30 at 12:21 -0700, Jason Gunthorpe wrote:
> On Wed, Jan 30, 2019 at 10:59:34AM -0800, Bart Van Assche wrote:
> > On Tue, 2019-01-22 at 10:25 -0800, Bart Van Assche wrote:
> > > The default behavior of the SCSI core is to set the block layer request
> > > queue parameter max_segment_size to 64 KB. That means that elements of
> > > scatterlists are limited to 64 KB. Since RDMA adapters support larger
> > > sizes, increase max_segment_size for the SRP initiator.
> > > 
> > > Notes:
> > > - The SCSI max_segment_size parameter was introduced in kernel v5.0. See
> > >   also commit 50c2e9107f17 ("scsi: introduce a max_segment_size
> > >   host_template parameters").
> > > - Some other block drivers already set max_segment_size to UINT_MAX,
> > >   e.g. nbd and rbd.
> > 
> > Hi Jason and Doug,
> > 
> > Can one of you have a look at this patch?
> 
> I was confused by the unanswered remark from Doug Gilbert - what that
> a request to change something?

Hi Jason,

My understanding is that Doug Gilbert's e-mail was not a request to change my
patch but instead an announcement of something he plans to implement in the
future. The functionality Doug mentioned seems useful to me but does not replace
this patch. It is much easier to users to have the default set correctly instead
of having to modify the default from the command line or from a udev rule.

Thanks,

Bart.
Bart Van Assche Jan. 30, 2019, 8:23 p.m. UTC | #5
On Tue, 2019-01-22 at 15:47 -0500, Douglas Gilbert wrote:
> On 2019-01-22 1:25 p.m., Bart Van Assche wrote:
> > The default behavior of the SCSI core is to set the block layer request
> > queue parameter max_segment_size to 64 KB. That means that elements of
> > scatterlists are limited to 64 KB. Since RDMA adapters support larger
> > sizes, increase max_segment_size for the SRP initiator.
> > 
> > Notes:
> > - The SCSI max_segment_size parameter was introduced in kernel v5.0. See
> >    also commit 50c2e9107f17 ("scsi: introduce a max_segment_size
> >    host_template parameters").
> > - Some other block drivers already set max_segment_size to UINT_MAX,
> >    e.g. nbd and rbd.
> 
> In my sg v4 driver rewrite one of options within the SG_SET_GET_EXTENDED
> ioctl is to allow the user to modify this value. It is a #define to
> 32 KB in the production sg v3 driver.

Hi Doug,

This functionality seems useful to me. But it is not clear to me how it
will be guaranteed that the value set by the user won't exceed the largest
max_segment_size value supported by the driver?

Thanks,

Bart.
Jason Gunthorpe Jan. 30, 2019, 10:22 p.m. UTC | #6
On Tue, Jan 22, 2019 at 10:25:20AM -0800, Bart Van Assche wrote:
> The default behavior of the SCSI core is to set the block layer request
> queue parameter max_segment_size to 64 KB. That means that elements of
> scatterlists are limited to 64 KB. Since RDMA adapters support larger
> sizes, increase max_segment_size for the SRP initiator.
> 
> Notes:
> - The SCSI max_segment_size parameter was introduced in kernel v5.0. See
>   also commit 50c2e9107f17 ("scsi: introduce a max_segment_size
>   host_template parameters").
> - Some other block drivers already set max_segment_size to UINT_MAX,
>   e.g. nbd and rbd.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/infiniband/ulp/srp/ib_srp.c |  1 +
>  include/rdma/ib_verbs.h             | 13 +++++++++++++
>  2 files changed, 14 insertions(+)

Applied to for-next

Thanks,
Jason
Douglas Gilbert Jan. 30, 2019, 10:50 p.m. UTC | #7
On 2019-01-30 3:23 p.m., Bart Van Assche wrote:
> On Tue, 2019-01-22 at 15:47 -0500, Douglas Gilbert wrote:
>> On 2019-01-22 1:25 p.m., Bart Van Assche wrote:
>>> The default behavior of the SCSI core is to set the block layer request
>>> queue parameter max_segment_size to 64 KB. That means that elements of
>>> scatterlists are limited to 64 KB. Since RDMA adapters support larger
>>> sizes, increase max_segment_size for the SRP initiator.
>>>
>>> Notes:
>>> - The SCSI max_segment_size parameter was introduced in kernel v5.0. See
>>>     also commit 50c2e9107f17 ("scsi: introduce a max_segment_size
>>>     host_template parameters").
>>> - Some other block drivers already set max_segment_size to UINT_MAX,
>>>     e.g. nbd and rbd.
>>
>> In my sg v4 driver rewrite one of options within the SG_SET_GET_EXTENDED
>> ioctl is to allow the user to modify this value. It is a #define to
>> 32 KB in the production sg v3 driver.
> 
> Hi Doug,
> 
> This functionality seems useful to me. But it is not clear to me how it
> will be guaranteed that the value set by the user won't exceed the largest
> max_segment_size value supported by the driver?

The sg v4 driver exists and its code can be found at:
    http://sg.danny.cz/sg/sg_v40.html

And the code there takes the min(<given_sgat_sz>, <following_function>) as
the element size. That function is:

static int
max_sectors_bytes(struct request_queue *q)
{
         unsigned int max_sectors = queue_max_sectors(q);

         max_sectors = min_t(unsigned int, max_sectors, INT_MAX >> 9);
         return max_sectors << 9;
}

Also <given_sgat_sz> is rounded up to PAGE_SIZE and rejected if it is not
a power of 2. Does that look correct?

Doug Gilbert
Bart Van Assche Jan. 30, 2019, 10:58 p.m. UTC | #8
On Wed, 2019-01-30 at 17:50 -0500, Douglas Gilbert wrote:
> On 2019-01-30 3:23 p.m., Bart Van Assche wrote:
> > On Tue, 2019-01-22 at 15:47 -0500, Douglas Gilbert wrote:
> > > On 2019-01-22 1:25 p.m., Bart Van Assche wrote:
> > > > The default behavior of the SCSI core is to set the block layer request
> > > > queue parameter max_segment_size to 64 KB. That means that elements of
> > > > scatterlists are limited to 64 KB. Since RDMA adapters support larger
> > > > sizes, increase max_segment_size for the SRP initiator.
> > > > 
> > > > Notes:
> > > > - The SCSI max_segment_size parameter was introduced in kernel v5.0. See
> > > >     also commit 50c2e9107f17 ("scsi: introduce a max_segment_size
> > > >     host_template parameters").
> > > > - Some other block drivers already set max_segment_size to UINT_MAX,
> > > >     e.g. nbd and rbd.
> > > 
> > > In my sg v4 driver rewrite one of options within the SG_SET_GET_EXTENDED
> > > ioctl is to allow the user to modify this value. It is a #define to
> > > 32 KB in the production sg v3 driver.
> > 
> > Hi Doug,
> > 
> > This functionality seems useful to me. But it is not clear to me how it
> > will be guaranteed that the value set by the user won't exceed the largest
> > max_segment_size value supported by the driver?
> 
> The sg v4 driver exists and its code can be found at:
>     http://sg.danny.cz/sg/sg_v40.html
> 
> And the code there takes the min(<given_sgat_sz>, <following_function>) as
> the element size. That function is:
> 
> static int
> max_sectors_bytes(struct request_queue *q)
> {
>          unsigned int max_sectors = queue_max_sectors(q);
> 
>          max_sectors = min_t(unsigned int, max_sectors, INT_MAX >> 9);
>          return max_sectors << 9;
> }
> 
> Also <given_sgat_sz> is rounded up to PAGE_SIZE and rejected if it is not
> a power of 2. Does that look correct?

Hi Doug,

Have you noticed that my patch affects max_segment_size and that your patch
refers to the max_sectors limit? From the Linux kernel header file blkdev.h:

static inline unsigned int queue_max_sectors(struct request_queue *q)
{
	return q->limits.max_sectors;
}
[ ... ]
static inline unsigned int queue_max_segment_size(struct request_queue *q)
{
	return q->limits.max_segment_size;
}

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 085dba075651..a1173e0992e6 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3819,6 +3819,7 @@  static ssize_t srp_create_target(struct device *dev,
 	target_host->max_id      = 1;
 	target_host->max_lun     = -1LL;
 	target_host->max_cmd_len = sizeof ((struct srp_cmd *) (void *) 0L)->cdb;
+	target_host->max_segment_size = ib_dma_max_seg_size(ibdev);
 
 	target = host_to_target(target_host);
 
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 94b6e1dd4dab..71ea144ec823 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -3715,6 +3715,19 @@  static inline unsigned int ib_sg_dma_len(struct ib_device *dev,
 	return sg_dma_len(sg);
 }
 
+/**
+ * ib_dma_max_seg_size - Return the size limit of a single DMA transfer
+ * @dev: The device to query
+ *
+ * The returned value represents a size in bytes.
+ */
+static inline unsigned int ib_dma_max_seg_size(struct ib_device *dev)
+{
+	struct device_dma_parameters *p = dev->dma_device->dma_parms;
+
+	return p ? p->max_segment_size : UINT_MAX;
+}
+
 /**
  * ib_dma_sync_single_for_cpu - Prepare DMA region to be accessed by CPU
  * @dev: The device for which the DMA address was created