* [PATCH] sd: Optimal I/O size is in bytes, not sectors
@ 2016-01-20 16:01 Martin K. Petersen
2016-01-20 16:11 ` Johannes Thumshirn
` (5 more replies)
0 siblings, 6 replies; 12+ messages in thread
From: Martin K. Petersen @ 2016-01-20 16:01 UTC (permalink / raw)
To: linux-scsi; +Cc: borntraeger, Martin K. Petersen
Commit ca369d51b3e1 ("block/sd: Fix device-imposed transfer length
limits") accidentally switched optimal I/O size reporting from bytes to
block layer sectors.
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
drivers/scsi/sd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4e08d1cd704d..ec163d08f6c3 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2893,7 +2893,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_CACHE_SIZE)
rw_max = q->limits.io_opt =
- logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
+ sdkp->opt_xfer_blocks * sdp->sector_size;
else
rw_max = BLK_DEF_MAX_SECTORS;
--
2.5.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors
2016-01-20 16:01 [PATCH] sd: Optimal I/O size is in bytes, not sectors Martin K. Petersen
@ 2016-01-20 16:11 ` Johannes Thumshirn
2016-01-20 16:19 ` James Bottomley
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Johannes Thumshirn @ 2016-01-20 16:11 UTC (permalink / raw)
To: Martin K. Petersen; +Cc: linux-scsi, borntraeger
On Wed, Jan 20, 2016 at 11:01:23AM -0500, Martin K. Petersen wrote:
> Commit ca369d51b3e1 ("block/sd: Fix device-imposed transfer length
> limits") accidentally switched optimal I/O size reporting from bytes to
> block layer sectors.
>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> drivers/scsi/sd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4e08d1cd704d..ec163d08f6c3 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2893,7 +2893,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
> sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
> sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_CACHE_SIZE)
> rw_max = q->limits.io_opt =
> - logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
> + sdkp->opt_xfer_blocks * sdp->sector_size;
> else
> rw_max = BLK_DEF_MAX_SECTORS;
>
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors
2016-01-20 16:01 [PATCH] sd: Optimal I/O size is in bytes, not sectors Martin K. Petersen
2016-01-20 16:11 ` Johannes Thumshirn
@ 2016-01-20 16:19 ` James Bottomley
2016-01-20 16:40 ` Martin K. Petersen
2016-01-20 16:19 ` Matthew R. Ochs
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: James Bottomley @ 2016-01-20 16:19 UTC (permalink / raw)
To: Martin K. Petersen, linux-scsi; +Cc: borntraeger
On Wed, 2016-01-20 at 11:01 -0500, Martin K. Petersen wrote:
> Commit ca369d51b3e1 ("block/sd: Fix device-imposed transfer length
> limits") accidentally switched optimal I/O size reporting from bytes
> to
> block layer sectors.
>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> drivers/scsi/sd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4e08d1cd704d..ec163d08f6c3 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2893,7 +2893,7 @@ static int sd_revalidate_disk(struct gendisk
> *disk)
> sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
> sdkp->opt_xfer_blocks * sdp->sector_size >=
> PAGE_CACHE_SIZE)
> rw_max = q->limits.io_opt =
> - logical_to_sectors(sdp, sdkp
> ->opt_xfer_blocks);
> + sdkp->opt_xfer_blocks * sdp->sector_size;
> else
> rw_max = BLK_DEF_MAX_SECTORS;
>
We should mark the commit causing the problems, which went into 4.4 if
I remember correctly:
Fixes: ca369d51b3e1649be4a72addd6d6a168cfb3f537
Cc: stable@vger.kernel.org # 4.4+
Reviewed-by: James E.J. Bottomley <James.Bottomley@HansenPartnership.com>
James
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors
2016-01-20 16:01 [PATCH] sd: Optimal I/O size is in bytes, not sectors Martin K. Petersen
2016-01-20 16:11 ` Johannes Thumshirn
2016-01-20 16:19 ` James Bottomley
@ 2016-01-20 16:19 ` Matthew R. Ochs
2016-01-20 17:17 ` Ewan Milne
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Matthew R. Ochs @ 2016-01-20 16:19 UTC (permalink / raw)
To: Martin K. Petersen; +Cc: linux-scsi, borntraeger
Reviewed-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors
2016-01-20 16:19 ` James Bottomley
@ 2016-01-20 16:40 ` Martin K. Petersen
2016-01-20 16:47 ` James Bottomley
0 siblings, 1 reply; 12+ messages in thread
From: Martin K. Petersen @ 2016-01-20 16:40 UTC (permalink / raw)
To: James Bottomley; +Cc: Martin K. Petersen, linux-scsi, borntraeger
>>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:
James> We should mark the commit causing the problems, which went into
James> 4.4 if I remember correctly:
James> Fixes: ca369d51b3e1649be4a72addd6d6a168cfb3f537 Cc:
James> stable@vger.kernel.org # 4.4+ Reviewed-by: James E.J. Bottomley
James> <James.Bottomley@HansenPartnership.com>
I'll add the tags. The reason I didn't explicitly put 4.4+ is that the
original commit has made its way quite far in various stable trees by
now.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors
2016-01-20 16:40 ` Martin K. Petersen
@ 2016-01-20 16:47 ` James Bottomley
2016-01-22 8:16 ` Greg Kroah-Hartman
0 siblings, 1 reply; 12+ messages in thread
From: James Bottomley @ 2016-01-20 16:47 UTC (permalink / raw)
To: Martin K. Petersen; +Cc: linux-scsi, borntraeger, Greg Kroah-Hartman
On Wed, 2016-01-20 at 11:40 -0500, Martin K. Petersen wrote:
> > > > > > "James" == James Bottomley <
> > > > > > James.Bottomley@HansenPartnership.com> writes:
>
> James> We should mark the commit causing the problems, which went
> into
> James> 4.4 if I remember correctly:
>
> James> Fixes: ca369d51b3e1649be4a72addd6d6a168cfb3f537 Cc:
> James> stable@vger.kernel.org # 4.4+ Reviewed-by: James E.J.
> Bottomley
> James> <James.Bottomley@HansenPartnership.com>
>
> I'll add the tags. The reason I didn't explicitly put 4.4+ is that
> the original commit has made its way quite far in various stable
> trees by now.
It has? It wasn't tagged for stable. However, if it got applied to
stable trees, then we should certainly backport further. I sort of
hope the stable process uses the Fixes: tag to decide when to backport
anyway, since the stable commit contains the original upstream sha256,
they can certainly identify it.
Greg, are we OK not to bother with qualifying the cc: stable tag if we
have a Fixes tag, or do you still want to see both? Perhaps an
addition to stable_kernel_rules.txt mentioning Fixes: might be useful
as well.
Thanks,
James
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors
2016-01-20 16:01 [PATCH] sd: Optimal I/O size is in bytes, not sectors Martin K. Petersen
` (2 preceding siblings ...)
2016-01-20 16:19 ` Matthew R. Ochs
@ 2016-01-20 17:17 ` Ewan Milne
2016-01-20 18:28 ` Christian Borntraeger
2016-05-12 4:04 ` Fam Zheng
5 siblings, 0 replies; 12+ messages in thread
From: Ewan Milne @ 2016-01-20 17:17 UTC (permalink / raw)
To: Martin K. Petersen; +Cc: linux-scsi, borntraeger
On Wed, 2016-01-20 at 11:01 -0500, Martin K. Petersen wrote:
> Commit ca369d51b3e1 ("block/sd: Fix device-imposed transfer length
> limits") accidentally switched optimal I/O size reporting from bytes to
> block layer sectors.
>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> drivers/scsi/sd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4e08d1cd704d..ec163d08f6c3 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2893,7 +2893,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
> sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
> sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_CACHE_SIZE)
> rw_max = q->limits.io_opt =
> - logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
> + sdkp->opt_xfer_blocks * sdp->sector_size;
> else
> rw_max = BLK_DEF_MAX_SECTORS;
>
Reviewed-by: Ewan D. Milne <emilne@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors
2016-01-20 16:01 [PATCH] sd: Optimal I/O size is in bytes, not sectors Martin K. Petersen
` (3 preceding siblings ...)
2016-01-20 17:17 ` Ewan Milne
@ 2016-01-20 18:28 ` Christian Borntraeger
2016-05-12 4:04 ` Fam Zheng
5 siblings, 0 replies; 12+ messages in thread
From: Christian Borntraeger @ 2016-01-20 18:28 UTC (permalink / raw)
To: Martin K. Petersen, linux-scsi
On 01/20/2016 05:01 PM, Martin K. Petersen wrote:
> Commit ca369d51b3e1 ("block/sd: Fix device-imposed transfer length
> limits") accidentally switched optimal I/O size reporting from bytes to
> block layer sectors.
>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> drivers/scsi/sd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4e08d1cd704d..ec163d08f6c3 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2893,7 +2893,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
> sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
> sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_CACHE_SIZE)
> rw_max = q->limits.io_opt =
> - logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
> + sdkp->opt_xfer_blocks * sdp->sector_size;
> else
> rw_max = BLK_DEF_MAX_SECTORS;
>
the error message is gone
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors
2016-01-20 16:47 ` James Bottomley
@ 2016-01-22 8:16 ` Greg Kroah-Hartman
0 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2016-01-22 8:16 UTC (permalink / raw)
To: James Bottomley; +Cc: Martin K. Petersen, linux-scsi, borntraeger
On Wed, Jan 20, 2016 at 08:47:29AM -0800, James Bottomley wrote:
> On Wed, 2016-01-20 at 11:40 -0500, Martin K. Petersen wrote:
> > > > > > > "James" == James Bottomley <
> > > > > > > James.Bottomley@HansenPartnership.com> writes:
> >
> > James> We should mark the commit causing the problems, which went
> > into
> > James> 4.4 if I remember correctly:
> >
> > James> Fixes: ca369d51b3e1649be4a72addd6d6a168cfb3f537 Cc:
> > James> stable@vger.kernel.org # 4.4+ Reviewed-by: James E.J.
> > Bottomley
> > James> <James.Bottomley@HansenPartnership.com>
> >
> > I'll add the tags. The reason I didn't explicitly put 4.4+ is that
> > the original commit has made its way quite far in various stable
> > trees by now.
>
> It has? It wasn't tagged for stable. However, if it got applied to
> stable trees, then we should certainly backport further. I sort of
> hope the stable process uses the Fixes: tag to decide when to backport
> anyway, since the stable commit contains the original upstream sha256,
> they can certainly identify it.
>
> Greg, are we OK not to bother with qualifying the cc: stable tag if we
> have a Fixes tag, or do you still want to see both? Perhaps an
> addition to stable_kernel_rules.txt mentioning Fixes: might be useful
> as well.
I want to see a stable tag if you know it is relevant. I do, when I
have the time, dig through the fixes: tags to see if they should also be
applied, but I don't always guarantee that I will get to them at all, so
don't count on that as a way to get a patch into a stable release.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors
2016-01-20 16:01 [PATCH] sd: Optimal I/O size is in bytes, not sectors Martin K. Petersen
` (4 preceding siblings ...)
2016-01-20 18:28 ` Christian Borntraeger
@ 2016-05-12 4:04 ` Fam Zheng
2016-05-12 17:13 ` Tom Yan
2016-05-13 2:32 ` Martin K. Petersen
5 siblings, 2 replies; 12+ messages in thread
From: Fam Zheng @ 2016-05-12 4:04 UTC (permalink / raw)
To: Martin K. Petersen; +Cc: linux-scsi, borntraeger
On Wed, 01/20 11:01, Martin K. Petersen wrote:
> Commit ca369d51b3e1 ("block/sd: Fix device-imposed transfer length
> limits") accidentally switched optimal I/O size reporting from bytes to
> block layer sectors.
>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> drivers/scsi/sd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4e08d1cd704d..ec163d08f6c3 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2893,7 +2893,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
> sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
> sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_CACHE_SIZE)
> rw_max = q->limits.io_opt =
> - logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
> + sdkp->opt_xfer_blocks * sdp->sector_size;
Hi Martin,
This looks wrong to me, maybe I'm missing the obvious? Here
sdkp->opt_xfer_blocks is in block size unit, and rw_max is in byte unit.
Following is:
else
rw_max = BLK_DEF_MAX_SECTORS;
Which seems in sector unit, and is already making above change suspicious, and
further down:
/* Combine with controller limits */
q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
looks like a unit mismatch to me. IIUC q->limits.max_sectors _is_ in sector
unit, similar to queue_max_hw_sectors().
Is the error reported by Christian fixed just because we are setting an
incorrect high max?
(I noticed this when I see I/O error because a virtio-scsi guest starts to
issue large reads that are rejected by host device.)
Fam
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors
2016-05-12 4:04 ` Fam Zheng
@ 2016-05-12 17:13 ` Tom Yan
2016-05-13 2:32 ` Martin K. Petersen
1 sibling, 0 replies; 12+ messages in thread
From: Tom Yan @ 2016-05-12 17:13 UTC (permalink / raw)
To: Fam Zheng; +Cc: Martin K. Petersen, linux-scsi, borntraeger
On 12 May 2016 at 12:04, Fam Zheng <famz@redhat.com> wrote:
>
> Following is:
>
> else
> rw_max = BLK_DEF_MAX_SECTORS;
>
> Which seems in sector unit, and is already making above change suspicious, and
Yes BLK_DEF_MAX_SECTORS is in sector unit. It has been bumped from
1024 to 2560 not long ago btw:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/include/linux/blkdev.h?id=d2be537c3ba3568acd79cd178327b842e60d035e
I suppose the code should be changed to something like this:
...
{
rw_max = sdkp->opt_xfer_blocks;
q->limits.io_opt = rw_max * sdp->sector_size;
}
> further down:
>
> /* Combine with controller limits */
> q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
>
> looks like a unit mismatch to me. IIUC q->limits.max_sectors _is_ in sector
> unit, similar to queue_max_hw_sectors().
>
> Is the error reported by Christian fixed just because we are setting an
> incorrect high max?
However in the worst case max_sectors should still be limited by
max_hw_sectors, which apparently depends on the host. For example uas
devices (without any quirks) have max_hw_sectors set to 1024
(SCSI_DEFAULT_MAX_SECTORS defined in include/scsi/scsi_host.h).
Speaking of this, shouldn't SCSI_DEFAULT_MAX_SECTORS be increased? For
example at least to the new BLK_DEF_MAX_SECTORS (2560), or maybe even
SD_DEF_XFER_BLOCKS (65535)?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors
2016-05-12 4:04 ` Fam Zheng
2016-05-12 17:13 ` Tom Yan
@ 2016-05-13 2:32 ` Martin K. Petersen
1 sibling, 0 replies; 12+ messages in thread
From: Martin K. Petersen @ 2016-05-13 2:32 UTC (permalink / raw)
To: Fam Zheng; +Cc: Martin K. Petersen, linux-scsi, borntraeger
>>>>> "Fam" == Fam Zheng <famz@redhat.com> writes:
Fam,
Fam> This looks wrong to me, maybe I'm missing the obvious? Here sdkp->
Fam> opt_xfer_blocks is in block size unit, and rw_max is in byte sdkp->
Fam> unit.
Oh, joy. It's the commit that keeps on giving!
Back when the I/O topology was added we were set on eradicating block
layer sectors from the stack and count everything in bytes. So the new
fields were added as bytes but for various reasons the existing values
were never transitioned. And now we have an error prone interface. I
think that for 4.8 I'll fix this up properly. I'll post a fix for 4.7
and older tomorrow...
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-05-13 2:32 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-20 16:01 [PATCH] sd: Optimal I/O size is in bytes, not sectors Martin K. Petersen
2016-01-20 16:11 ` Johannes Thumshirn
2016-01-20 16:19 ` James Bottomley
2016-01-20 16:40 ` Martin K. Petersen
2016-01-20 16:47 ` James Bottomley
2016-01-22 8:16 ` Greg Kroah-Hartman
2016-01-20 16:19 ` Matthew R. Ochs
2016-01-20 17:17 ` Ewan Milne
2016-01-20 18:28 ` Christian Borntraeger
2016-05-12 4:04 ` Fam Zheng
2016-05-12 17:13 ` Tom Yan
2016-05-13 2:32 ` Martin K. Petersen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.