Re: [PATCH 1/2] target: Don't arbitrary limit I/O size to fabric_max_sectors
On Fri, 2015-01-09 at 15:43 -0500, Martin K. Petersen wrote: > > "nab" == Nicholas A Bellinger writes: > > nab> The concern is when older hardware drivers are reporting say > nab> queue_max_hw_sectors=128 with initiators are not actively honoring > nab> block limits EVPD MAXIMUM TRANSFER LENGTH, that would result in > nab> I/Os over 64K generating exception status. > > Given that you're already splitting I/Os in IBLOCK I think it would > probably be better to pick a size you're comfortable with. 4 or 8 megs > sound reasonable to be > I'm now thinking to go just ahead and drop the hw_max_sectors check in sbc_parse_cdb() as well, and leave hw_max_sectors as a purely informational attribute representing the granularity each backend driver and/or subsystem is splitting I/Os upon. Which would mean there is no longer a maximum I/O size enforced by the target. This used to be the case in < v3.5 code, before Roland added fabric_max_sectors to enforce the global 4 MB maximum that people recently have been starting to run up against with initiators not honoring block limits VPD. That said, I don't recall there being any issues with the earlier code not enforcing a maximum I/O size, so in order to avoid the same problem in the future it probably makes the sense to go ahead process any arbitrary sized I/O. Re-spinning a -v2 with this in mind. --nab -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] target: Don't arbitrary limit I/O size to fabric_max_sectors
> "nab" == Nicholas A Bellinger writes: nab> The concern is when older hardware drivers are reporting say nab> queue_max_hw_sectors=128 with initiators are not actively honoring nab> block limits EVPD MAXIMUM TRANSFER LENGTH, that would result in nab> I/Os over 64K generating exception status. Given that you're already splitting I/Os in IBLOCK I think it would probably be better to pick a size you're comfortable with. 4 or 8 megs sound reasonable to be -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] target: Don't arbitrary limit I/O size to fabric_max_sectors
On Thu, 2015-01-08 at 14:49 -0800, Nicholas A. Bellinger wrote: > On Thu, 2015-01-08 at 09:37 -0500, Martin K. Petersen wrote: > > > "nab" == Nicholas A Bellinger writes: > > > > nab> IIRC, most modern hardware is reporting a large enough value for > > nab> queue_max_hw_sectors() to support 8 MB I/Os, but I'm thinking that > > nab> this could end up being problematic for older hardware that is > > nab> reporting much smaller values. > > > > Reporting queue_max_hw_sectors sounds sane to me. > > > > What's your concern wrt. older hardware? > > > > The target is still enforcing it's own hw_max_sectors in sbc_parse_cdb() > based upon what queue_max_hw_sectors() reports for IBLOCK, and will > throw an exception for I/Os who's sector count exceeds this maximum. > > The concern is when older hardware drivers are reporting say > queue_max_hw_sectors=128 with initiators are not actively honoring block > limits EVPD MAXIMUM TRANSFER LENGTH, that would result in I/Os over 64K > generating exception status. > > So the question is what is a sane minimum for IBLOCK's hw_max_sectors so > that large I/Os (say up to 8 MB) aren't rejected by sbc_parse_sbc(), and > don't trigger the subsequent checks in generic_make_request() -> > generic_make_request_checks(). > Christoph, any input on this..? --nab -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] target: Don't arbitrary limit I/O size to fabric_max_sectors
On Thu, 2015-01-08 at 09:37 -0500, Martin K. Petersen wrote: > > "nab" == Nicholas A Bellinger writes: > > nab> IIRC, most modern hardware is reporting a large enough value for > nab> queue_max_hw_sectors() to support 8 MB I/Os, but I'm thinking that > nab> this could end up being problematic for older hardware that is > nab> reporting much smaller values. > > Reporting queue_max_hw_sectors sounds sane to me. > > What's your concern wrt. older hardware? > The target is still enforcing it's own hw_max_sectors in sbc_parse_cdb() based upon what queue_max_hw_sectors() reports for IBLOCK, and will throw an exception for I/Os who's sector count exceeds this maximum. The concern is when older hardware drivers are reporting say queue_max_hw_sectors=128 with initiators are not actively honoring block limits EVPD MAXIMUM TRANSFER LENGTH, that would result in I/Os over 64K generating exception status. So the question is what is a sane minimum for IBLOCK's hw_max_sectors so that large I/Os (say up to 8 MB) aren't rejected by sbc_parse_sbc(), and don't trigger the subsequent checks in generic_make_request() -> generic_make_request_checks(). --nab -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] target: Don't arbitrary limit I/O size to fabric_max_sectors
> "nab" == Nicholas A Bellinger writes: nab> IIRC, most modern hardware is reporting a large enough value for nab> queue_max_hw_sectors() to support 8 MB I/Os, but I'm thinking that nab> this could end up being problematic for older hardware that is nab> reporting much smaller values. Reporting queue_max_hw_sectors sounds sane to me. What's your concern wrt. older hardware? -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] target: Don't arbitrary limit I/O size to fabric_max_sectors
Hey Christoph, Adding CC' for MKP. On Wed, 2015-01-07 at 00:24 +, Nicholas A. Bellinger wrote: > From: Nicholas Bellinger > > This patch avoids the arbitrary limiting of I/O size to fabric_max_sectors, > which currently has a hardcoded max of 8192 (4 MB for 512 byte sector > devices). > > This is problematic because Linux initiators have only recently started > to honor block limits MAXIMUM TRANSFER LENGTH, and other non-Linux based > initiators (eg: Fibre Channel) can also generate I/Os larger than 4 MB. > > Currently when this happens, the following message will appear on the > target resulting in I/Os being returned with non recoverable status: > > SCSI OP 28h with too big sectors 16384 exceeds fabric_max_sectors: 8192 > > Instead, use the existing hw_max_sectors value to determine the maximum > sized I/O the backend is capable of supporting. Also, change IBLOCK > backend driver to set hw_max_sectors based on queue_max_hw_sectors(), > instead of UINT_MAX. > > Reported-by: Lance Gropper > Reported-by: Stefan Priebe > Cc: Christoph Hellwig > Cc: Roland Dreier > Signed-off-by: Nicholas Bellinger > --- > drivers/target/target_core_device.c | 8 > drivers/target/target_core_iblock.c | 2 +- > drivers/target/target_core_sbc.c| 7 --- > drivers/target/target_core_spc.c| 5 + > 4 files changed, 6 insertions(+), 16 deletions(-) > > diff --git a/drivers/target/target_core_iblock.c > b/drivers/target/target_core_iblock.c > index 3efff94..5795cd8 100644 > --- a/drivers/target/target_core_iblock.c > +++ b/drivers/target/target_core_iblock.c > @@ -124,7 +124,7 @@ static int iblock_configure_device(struct se_device *dev) > q = bdev_get_queue(bd); > > dev->dev_attrib.hw_block_size = bdev_logical_block_size(bd); > - dev->dev_attrib.hw_max_sectors = UINT_MAX; > + dev->dev_attrib.hw_max_sectors = queue_max_hw_sectors(q); > dev->dev_attrib.hw_queue_depth = q->nr_requests; > > /* After thinking about this a bit more, I'm starting to have second thoughts about using queue_max_hw_sectors() to set hw_max_sectors for IBLOCK. IIRC, most modern hardware is reporting a large enough value for queue_max_hw_sectors() to support 8 MB I/Os, but I'm thinking that this could end up being problematic for older hardware that is reporting much smaller values. Is there any reason why the underlying queue_max_hw_sectors() is going to trigger an -EIO failure in generic_make_request_checks() when a IBLOCK dispatched bio exceeds the reported size..? What are your thoughts for a sane default here..? --nab -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
