* [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).