[PATCH] sd: Optimal I/O size is in bytes, not sectors

[PATCH] sd: Optimal I/O size is in bytes, not sectors

[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

Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors

[Johannes Thumshirn]

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

Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors

[James Bottomley]

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

Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors

[Martin K. Petersen]

>>>>> "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

Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors

[James Bottomley]

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

Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors

[Greg Kroah-Hartman]

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

Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors

[Matthew R. Ochs]

Reviewed-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com> 

Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors

[Ewan Milne]

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>

Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors

[Christian Borntraeger]

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>

Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors

[Fam Zheng]

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

Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors

[Tom Yan]

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)?

Re: [PATCH] sd: Optimal I/O size is in bytes, not sectors

[Martin K. Petersen]

>>>>> "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