| Message ID | 1453305683-22424-1-git-send-email-martin.petersen@oracle.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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>
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 -- 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: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
--
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
>>>>> "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.
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 -- 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
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> -- 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
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> -- 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
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 -- 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
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 -- 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
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)? -- 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
>>>>> "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...
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;
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(-)