* [PATCH] scsi: sd: Optimal I/O size should be a multiple of physical block size @ 2019-02-08 23:41 Martin K. Petersen 2019-02-12 8:04 ` Christoph Hellwig 0 siblings, 1 reply; 5+ messages in thread From: Martin K. Petersen @ 2019-02-08 23:41 UTC (permalink / raw) To: linux-scsi; +Cc: Martin K. Petersen, stable It was reported that some devices report an OPTIMAL TRANSFER LENGTH of 0xFFFF blocks. That looks bogus, especially for a device with a 4096-byte physical block size. Ignore OPTIMAL TRANSFER LENGTH if it is not a multiple of the device's reported physical block size. Cc: <stable@vger.kernel.org> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199759 Reported-by: Christoph Anton Mitterer <calestyo@scientia.net> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> --- Before: NAME ALIGNMENT MIN-IO OPT-IO PHY-SEC LOG-SEC ROTA SCHED RQ-SIZE RA WSAME sda 0 4096 33553920 4096 512 0 mq-deadline 256 128 32M After: NAME ALIGNMENT MIN-IO OPT-IO PHY-SEC LOG-SEC ROTA SCHED RQ-SIZE RA WSAME sda 0 4096 0 4096 512 0 mq-deadline 256 4096 32M --- drivers/scsi/sd.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 9aa409b38765..d5ede5d65838 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -3060,7 +3060,7 @@ static int sd_revalidate_disk(struct gendisk *disk) struct request_queue *q = sdkp->disk->queue; sector_t old_capacity = sdkp->capacity; unsigned char *buffer; - unsigned int dev_max, rw_max; + unsigned int dev_max, rw_max, opt_xfer_bytes; SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp, "sd_revalidate_disk\n")); @@ -3119,13 +3119,16 @@ static int sd_revalidate_disk(struct gendisk *disk) /* * Determine the device's preferred I/O size for reads and writes - * unless the reported value is unreasonably small, large, or - * garbage. + * unless the reported value is unreasonably small, large, not a + * multiple of the physical block size, or simply garbage. */ + opt_xfer_bytes = logical_to_bytes(sdp, sdkp->opt_xfer_blocks); + if (sdkp->opt_xfer_blocks && sdkp->opt_xfer_blocks <= dev_max && sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS && - logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) { + opt_xfer_bytes >= PAGE_SIZE && + (opt_xfer_bytes & (sdkp->physical_block_size - 1)) != 0) { q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks); rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks); } else -- 2.19.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] scsi: sd: Optimal I/O size should be a multiple of physical block size 2019-02-08 23:41 [PATCH] scsi: sd: Optimal I/O size should be a multiple of physical block size Martin K. Petersen @ 2019-02-12 8:04 ` Christoph Hellwig 2019-02-12 21:21 ` [PATCH v2] " Martin K. Petersen 0 siblings, 1 reply; 5+ messages in thread From: Christoph Hellwig @ 2019-02-12 8:04 UTC (permalink / raw) To: Martin K. Petersen; +Cc: linux-scsi, stable On Fri, Feb 08, 2019 at 06:41:47PM -0500, Martin K. Petersen wrote: > It was reported that some devices report an OPTIMAL TRANSFER LENGTH of > 0xFFFF blocks. That looks bogus, especially for a device with a > 4096-byte physical block size. > > Ignore OPTIMAL TRANSFER LENGTH if it is not a multiple of the device's > reported physical block size. Looks generally fine, but should we log a warning in case of a bogus OPTIMAL TRANSFER LENGTH? ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] scsi: sd: Optimal I/O size should be a multiple of physical block size 2019-02-12 8:04 ` Christoph Hellwig @ 2019-02-12 21:21 ` Martin K. Petersen 2019-02-16 3:00 ` Martin K. Petersen 2019-02-22 14:30 ` Christoph Hellwig 0 siblings, 2 replies; 5+ messages in thread From: Martin K. Petersen @ 2019-02-12 21:21 UTC (permalink / raw) To: linux-scsi; +Cc: Martin K. Petersen, stable It was reported that some devices report an OPTIMAL TRANSFER LENGTH of 0xFFFF blocks. That looks bogus, especially for a device with a 4096-byte physical block size. Ignore OPTIMAL TRANSFER LENGTH if it is not a multiple of the device's reported physical block size. To make the sanity checking conditionals more readable--and to facilitate printing warnings--relocate the checking to a helper function. No functional change aside from the printks. Cc: <stable@vger.kernel.org> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199759 Reported-by: Christoph Anton Mitterer <calestyo@scientia.net> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> --- v2: - Added warnings as requested by hch - Moved the checks to a helper function Before: NAME ALIGNMENT MIN-IO OPT-IO PHY-SEC LOG-SEC ROTA SCHED RQ-SIZE RA WSAME sda 0 4096 33553920 4096 512 0 mq-deadline 256 128 32M After: NAME ALIGNMENT MIN-IO OPT-IO PHY-SEC LOG-SEC ROTA SCHED RQ-SIZE RA WSAME sda 0 4096 0 4096 512 0 mq-deadline 256 4096 32M --- drivers/scsi/sd.c | 59 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 50 insertions(+), 9 deletions(-) diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 9aa409b38765..5dfe37b08d3b 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -3048,6 +3048,55 @@ static void sd_read_security(struct scsi_disk *sdkp, unsigned char *buffer) sdkp->security = 1; } +/* + * Determine the device's preferred I/O size for reads and writes + * unless the reported value is unreasonably small, large, not a + * multiple of the physical block size, or simply garbage. + */ +static bool sd_validate_opt_xfer_size(struct scsi_disk *sdkp, + unsigned int dev_max) +{ + struct scsi_device *sdp = sdkp->device; + unsigned int opt_xfer_bytes = + logical_to_bytes(sdp, sdkp->opt_xfer_blocks); + + if (sdkp->opt_xfer_blocks > dev_max) { + sd_first_printk(KERN_WARNING, sdkp, + "Optimal transfer size %u logical blocks " \ + "> dev_max (%u logical blocks)\n", + sdkp->opt_xfer_blocks, dev_max); + return false; + } + + if (sdkp->opt_xfer_blocks > SD_DEF_XFER_BLOCKS) { + sd_first_printk(KERN_WARNING, sdkp, + "Optimal transfer size %u logical blocks " \ + "> sd driver limit (%u logical blocks)\n", + sdkp->opt_xfer_blocks, SD_DEF_XFER_BLOCKS); + return false; + } + + if (opt_xfer_bytes < PAGE_SIZE) { + sd_first_printk(KERN_WARNING, sdkp, + "Optimal transfer size %u bytes < " \ + "PAGE_SIZE (%u bytes)\n", + opt_xfer_bytes, (unsigned int)PAGE_SIZE); + return false; + } + + if (opt_xfer_bytes & (sdkp->physical_block_size - 1)) { + sd_first_printk(KERN_WARNING, sdkp, + "Optimal transfer size %u bytes not a " \ + "multiple of physical block size (%u bytes)\n", + opt_xfer_bytes, sdkp->physical_block_size); + return false; + } + + sd_first_printk(KERN_INFO, sdkp, "Optimal transfer size %u bytes\n", + opt_xfer_bytes); + return true; +} + /** * sd_revalidate_disk - called the first time a new disk is seen, * performs disk spin up, read_capacity, etc. @@ -3117,15 +3166,7 @@ static int sd_revalidate_disk(struct gendisk *disk) dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks); q->limits.max_dev_sectors = logical_to_sectors(sdp, dev_max); - /* - * Determine the device's preferred I/O size for reads and writes - * unless the reported value is unreasonably small, large, or - * garbage. - */ - if (sdkp->opt_xfer_blocks && - sdkp->opt_xfer_blocks <= dev_max && - sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS && - logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) { + if (sd_validate_opt_xfer_size(sdkp, dev_max)) { q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks); rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks); } else -- 2.19.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] scsi: sd: Optimal I/O size should be a multiple of physical block size 2019-02-12 21:21 ` [PATCH v2] " Martin K. Petersen @ 2019-02-16 3:00 ` Martin K. Petersen 2019-02-22 14:30 ` Christoph Hellwig 1 sibling, 0 replies; 5+ messages in thread From: Martin K. Petersen @ 2019-02-16 3:00 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-scsi, stable Christoph? > It was reported that some devices report an OPTIMAL TRANSFER LENGTH of > 0xFFFF blocks. That looks bogus, especially for a device with a > 4096-byte physical block size. > > Ignore OPTIMAL TRANSFER LENGTH if it is not a multiple of the device's > reported physical block size. > > To make the sanity checking conditionals more readable--and to > facilitate printing warnings--relocate the checking to a helper > function. No functional change aside from the printks. > > Cc: <stable@vger.kernel.org> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199759 > Reported-by: Christoph Anton Mitterer <calestyo@scientia.net> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> > > --- > > v2: > - Added warnings as requested by hch > - Moved the checks to a helper function > > Before: > > NAME ALIGNMENT MIN-IO OPT-IO PHY-SEC LOG-SEC ROTA SCHED RQ-SIZE RA WSAME > sda 0 4096 33553920 4096 512 0 mq-deadline 256 128 32M > > After: > > NAME ALIGNMENT MIN-IO OPT-IO PHY-SEC LOG-SEC ROTA SCHED RQ-SIZE RA WSAME > sda 0 4096 0 4096 512 0 mq-deadline 256 4096 32M > --- > drivers/scsi/sd.c | 59 +++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 50 insertions(+), 9 deletions(-) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 9aa409b38765..5dfe37b08d3b 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -3048,6 +3048,55 @@ static void sd_read_security(struct scsi_disk *sdkp, unsigned char *buffer) > sdkp->security = 1; > } > > +/* > + * Determine the device's preferred I/O size for reads and writes > + * unless the reported value is unreasonably small, large, not a > + * multiple of the physical block size, or simply garbage. > + */ > +static bool sd_validate_opt_xfer_size(struct scsi_disk *sdkp, > + unsigned int dev_max) > +{ > + struct scsi_device *sdp = sdkp->device; > + unsigned int opt_xfer_bytes = > + logical_to_bytes(sdp, sdkp->opt_xfer_blocks); > + > + if (sdkp->opt_xfer_blocks > dev_max) { > + sd_first_printk(KERN_WARNING, sdkp, > + "Optimal transfer size %u logical blocks " \ > + "> dev_max (%u logical blocks)\n", > + sdkp->opt_xfer_blocks, dev_max); > + return false; > + } > + > + if (sdkp->opt_xfer_blocks > SD_DEF_XFER_BLOCKS) { > + sd_first_printk(KERN_WARNING, sdkp, > + "Optimal transfer size %u logical blocks " \ > + "> sd driver limit (%u logical blocks)\n", > + sdkp->opt_xfer_blocks, SD_DEF_XFER_BLOCKS); > + return false; > + } > + > + if (opt_xfer_bytes < PAGE_SIZE) { > + sd_first_printk(KERN_WARNING, sdkp, > + "Optimal transfer size %u bytes < " \ > + "PAGE_SIZE (%u bytes)\n", > + opt_xfer_bytes, (unsigned int)PAGE_SIZE); > + return false; > + } > + > + if (opt_xfer_bytes & (sdkp->physical_block_size - 1)) { > + sd_first_printk(KERN_WARNING, sdkp, > + "Optimal transfer size %u bytes not a " \ > + "multiple of physical block size (%u bytes)\n", > + opt_xfer_bytes, sdkp->physical_block_size); > + return false; > + } > + > + sd_first_printk(KERN_INFO, sdkp, "Optimal transfer size %u bytes\n", > + opt_xfer_bytes); > + return true; > +} > + > /** > * sd_revalidate_disk - called the first time a new disk is seen, > * performs disk spin up, read_capacity, etc. > @@ -3117,15 +3166,7 @@ static int sd_revalidate_disk(struct gendisk *disk) > dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks); > q->limits.max_dev_sectors = logical_to_sectors(sdp, dev_max); > > - /* > - * Determine the device's preferred I/O size for reads and writes > - * unless the reported value is unreasonably small, large, or > - * garbage. > - */ > - if (sdkp->opt_xfer_blocks && > - sdkp->opt_xfer_blocks <= dev_max && > - sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS && > - logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) { > + if (sd_validate_opt_xfer_size(sdkp, dev_max)) { > q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks); > rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks); > } else -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] scsi: sd: Optimal I/O size should be a multiple of physical block size 2019-02-12 21:21 ` [PATCH v2] " Martin K. Petersen 2019-02-16 3:00 ` Martin K. Petersen @ 2019-02-22 14:30 ` Christoph Hellwig 1 sibling, 0 replies; 5+ messages in thread From: Christoph Hellwig @ 2019-02-22 14:30 UTC (permalink / raw) To: Martin K. Petersen; +Cc: linux-scsi, stable On Tue, Feb 12, 2019 at 04:21:05PM -0500, Martin K. Petersen wrote: > It was reported that some devices report an OPTIMAL TRANSFER LENGTH of > 0xFFFF blocks. That looks bogus, especially for a device with a > 4096-byte physical block size. > > Ignore OPTIMAL TRANSFER LENGTH if it is not a multiple of the device's > reported physical block size. > > To make the sanity checking conditionals more readable--and to > facilitate printing warnings--relocate the checking to a helper > function. No functional change aside from the printks. > > Cc: <stable@vger.kernel.org> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199759 > Reported-by: Christoph Anton Mitterer <calestyo@scientia.net> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-22 14:30 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-08 23:41 [PATCH] scsi: sd: Optimal I/O size should be a multiple of physical block size Martin K. Petersen 2019-02-12 8:04 ` Christoph Hellwig 2019-02-12 21:21 ` [PATCH v2] " Martin K. Petersen 2019-02-16 3:00 ` Martin K. Petersen 2019-02-22 14:30 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).