| Message ID | 20220929115504.23806-1-ntsironis@arrikto.com |
|---|---|
| State | New |
| Headers | show |
| Series | tcm_loop: Increase maximum request size | expand |
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.
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
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.
Hello, This is a kind reminder for this patch. Do you require any changes to merge it? Nikos
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.
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.
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
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?
> Christoph is that what you were thinking?
Yes.
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,
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(-)