[PATCH] tcm_loop: Increase maximum request size

[PATCH] tcm_loop: Increase maximum request size

[Nikos Tsironis]

Increase the maximum request size for tcm_loop, by setting sg_tablesize
to SG_MAX_SEGMENTS.

The current value of 256 for sg_tablesize limits the request size to
PAGE_SIZE * 256, which for 4K pages is 1MiB.

Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com>
---
 drivers/target/loopback/tcm_loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 4407b56aa6d1..6d7c3ebd8613 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -308,7 +308,7 @@ static struct scsi_host_template tcm_loop_driver_template = {
 	.eh_device_reset_handler = tcm_loop_device_reset,
 	.eh_target_reset_handler = tcm_loop_target_reset,
 	.this_id		= -1,
-	.sg_tablesize		= 256,
+	.sg_tablesize		= SG_MAX_SEGMENTS,
 	.max_sectors		= 0xFFFF,
 	.dma_boundary		= PAGE_SIZE - 1,
 	.module			= THIS_MODULE,
-- 
2.30.2

Re: [PATCH] tcm_loop: Increase maximum request size

[Bart Van Assche]

On 9/29/22 04:55, Nikos Tsironis wrote:
> Increase the maximum request size for tcm_loop, by setting sg_tablesize
> to SG_MAX_SEGMENTS.
> 
> The current value of 256 for sg_tablesize limits the request size to
> PAGE_SIZE * 256, which for 4K pages is 1MiB.
> 
> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com>
> ---
>   drivers/target/loopback/tcm_loop.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
> index 4407b56aa6d1..6d7c3ebd8613 100644
> --- a/drivers/target/loopback/tcm_loop.c
> +++ b/drivers/target/loopback/tcm_loop.c
> @@ -308,7 +308,7 @@ static struct scsi_host_template tcm_loop_driver_template = {
>   	.eh_device_reset_handler = tcm_loop_device_reset,
>   	.eh_target_reset_handler = tcm_loop_target_reset,
>   	.this_id		= -1,
> -	.sg_tablesize		= 256,
> +	.sg_tablesize		= SG_MAX_SEGMENTS,
>   	.max_sectors		= 0xFFFF,
>   	.dma_boundary		= PAGE_SIZE - 1,
>   	.module			= THIS_MODULE,

There is more that can be improved for this driver, namely removal of 
the dma_boundary parameter and increasing max_sectors.

Thanks,

Bart.

Re: [PATCH] tcm_loop: Increase maximum request size

[Nikos Tsironis]

On 10/3/22 00:09, Bart Van Assche wrote:
> On 9/29/22 04:55, Nikos Tsironis wrote:
>> Increase the maximum request size for tcm_loop, by setting sg_tablesize
>> to SG_MAX_SEGMENTS.
>>
>> The current value of 256 for sg_tablesize limits the request size to
>> PAGE_SIZE * 256, which for 4K pages is 1MiB.
>>
>> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com>
>> ---
>>   drivers/target/loopback/tcm_loop.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
>> index 4407b56aa6d1..6d7c3ebd8613 100644
>> --- a/drivers/target/loopback/tcm_loop.c
>> +++ b/drivers/target/loopback/tcm_loop.c
>> @@ -308,7 +308,7 @@ static struct scsi_host_template tcm_loop_driver_template = {
>>       .eh_device_reset_handler = tcm_loop_device_reset,
>>       .eh_target_reset_handler = tcm_loop_target_reset,
>>       .this_id        = -1,
>> -    .sg_tablesize        = 256,
>> +    .sg_tablesize        = SG_MAX_SEGMENTS,
>>       .max_sectors        = 0xFFFF,
>>       .dma_boundary        = PAGE_SIZE - 1,
>>       .module            = THIS_MODULE,
> 
> There is more that can be improved for this driver, namely removal of the dma_boundary parameter and increasing max_sectors.
> 
Hi Bart,

Thanks for the feedback!

Should I make these changes as part of this patch, or can I leave them for a
follow up patch?

Thanks,
Nikos

Re: [PATCH] tcm_loop: Increase maximum request size

[Bart Van Assche]

On 10/12/22 07:19, Nikos Tsironis wrote:
> Should I make these changes as part of this patch, or can I leave them for a
> follow up patch?

A (thoroughly tested) follow-up patch is probably better.

Thanks,

Bart.

Re: [PATCH] tcm_loop: Increase maximum request size

[Nikos Tsironis]

Hello,

This is a kind reminder for this patch. Do you require any changes to
merge it?

Nikos

Re: [PATCH] tcm_loop: Increase maximum request size

[Mike Christie]

On 9/29/22 6:55 AM, Nikos Tsironis wrote:
> Increase the maximum request size for tcm_loop, by setting sg_tablesize
> to SG_MAX_SEGMENTS.
> 
> The current value of 256 for sg_tablesize limits the request size to
> PAGE_SIZE * 256, which for 4K pages is 1MiB.
> 
> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com>
> ---
>  drivers/target/loopback/tcm_loop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
> index 4407b56aa6d1..6d7c3ebd8613 100644
> --- a/drivers/target/loopback/tcm_loop.c
> +++ b/drivers/target/loopback/tcm_loop.c
> @@ -308,7 +308,7 @@ static struct scsi_host_template tcm_loop_driver_template = {
>  	.eh_device_reset_handler = tcm_loop_device_reset,
>  	.eh_target_reset_handler = tcm_loop_target_reset,
>  	.this_id		= -1,
> -	.sg_tablesize		= 256,
> +	.sg_tablesize		= SG_MAX_SEGMENTS,
>  	.max_sectors		= 0xFFFF,
>  	.dma_boundary		= PAGE_SIZE - 1,
>  	.module			= THIS_MODULE,

I think you need to make this configurable.

If you use loop with pscsi, then the sgl that loop now gets might be too
big for the backend device so we now fail in:

pscsi_map_sg -> blk_rq_append_bio -> ll_back_merge_fn

So some users might be relying on the smaller limit.

Re: [PATCH] tcm_loop: Increase maximum request size

[Christoph Hellwig]

On Wed, Dec 07, 2022 at 12:29:56PM -0600, Mike Christie wrote:
> I think you need to make this configurable.
> 
> If you use loop with pscsi, then the sgl that loop now gets might be too
> big for the backend device so we now fail in:
> 
> pscsi_map_sg -> blk_rq_append_bio -> ll_back_merge_fn
> 
> So some users might be relying on the smaller limit.

Note that this could happen even now, you just need sufficiently
horrible hardware to pass through for it.  But yes, for pscsi
this needs to look at the underlying device, and increasing the
limit might be a good point to do that.  I'm not sure it's worth
to add user configuration, though.

Re: [PATCH] tcm_loop: Increase maximum request size

[Nikos Tsironis]

On 12/7/22 20:35, Christoph Hellwig wrote:
> On Wed, Dec 07, 2022 at 12:29:56PM -0600, Mike Christie wrote:
>> I think you need to make this configurable.
>>
>> If you use loop with pscsi, then the sgl that loop now gets might be too
>> big for the backend device so we now fail in:
>>
>> pscsi_map_sg -> blk_rq_append_bio -> ll_back_merge_fn
>>
>> So some users might be relying on the smaller limit.
> 
> Note that this could happen even now, you just need sufficiently
> horrible hardware to pass through for it.  But yes, for pscsi
> this needs to look at the underlying device, and increasing the
> limit might be a good point to do that.  I'm not sure it's worth
> to add user configuration, though.

Thanks for the feedback.

What should I do? Change pscsi to look at the underlying device limits?

Nikos

Re: [PATCH] tcm_loop: Increase maximum request size

[Mike Christie]

On 12/19/22 8:39 AM, Nikos Tsironis wrote:
> On 12/7/22 20:35, Christoph Hellwig wrote:
>> On Wed, Dec 07, 2022 at 12:29:56PM -0600, Mike Christie wrote:
>>> I think you need to make this configurable.
>>>
>>> If you use loop with pscsi, then the sgl that loop now gets might be too
>>> big for the backend device so we now fail in:
>>>
>>> pscsi_map_sg -> blk_rq_append_bio -> ll_back_merge_fn
>>>
>>> So some users might be relying on the smaller limit.
>>
>> Note that this could happen even now, you just need sufficiently
>> horrible hardware to pass through for it.  But yes, for pscsi
>> this needs to look at the underlying device, and increasing the
>> limit might be a good point to do that.  I'm not sure it's worth
>> to add user configuration, though.
> 
> Thanks for the feedback.
> 
> What should I do? Change pscsi to look at the underlying device limits?

Yes. I think there might be 2 things to do here:

1. SCSI VPD values.

I think we would do something similar to iblock_configure_device
where when we setup the device we look at the underlying device's limits
and then set the se_device's limits. Those values then get reported to
the initiator in the VPD info, so that would be useful for normal drivers
like iscsi and FC.

For the max segments there's not a SCSI VPD value, but we have this code:

        /*
         * Set MAXIMUM TRANSFER LENGTH
         *
         * XXX: Currently assumes single PAGE_SIZE per scatterlist for fabrics
         * enforcing maximum HW scatter-gather-list entry limit
         */
        if (cmd->se_tfo->max_data_sg_nents) {
                mtl = (cmd->se_tfo->max_data_sg_nents * PAGE_SIZE) /
                       dev->dev_attrib.block_size;
        }
        put_unaligned_be32(min_not_zero(mtl, dev->dev_attrib.hw_max_sectors), &buf[8]);

which would limit the IO size. max_data_sg_nents is only currently a fabric
driver (front end drivers like iscsi, fc, etc) limit, so we would need to make
that more generic so backends like pscsi can set it.


2. For loop we have the sg_tablesize setting like you saw, but the problem is
that it's host level and we don't know the devices when we add the host.

I think we would have to modify tcm_loop_port_link so if adds the device
successfully, we call blk_queue_max_segments.

Christoph is that what you were thinking?

Re: [PATCH] tcm_loop: Increase maximum request size

[Christoph Hellwig]

> Christoph is that what you were thinking?

Yes.