diff mbox series

[5.10/5.15] scsi: sd: Revert "scsi: sd: Remove a local variable"

Message ID 20221029025837.1258377-1-yukuai1@huaweicloud.com (mailing list archive)
State Superseded
Headers show
Series [5.10/5.15] scsi: sd: Revert "scsi: sd: Remove a local variable" | expand

Commit Message

Yu Kuai Oct. 29, 2022, 2:58 a.m. UTC 
From: Yu Kuai <yukuai3@huawei.com>

This reverts commit 84f7a9de0602704bbec774a6c7f7c8c4994bee9c.

Because it introduces a problem that rq->__data_len is set to the wrong
value.

before this patch:
1) nr_bytes = rq->__data_len
2) rq->__data_len = sdp->sector_size
3) scsi_init_io()
4) rq->__data_len = nr_bytes

after this patch:
1) rq->__data_len = sdp->sector_size
2) scsi_init_io()
3) rq->__data_len = rq->__data_len -> __data_len is wrong

It will cause that io can only complete one segment each time, and the io
will requeue in scsi_io_completion_action(), which will cause severe
performance degradation.

Fixes: 84f7a9de0602 ("scsi: sd: Remove a local variable")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/scsi/sd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Bart Van Assche Oct. 29, 2022, 11:10 p.m. UTC | #1 
On 10/28/22 19:58, Yu Kuai wrote:
> This reverts commit 84f7a9de0602704bbec774a6c7f7c8c4994bee9c.
> 
> Because it introduces a problem that rq->__data_len is set to the wrong
> value.
> 
> before this patch:
> 1) nr_bytes = rq->__data_len
> 2) rq->__data_len = sdp->sector_size
> 3) scsi_init_io()
> 4) rq->__data_len = nr_bytes
> 
> after this patch:
> 1) rq->__data_len = sdp->sector_size
> 2) scsi_init_io()
> 3) rq->__data_len = rq->__data_len -> __data_len is wrong
> 
> It will cause that io can only complete one segment each time, and the io
> will requeue in scsi_io_completion_action(), which will cause severe
> performance degradation.

It's probably worth mentioning that the code affected by this patch has 
been removed from the master branch and hence that this patch is only 
needed for stable kernels. Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Greg KH Oct. 31, 2022, 7:27 a.m. UTC | #2 
On Sat, Oct 29, 2022 at 04:10:47PM -0700, Bart Van Assche wrote:
> On 10/28/22 19:58, Yu Kuai wrote:
> > This reverts commit 84f7a9de0602704bbec774a6c7f7c8c4994bee9c.
> > 
> > Because it introduces a problem that rq->__data_len is set to the wrong
> > value.
> > 
> > before this patch:
> > 1) nr_bytes = rq->__data_len
> > 2) rq->__data_len = sdp->sector_size
> > 3) scsi_init_io()
> > 4) rq->__data_len = nr_bytes
> > 
> > after this patch:
> > 1) rq->__data_len = sdp->sector_size
> > 2) scsi_init_io()
> > 3) rq->__data_len = rq->__data_len -> __data_len is wrong
> > 
> > It will cause that io can only complete one segment each time, and the io
> > will requeue in scsi_io_completion_action(), which will cause severe
> > performance degradation.
> 
> It's probably worth mentioning that the code affected by this patch has been
> removed from the master branch and hence that this patch is only needed for
> stable kernels. Anyway:

Yes, that needs to be in the changelog in lots of detail.

Yu, please fix this up and resend.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index de6640ad1943..1e887c11e83d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1072,6 +1072,7 @@  static blk_status_t sd_setup_write_same_cmnd(struct scsi_cmnd *cmd)
 	struct bio *bio = rq->bio;
 	u64 lba = sectors_to_logical(sdp, blk_rq_pos(rq));
 	u32 nr_blocks = sectors_to_logical(sdp, blk_rq_sectors(rq));
+	unsigned int nr_bytes = blk_rq_bytes(rq);
 	blk_status_t ret;
 
 	if (sdkp->device->no_write_same)
@@ -1108,7 +1109,7 @@  static blk_status_t sd_setup_write_same_cmnd(struct scsi_cmnd *cmd)
 	 */
 	rq->__data_len = sdp->sector_size;
 	ret = scsi_alloc_sgtables(cmd);
-	rq->__data_len = blk_rq_bytes(rq);
+	rq->__data_len = nr_bytes;
 
 	return ret;
 }