[PATCH] sd: Micro-optimize READ / WRITE CDB encoding

[PATCH] sd: Micro-optimize READ / WRITE CDB encoding

[Bart Van Assche]

Only compute 'threshold' if .last_sector_bug has been set. Reduce
the number of branches that has to be taken to check the starting
LBA and transfer length alignment. Optimize the encoding of the
READ(10), WRITE(10), READ(16), WRITE(16), READ(32) and WRITE(32)
CDB encoding.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/sd.c | 102 +++++++++++++++---------------------------------------
 1 file changed, 28 insertions(+), 74 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 67b50baa943c..83284a8fa2cd 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1025,7 +1025,6 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
 	struct gendisk *disk = rq->rq_disk;
 	struct scsi_disk *sdkp = scsi_disk(disk);
 	sector_t block = blk_rq_pos(rq);
-	sector_t threshold;
 	unsigned int this_count = blk_rq_sectors(rq);
 	unsigned int dif, dix;
 	bool zoned_write = sd_is_zoned(sdkp) && rq_data_dir(rq) == WRITE;
@@ -1071,20 +1070,22 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
 		goto out;
 	}
 
-	/*
-	 * Some SD card readers can't handle multi-sector accesses which touch
-	 * the last one or two hardware sectors.  Split accesses as needed.
-	 */
-	threshold = get_capacity(disk) - SD_LAST_BUGGY_SECTORS *
-		(sdp->sector_size / 512);
+	if (unlikely(sdp->last_sector_bug)) {
+		sector_t threshold;
 
-	if (unlikely(sdp->last_sector_bug && block + this_count > threshold)) {
-		if (block < threshold) {
-			/* Access up to the threshold but not beyond */
-			this_count = threshold - block;
-		} else {
+		/*
+		 * Some SD card readers can't handle multi-sector accesses
+		 * which touch the last one or two hardware sectors.  Split
+		 * accesses as needed.
+		 */
+		threshold = get_capacity(disk) - SD_LAST_BUGGY_SECTORS *
+			(sdp->sector_size / 512);
+		if (block >= threshold) {
 			/* Access only a single hardware sector */
 			this_count = sdp->sector_size / 512;
+		} else if (block + this_count > threshold) {
+			/* Access up to the threshold but not beyond */
+			this_count = threshold - block;
 		}
 	}
 
@@ -1102,34 +1103,17 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
 	 * and not force the scsi disk driver to use bounce buffers
 	 * for this.
 	 */
-	if (sdp->sector_size == 1024) {
-		if ((block & 1) || (blk_rq_sectors(rq) & 1)) {
+	if (sdp->sector_size != 512) {
+		if ((block | this_count) & (sdp->sector_size / 512 - 1)) {
 			scmd_printk(KERN_ERR, SCpnt,
-				    "Bad block number requested\n");
+				    "Bad block number requested: block number = %llu, sector count = %u, sector size = %u bytes\n",
+				    block + 0ULL, this_count, sdp->sector_size);
 			goto out;
 		} else {
-			block = block >> 1;
-			this_count = this_count >> 1;
-		}
-	}
-	if (sdp->sector_size == 2048) {
-		if ((block & 3) || (blk_rq_sectors(rq) & 3)) {
-			scmd_printk(KERN_ERR, SCpnt,
-				    "Bad block number requested\n");
-			goto out;
-		} else {
-			block = block >> 2;
-			this_count = this_count >> 2;
-		}
-	}
-	if (sdp->sector_size == 4096) {
-		if ((block & 7) || (blk_rq_sectors(rq) & 7)) {
-			scmd_printk(KERN_ERR, SCpnt,
-				    "Bad block number requested\n");
-			goto out;
-		} else {
-			block = block >> 3;
-			this_count = this_count >> 3;
+			int shift = ilog2(sdp->sector_size / 512);
+
+			block = block >> shift;
+			this_count = this_count >> shift;
 		}
 	}
 	if (rq_data_dir(rq) == WRITE) {
@@ -1173,56 +1157,26 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
 		SCpnt->cmnd[7] = 0x18;
 		SCpnt->cmnd[9] = (rq_data_dir(rq) == READ) ? READ_32 : WRITE_32;
 		SCpnt->cmnd[10] = protect | ((rq->cmd_flags & REQ_FUA) ? 0x8 : 0);
-
 		/* LBA */
-		SCpnt->cmnd[12] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
-		SCpnt->cmnd[13] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
-		SCpnt->cmnd[14] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
-		SCpnt->cmnd[15] = sizeof(block) > 4 ? (unsigned char) (block >> 32) & 0xff : 0;
-		SCpnt->cmnd[16] = (unsigned char) (block >> 24) & 0xff;
-		SCpnt->cmnd[17] = (unsigned char) (block >> 16) & 0xff;
-		SCpnt->cmnd[18] = (unsigned char) (block >> 8) & 0xff;
-		SCpnt->cmnd[19] = (unsigned char) block & 0xff;
-
+		put_unaligned_be64(block, &SCpnt->cmnd[12]);
 		/* Expected Indirect LBA */
-		SCpnt->cmnd[20] = (unsigned char) (block >> 24) & 0xff;
-		SCpnt->cmnd[21] = (unsigned char) (block >> 16) & 0xff;
-		SCpnt->cmnd[22] = (unsigned char) (block >> 8) & 0xff;
-		SCpnt->cmnd[23] = (unsigned char) block & 0xff;
-
+		put_unaligned_be32(block, &SCpnt->cmnd[20]);
 		/* Transfer length */
-		SCpnt->cmnd[28] = (unsigned char) (this_count >> 24) & 0xff;
-		SCpnt->cmnd[29] = (unsigned char) (this_count >> 16) & 0xff;
-		SCpnt->cmnd[30] = (unsigned char) (this_count >> 8) & 0xff;
-		SCpnt->cmnd[31] = (unsigned char) this_count & 0xff;
+		put_unaligned_be32(this_count, &SCpnt->cmnd[28]);
 	} else if (sdp->use_16_for_rw || (this_count > 0xffff)) {
 		SCpnt->cmnd[0] += READ_16 - READ_6;
 		SCpnt->cmnd[1] = protect | ((rq->cmd_flags & REQ_FUA) ? 0x8 : 0);
-		SCpnt->cmnd[2] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
-		SCpnt->cmnd[3] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
-		SCpnt->cmnd[4] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
-		SCpnt->cmnd[5] = sizeof(block) > 4 ? (unsigned char) (block >> 32) & 0xff : 0;
-		SCpnt->cmnd[6] = (unsigned char) (block >> 24) & 0xff;
-		SCpnt->cmnd[7] = (unsigned char) (block >> 16) & 0xff;
-		SCpnt->cmnd[8] = (unsigned char) (block >> 8) & 0xff;
-		SCpnt->cmnd[9] = (unsigned char) block & 0xff;
-		SCpnt->cmnd[10] = (unsigned char) (this_count >> 24) & 0xff;
-		SCpnt->cmnd[11] = (unsigned char) (this_count >> 16) & 0xff;
-		SCpnt->cmnd[12] = (unsigned char) (this_count >> 8) & 0xff;
-		SCpnt->cmnd[13] = (unsigned char) this_count & 0xff;
+		put_unaligned_be64(block, &SCpnt->cmnd[2]);
+		put_unaligned_be32(this_count, &SCpnt->cmnd[10]);
 		SCpnt->cmnd[14] = SCpnt->cmnd[15] = 0;
 	} else if ((this_count > 0xff) || (block > 0x1fffff) ||
 		   scsi_device_protection(SCpnt->device) ||
 		   SCpnt->device->use_10_for_rw) {
 		SCpnt->cmnd[0] += READ_10 - READ_6;
 		SCpnt->cmnd[1] = protect | ((rq->cmd_flags & REQ_FUA) ? 0x8 : 0);
-		SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
-		SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
-		SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
-		SCpnt->cmnd[5] = (unsigned char) block & 0xff;
+		put_unaligned_be32(block, &SCpnt->cmnd[2]);
 		SCpnt->cmnd[6] = SCpnt->cmnd[9] = 0;
-		SCpnt->cmnd[7] = (unsigned char) (this_count >> 8) & 0xff;
-		SCpnt->cmnd[8] = (unsigned char) this_count & 0xff;
+		put_unaligned_be16(this_count, &SCpnt->cmnd[7]);
 	} else {
 		if (unlikely(rq->cmd_flags & REQ_FUA)) {
 			/*
-- 
2.14.2

Re: [PATCH] sd: Micro-optimize READ / WRITE CDB encoding

[Douglas Gilbert]

On 2017-10-17 05:03 PM, Bart Van Assche wrote:
> Only compute 'threshold' if .last_sector_bug has been set. Reduce
> the number of branches that has to be taken to check the starting
> LBA and transfer length alignment. Optimize the encoding of the
> READ(10), WRITE(10), READ(16), WRITE(16), READ(32) and WRITE(32)
> CDB encoding.

Great!

Some suggestions, see inline below.

And showing us the improved version of sd_setup_read_write_cmnd()
as well as the diff could help us (or at least me) see the bigger
picture.

Doug Gilbert

> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>   drivers/scsi/sd.c | 102 +++++++++++++++---------------------------------------
>   1 file changed, 28 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 67b50baa943c..83284a8fa2cd 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1025,7 +1025,6 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>   	struct gendisk *disk = rq->rq_disk;
>   	struct scsi_disk *sdkp = scsi_disk(disk);
>   	sector_t block = blk_rq_pos(rq);

s/block/lba/		# use the well understood SCSI abbreviation

> -	sector_t threshold;
>   	unsigned int this_count = blk_rq_sectors(rq);
>   	unsigned int dif, dix;
>   	bool zoned_write = sd_is_zoned(sdkp) && rq_data_dir(rq) == WRITE;
> @@ -1071,20 +1070,22 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>   		goto out;
>   	}
>   
> -	/*
> -	 * Some SD card readers can't handle multi-sector accesses which touch
> -	 * the last one or two hardware sectors.  Split accesses as needed.
> -	 */
> -	threshold = get_capacity(disk) - SD_LAST_BUGGY_SECTORS *
> -		(sdp->sector_size / 512);
> +	if (unlikely(sdp->last_sector_bug)) {
> +		sector_t threshold;

s/threshold/threshold_lba/	# a bit long but more precise

>   
> -	if (unlikely(sdp->last_sector_bug && block + this_count > threshold)) {
> -		if (block < threshold) {
> -			/* Access up to the threshold but not beyond */
> -			this_count = threshold - block;
> -		} else {
> +		/*
> +		 * Some SD card readers can't handle multi-sector accesses
> +		 * which touch the last one or two hardware sectors.  Split
> +		 * accesses as needed.
> +		 */
> +		threshold = get_capacity(disk) - SD_LAST_BUGGY_SECTORS *
> +			(sdp->sector_size / 512);
> +		if (block >= threshold) {
>   			/* Access only a single hardware sector */
>   			this_count = sdp->sector_size / 512;
> +		} else if (block + this_count > threshold) {
> +			/* Access up to the threshold but not beyond */
> +			this_count = threshold - block;
>   		}
>   	}
>   
> @@ -1102,34 +1103,17 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>   	 * and not force the scsi disk driver to use bounce buffers
>   	 * for this.
>   	 */
> -	if (sdp->sector_size == 1024) {
> -		if ((block & 1) || (blk_rq_sectors(rq) & 1)) {
> +	if (sdp->sector_size != 512) {
> +		if ((block | this_count) & (sdp->sector_size / 512 - 1)) {
>   			scmd_printk(KERN_ERR, SCpnt,
> -				    "Bad block number requested\n");
> +				    "Bad block number requested: block number = %llu, sector count = %u, sector size = %u bytes\n",
> +				    block + 0ULL, this_count, sdp->sector_size);
>   			goto out;
>   		} else {
> -			block = block >> 1;
> -			this_count = this_count >> 1;
> -		}
> -	}
> -	if (sdp->sector_size == 2048) {
> -		if ((block & 3) || (blk_rq_sectors(rq) & 3)) {
> -			scmd_printk(KERN_ERR, SCpnt,
> -				    "Bad block number requested\n");
> -			goto out;
> -		} else {
> -			block = block >> 2;
> -			this_count = this_count >> 2;
> -		}
> -	}
> -	if (sdp->sector_size == 4096) {
> -		if ((block & 7) || (blk_rq_sectors(rq) & 7)) {
> -			scmd_printk(KERN_ERR, SCpnt,
> -				    "Bad block number requested\n");
> -			goto out;
> -		} else {
> -			block = block >> 3;
> -			this_count = this_count >> 3;
> +			int shift = ilog2(sdp->sector_size / 512);
> +
> +			block = block >> shift;

			lba >>= shift;	/* scale down LBA */

> +			this_count = this_count >> shift;

likewise

>   		}
>   	}
>   	if (rq_data_dir(rq) == WRITE) {
> @@ -1173,56 +1157,26 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>   		SCpnt->cmnd[7] = 0x18;
>   		SCpnt->cmnd[9] = (rq_data_dir(rq) == READ) ? READ_32 : WRITE_32;

Perhaps rq_data_dir(rq) could be placed in a local variable

>   		SCpnt->cmnd[10] = protect | ((rq->cmd_flags & REQ_FUA) ? 0x8 : 0);

		SCpnt->cmnd[10] = protect;
		if (unlikely(rq->cmd_flags & REQ_FUA))
			SCpnt->cmnd[10] |= 0x8;

> -
>   		/* LBA */
> -		SCpnt->cmnd[12] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
> -		SCpnt->cmnd[13] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
> -		SCpnt->cmnd[14] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
> -		SCpnt->cmnd[15] = sizeof(block) > 4 ? (unsigned char) (block >> 32) & 0xff : 0;
> -		SCpnt->cmnd[16] = (unsigned char) (block >> 24) & 0xff;
> -		SCpnt->cmnd[17] = (unsigned char) (block >> 16) & 0xff;
> -		SCpnt->cmnd[18] = (unsigned char) (block >> 8) & 0xff;
> -		SCpnt->cmnd[19] = (unsigned char) block & 0xff;
> -
> +		put_unaligned_be64(block, &SCpnt->cmnd[12]);
>   		/* Expected Indirect LBA */
> -		SCpnt->cmnd[20] = (unsigned char) (block >> 24) & 0xff;
> -		SCpnt->cmnd[21] = (unsigned char) (block >> 16) & 0xff;
> -		SCpnt->cmnd[22] = (unsigned char) (block >> 8) & 0xff;
> -		SCpnt->cmnd[23] = (unsigned char) block & 0xff;
> -
> +		put_unaligned_be32(block, &SCpnt->cmnd[20]);
>   		/* Transfer length */
> -		SCpnt->cmnd[28] = (unsigned char) (this_count >> 24) & 0xff;
> -		SCpnt->cmnd[29] = (unsigned char) (this_count >> 16) & 0xff;
> -		SCpnt->cmnd[30] = (unsigned char) (this_count >> 8) & 0xff;
> -		SCpnt->cmnd[31] = (unsigned char) this_count & 0xff;
> +		put_unaligned_be32(this_count, &SCpnt->cmnd[28]);
>   	} else if (sdp->use_16_for_rw || (this_count > 0xffff)) {
>   		SCpnt->cmnd[0] += READ_16 - READ_6;
>   		SCpnt->cmnd[1] = protect | ((rq->cmd_flags & REQ_FUA) ? 0x8 : 0);
> -		SCpnt->cmnd[2] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
> -		SCpnt->cmnd[3] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
> -		SCpnt->cmnd[4] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
> -		SCpnt->cmnd[5] = sizeof(block) > 4 ? (unsigned char) (block >> 32) & 0xff : 0;
> -		SCpnt->cmnd[6] = (unsigned char) (block >> 24) & 0xff;
> -		SCpnt->cmnd[7] = (unsigned char) (block >> 16) & 0xff;
> -		SCpnt->cmnd[8] = (unsigned char) (block >> 8) & 0xff;
> -		SCpnt->cmnd[9] = (unsigned char) block & 0xff;
> -		SCpnt->cmnd[10] = (unsigned char) (this_count >> 24) & 0xff;
> -		SCpnt->cmnd[11] = (unsigned char) (this_count >> 16) & 0xff;
> -		SCpnt->cmnd[12] = (unsigned char) (this_count >> 8) & 0xff;
> -		SCpnt->cmnd[13] = (unsigned char) this_count & 0xff;
> +		put_unaligned_be64(block, &SCpnt->cmnd[2]);
> +		put_unaligned_be32(this_count, &SCpnt->cmnd[10]);
>   		SCpnt->cmnd[14] = SCpnt->cmnd[15] = 0;
>   	} else if ((this_count > 0xff) || (block > 0x1fffff) ||
>   		   scsi_device_protection(SCpnt->device) ||
>   		   SCpnt->device->use_10_for_rw) {
>   		SCpnt->cmnd[0] += READ_10 - READ_6;
>   		SCpnt->cmnd[1] = protect | ((rq->cmd_flags & REQ_FUA) ? 0x8 : 0);
> -		SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
> -		SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
> -		SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
> -		SCpnt->cmnd[5] = (unsigned char) block & 0xff;
> +		put_unaligned_be32(block, &SCpnt->cmnd[2]);
>   		SCpnt->cmnd[6] = SCpnt->cmnd[9] = 0;
> -		SCpnt->cmnd[7] = (unsigned char) (this_count >> 8) & 0xff;
> -		SCpnt->cmnd[8] = (unsigned char) this_count & 0xff;
> +		put_unaligned_be16(this_count, &SCpnt->cmnd[7]);
>   	} else {
>   		if (unlikely(rq->cmd_flags & REQ_FUA)) {
>   			/*
> 

Re: [PATCH] sd: Micro-optimize READ / WRITE CDB encoding

[Bart Van Assche]

On Tue, 2017-10-17 at 18:17 -0400, Douglas Gilbert wrote:
> On 2017-10-17 05:03 PM, Bart Van Assche wrote:
> > @@ -1025,7 +1025,6 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
> >   	struct gendisk *disk = rq->rq_disk;
> >   	struct scsi_disk *sdkp = scsi_disk(disk);
> >   	sector_t block = blk_rq_pos(rq);
> 
> s/block/lba/		# use the well understood SCSI abbreviation

Since blk_rq_pos() returns the offset in units of 512 bytes, how about renaming
'block' into 'sector' and using the name 'lba' for the number obtained after the
shift operation?

> 
> > -	sector_t threshold;
> >   	unsigned int this_count = blk_rq_sectors(rq);
> >   	unsigned int dif, dix;
> >   	bool zoned_write = sd_is_zoned(sdkp) && rq_data_dir(rq) == WRITE;
> > @@ -1071,20 +1070,22 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
> >   		goto out;
> >   	}
> >   
> > -	/*
> > -	 * Some SD card readers can't handle multi-sector accesses which touch
> > -	 * the last one or two hardware sectors.  Split accesses as needed.
> > -	 */
> > -	threshold = get_capacity(disk) - SD_LAST_BUGGY_SECTORS *
> > -		(sdp->sector_size / 512);
> > +	if (unlikely(sdp->last_sector_bug)) {
> > +		sector_t threshold;
> 
> s/threshold/threshold_lba/	# a bit long but more precise

A similar comment applies here - shouldn't this be called 'threshold_sector'?

> >   		}
> >   	}
> >   	if (rq_data_dir(rq) == WRITE) {
> > @@ -1173,56 +1157,26 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
> >   		SCpnt->cmnd[7] = 0x18;
> >   		SCpnt->cmnd[9] = (rq_data_dir(rq) == READ) ? READ_32 : WRITE_32;
> 
> Perhaps rq_data_dir(rq) could be placed in a local variable

I will keep that for a separate patch.

Bart.

Re: [PATCH] sd: Micro-optimize READ / WRITE CDB encoding

[Martin K. Petersen]

Bart,

> Only compute 'threshold' if .last_sector_bug has been set. Reduce
> the number of branches that has to be taken to check the starting
> LBA and transfer length alignment. Optimize the encoding of the
> READ(10), WRITE(10), READ(16), WRITE(16), READ(32) and WRITE(32)
> CDB encoding.

Please take a look at this. It missed 4.15 because I've been fighting a
hardware bug the last several weeks. However, I'd still like to get it
in shape for 4.16:

https://git.kernel.org/pub/scm/linux/kernel/git/mkp/linux.git/log/?h=scsi-work

Feel free to beat me to it.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] sd: Micro-optimize READ / WRITE CDB encoding

[Christoph Hellwig]

On Tue, Oct 17, 2017 at 11:10:42PM -0400, Martin K. Petersen wrote:
> Please take a look at this. It missed 4.15 because I've been fighting a
> hardware bug the last several weeks. However, I'd still like to get it
> in shape for 4.16:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mkp/linux.git/log/?h=scsi-work
> 
> Feel free to beat me to it.

It would be great to get it into 4.15, we stil have some time for that.

Re: [PATCH] sd: Micro-optimize READ / WRITE CDB encoding

[Douglas Gilbert]

[-- Attachment #1: Type: text/plain, Size: 11081 bytes --]

On 2017-10-17 06:41 PM, Bart Van Assche wrote:
> On Tue, 2017-10-17 at 18:17 -0400, Douglas Gilbert wrote:
>> On 2017-10-17 05:03 PM, Bart Van Assche wrote:
>>> @@ -1025,7 +1025,6 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>>>    	struct gendisk *disk = rq->rq_disk;
>>>    	struct scsi_disk *sdkp = scsi_disk(disk);
>>>    	sector_t block = blk_rq_pos(rq);
>>
>> s/block/lba/		# use the well understood SCSI abbreviation
> 
> Since blk_rq_pos() returns the offset in units of 512 bytes, how about renaming
> 'block' into 'sector' and using the name 'lba' for the number obtained after the
> shift operation?
> 
>>
>>> -	sector_t threshold;
>>>    	unsigned int this_count = blk_rq_sectors(rq);
>>>    	unsigned int dif, dix;
>>>    	bool zoned_write = sd_is_zoned(sdkp) && rq_data_dir(rq) == WRITE;
>>> @@ -1071,20 +1070,22 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>>>    		goto out;
>>>    	}
>>>    
>>> -	/*
>>> -	 * Some SD card readers can't handle multi-sector accesses which touch
>>> -	 * the last one or two hardware sectors.  Split accesses as needed.
>>> -	 */
>>> -	threshold = get_capacity(disk) - SD_LAST_BUGGY_SECTORS *
>>> -		(sdp->sector_size / 512);
>>> +	if (unlikely(sdp->last_sector_bug)) {
>>> +		sector_t threshold;
>>
>> s/threshold/threshold_lba/	# a bit long but more precise
> 
> A similar comment applies here - shouldn't this be called 'threshold_sector'?
> 
>>>    		}
>>>    	}
>>>    	if (rq_data_dir(rq) == WRITE) {
>>> @@ -1173,56 +1157,26 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>>>    		SCpnt->cmnd[7] = 0x18;
>>>    		SCpnt->cmnd[9] = (rq_data_dir(rq) == READ) ? READ_32 : WRITE_32;
>>
>> Perhaps rq_data_dir(rq) could be placed in a local variable
> 
> I will keep that for a separate patch.

Bart,
Below is a rewrite of that function taking on board your ideas
and adding some of mine. It is inline in this post (but lines
are wrapped) and it is attached. It compiles.

The variable names are a little more descriptive (except
"last_lba" should be "first_lba_beyond_access") and more
state is cached in local variables (e.g. is_write).

Doug Gilbert


static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
{
         struct request *rq = SCpnt->request;
         struct scsi_device *sdp = SCpnt->device;
         struct gendisk *disk = rq->rq_disk;
         struct scsi_disk *sdkp = scsi_disk(disk);
         unsigned int num_blks = blk_rq_sectors(rq);
         unsigned int dif, dix;
         unsigned int sect_sz;
         sector_t lba = blk_rq_pos(rq);
         sector_t threshold;
         sector_t lu_capacity = get_capacity(disk);
         sector_t last_lba = lba + num_blks;
         bool is_write = (rq_data_dir(rq) == WRITE);
         bool zoned_write = sd_is_zoned(sdkp) && is_write;
         int ret;
         unsigned char protect;

         if (zoned_write) {
                 ret = sd_zbc_write_lock_zone(SCpnt);
                 if (ret != BLKPREP_OK)
                         return ret;
         }

         ret = scsi_init_io(SCpnt);
         if (ret != BLKPREP_OK)
                 goto out;
         SCpnt = rq->special;

         /* from here on until we're complete, any goto out
          * is used for a killable error condition */
         ret = BLKPREP_KILL;

         SCSI_LOG_HLQUEUE(1,
                 scmd_printk(KERN_INFO, SCpnt,
                         "%s: lba=%llu, count=%d\n",
                         __func__, (unsigned long long)lba, num_blks));

         if (likely(sdp && scsi_device_online(sdp) &&
                    (last_lba <= lu_capacity)))
                 ; /* ok: have device, its online and access fits on medium */
         else {
                 SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
                                                 "Finishing %u sectors\n",
                                                 num_blks));
                 SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
                                                 "Retry with 0x%p\n", SCpnt));
                 goto out;
         }
         sect_sz = sdp->sector_size;

         if (unlikely(sdp->changed)) {
                 /*
                  * quietly refuse to do anything to a changed disc until
                  * the changed bit has been reset
                  */
                 /* printk("SCSI disk has been changed or is not present. 
Prohibiting further I/O.\n"); */
                 goto out;
         }

         /*
          * Some SD card readers can't handle multi-sector accesses which touch
          * the last one or two hardware sectors.  Split accesses as needed.
          */
         if (unlikely(sdp->last_sector_bug)) {
                 threshold = lu_capacity -
                             (SD_LAST_BUGGY_SECTORS * (sect_sz / 512));

                 if (unlikely(last_lba > threshold))
                         num_blks = (lba < threshold) ? (threshold - lba) :
                                                 (sect_sz / 512);
                         /* If LBA less than threshold the access up to the
                          * threshold but not beyond; otherwise access only
                          * a single hardware sector. */
         }

         SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt, "lba=%llu\n",
                                         (unsigned long long)num_blks));

         /*
          * If we have a 1K hardware sectorsize, prevent access to single
          * 512 byte sectors.  In theory we could handle this - in fact
          * the scsi cdrom driver must be able to handle this because
          * we typically use 1K blocksizes, and cdroms typically have
          * 2K hardware sectorsizes.  Of course, things are simpler
          * with the cdrom, since it is read-only.  For performance
          * reasons, the filesystems should be able to handle this
          * and not force the scsi disk driver to use bounce buffers
          * for this.
          */
         if (sect_sz != 512) {
                 if ((lba | num_blks) & (sect_sz / 512 - 1)) {
                         scmd_printk(KERN_ERR, SCpnt,
                                     "Bad lba requested: lba = %llu, sector 
count = %u, sector size = %u bytes\n",
                                     lba + 0ULL, num_blks, sect_sz);
                         goto out;
                 } else {
                         int shift = ilog2(sect_sz / 512);

                         lba >>= shift;
                         num_blks >>= shift;
                 }
         }

         if (is_write) {
                 if (blk_integrity_rq(rq))
                         sd_dif_prepare(SCpnt);

         } else if (rq_data_dir(rq) != READ) {
                 scmd_printk(KERN_ERR, SCpnt, "Unknown command %d\n", req_op(rq));
                 goto out;
         }

         SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
                                         "%s %d/%u 512 byte blocks.\n",
                                         is_write ?  "writing" : "reading",
                                         num_blks, blk_rq_sectors(rq)));

         dix = scsi_prot_sg_count(SCpnt);
         dif = scsi_host_dif_capable(SCpnt->device->host, sdkp->protection_type);

         if (dif || dix)
                 protect = sd_setup_protect_cmnd(SCpnt, dix, dif);
         else
                 protect = 0;

         if (protect && sdkp->protection_type == T10_PI_TYPE2_PROTECTION) {
                 SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);

                 if (unlikely(SCpnt->cmnd == NULL)) {
                         ret = BLKPREP_DEFER;
                         goto out;
                 }

                 SCpnt->cmd_len = SD_EXT_CDB_SIZE;
                 memset(SCpnt->cmnd, 0, SCpnt->cmd_len);
                 SCpnt->cmnd[0] = VARIABLE_LENGTH_CMD;
                 SCpnt->cmnd[7] = 0x18;
                 SCpnt->cmnd[9] = is_write ? WRITE_32 : READ_32;
                 SCpnt->cmnd[10] = protect;
                 if (unlikely(rq->cmd_flags & REQ_FUA))
                         SCpnt->cmnd[10] |= 0x8;
                 put_unaligned_be64(lba, SCpnt->cmnd + 12);

                 /* Expected initial LB reference tag: lower 4 bytes of LBA */
                 put_unaligned_be32(lba, SCpnt->cmnd + 20);
                 /* Leave Expected LB application tag and LB application tag 
mask zeroed */

                 /* Transfer length */
                 put_unaligned_be32(num_blks, SCpnt->cmnd + 28);
         } else if (sdp->use_16_for_rw || unlikely(num_blks > 0xffff)) {
                 SCpnt->cmnd[0] = is_write ? WRITE_16 : READ_16;
                 SCpnt->cmnd[1] = protect;
                 if (unlikely(rq->cmd_flags & REQ_FUA))
                         SCpnt->cmnd[1] |= 0x8;
                 put_unaligned_be64(lba, SCpnt->cmnd + 2);
                 put_unaligned_be32(num_blks, SCpnt->cmnd + 10);
                 SCpnt->cmnd[14] = 0;
                 SCpnt->cmnd[15] = 0;
         } else if (SCpnt->device->use_10_for_rw || (num_blks > 0xff) ||
                    (lba > 0x1fffff) || scsi_device_protection(SCpnt->device)) {
                 SCpnt->cmnd[0] = is_write ? WRITE_10 : READ_10;
                 SCpnt->cmnd[1] = protect;
                 if (unlikely(rq->cmd_flags & REQ_FUA))
                         SCpnt->cmnd[1] |= 0x8;
                 put_unaligned_be32(lba, SCpnt->cmnd + 2);
                 SCpnt->cmnd[6] = 0;
                 put_unaligned_be16(num_blks, SCpnt->cmnd + 7);
                 SCpnt->cmnd[9] = 0;
         } else {
                 if (unlikely(rq->cmd_flags & REQ_FUA)) {
                         /*
                          * This happens only if this drive failed
                          * 10byte rw command with ILLEGAL_REQUEST
                          * during operation and thus turned off
                          * use_10_for_rw.
                          */
                         scmd_printk(KERN_ERR, SCpnt,
                                     "FUA write on READ/WRITE(6) drive\n");
                         goto out;
                 }
                 /* rely on lba <= 0x1fffff */
                 put_unaligned_be32(lba, SCpnt->cmnd + 0);
                 SCpnt->cmnd[0] = is_write ? WRITE_6 : READ_6;
                 SCpnt->cmnd[4] = (unsigned char) num_blks;
                 SCpnt->cmnd[5] = 0;
         }
         SCpnt->sdb.length = num_blks * sect_sz;

         /*
          * We shouldn't disconnect in the middle of a sector, so with a dumb
          * host adapter, it's safe to assume that we can at least transfer
          * this many bytes between each connect / disconnect.
          */
         SCpnt->transfersize = sect_sz;
         SCpnt->underflow = num_blks << 9;
         SCpnt->allowed = SD_MAX_RETRIES;

         /*
          * This indicates that the command is ready from our end to be
          * queued.
          */
         ret = BLKPREP_OK;
  out:
         if (zoned_write && ret != BLKPREP_OK)
                 sd_zbc_write_unlock_zone(SCpnt);

         return ret;
}

[-- Attachment #2: sd_setup_rw_cmd.c --]
[-- Type: text/x-csrc, Size: 6373 bytes --]


static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
{
	struct request *rq = SCpnt->request;
	struct scsi_device *sdp = SCpnt->device;
	struct gendisk *disk = rq->rq_disk;
	struct scsi_disk *sdkp = scsi_disk(disk);
	unsigned int num_blks = blk_rq_sectors(rq);
	unsigned int dif, dix;
	unsigned int sect_sz;
	sector_t lba = blk_rq_pos(rq);
	sector_t threshold;
	sector_t lu_capacity = get_capacity(disk);
	sector_t last_lba = lba + num_blks;
	bool is_write = (rq_data_dir(rq) == WRITE);
	bool zoned_write = sd_is_zoned(sdkp) && is_write;
	int ret;
	unsigned char protect;

	if (zoned_write) {
		ret = sd_zbc_write_lock_zone(SCpnt);
		if (ret != BLKPREP_OK)
			return ret;
	}

	ret = scsi_init_io(SCpnt);
	if (ret != BLKPREP_OK)
		goto out;
	SCpnt = rq->special;

	/* from here on until we're complete, any goto out
	 * is used for a killable error condition */
	ret = BLKPREP_KILL;

	SCSI_LOG_HLQUEUE(1,
		scmd_printk(KERN_INFO, SCpnt,
			"%s: lba=%llu, count=%d\n",
			__func__, (unsigned long long)lba, num_blks));

	if (likely(sdp && scsi_device_online(sdp) &&
		   (last_lba <= lu_capacity)))
       		; /* ok: have device, its online and access fits on medium */
	else {
		SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
						"Finishing %u sectors\n",
						num_blks));
		SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
						"Retry with 0x%p\n", SCpnt));
		goto out;
	}
	sect_sz = sdp->sector_size;

	if (unlikely(sdp->changed)) {
		/*
		 * quietly refuse to do anything to a changed disc until 
		 * the changed bit has been reset
		 */
		/* printk("SCSI disk has been changed or is not present. Prohibiting further I/O.\n"); */
		goto out;
	}

	/*
	 * Some SD card readers can't handle multi-sector accesses which touch
	 * the last one or two hardware sectors.  Split accesses as needed.
	 */
	if (unlikely(sdp->last_sector_bug)) {
		threshold = lu_capacity -
			    (SD_LAST_BUGGY_SECTORS * (sect_sz / 512));

		if (unlikely(last_lba > threshold))
			num_blks = (lba < threshold) ? (threshold - lba) :
						(sect_sz / 512);
			/* If LBA less than threshold the access up to the
			 * threshold but not beyond; otherwise access only
			 * a single hardware sector. */
	}

	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt, "lba=%llu\n",
					(unsigned long long)num_blks));

	/*
	 * If we have a 1K hardware sectorsize, prevent access to single
	 * 512 byte sectors.  In theory we could handle this - in fact
	 * the scsi cdrom driver must be able to handle this because
	 * we typically use 1K blocksizes, and cdroms typically have
	 * 2K hardware sectorsizes.  Of course, things are simpler
	 * with the cdrom, since it is read-only.  For performance
	 * reasons, the filesystems should be able to handle this
	 * and not force the scsi disk driver to use bounce buffers
	 * for this.
	 */
	if (sect_sz != 512) {
		if ((lba | num_blks) & (sect_sz / 512 - 1)) {
			scmd_printk(KERN_ERR, SCpnt,
				    "Bad lba requested: lba = %llu, sector count = %u, sector size = %u bytes\n",
				    lba + 0ULL, num_blks, sect_sz);
			goto out;
		} else {
			int shift = ilog2(sect_sz / 512);

			lba >>= shift;
			num_blks >>= shift;
		}
	}

	if (is_write) {
		if (blk_integrity_rq(rq))
			sd_dif_prepare(SCpnt);

	} else if (rq_data_dir(rq) != READ) {
		scmd_printk(KERN_ERR, SCpnt, "Unknown command %d\n", req_op(rq));
		goto out;
	}

	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
					"%s %d/%u 512 byte blocks.\n",
					is_write ?  "writing" : "reading",
				       	num_blks, blk_rq_sectors(rq)));

	dix = scsi_prot_sg_count(SCpnt);
	dif = scsi_host_dif_capable(SCpnt->device->host, sdkp->protection_type);

	if (dif || dix)
		protect = sd_setup_protect_cmnd(SCpnt, dix, dif);
	else
		protect = 0;

	if (protect && sdkp->protection_type == T10_PI_TYPE2_PROTECTION) {
		SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);

		if (unlikely(SCpnt->cmnd == NULL)) {
			ret = BLKPREP_DEFER;
			goto out;
		}

		SCpnt->cmd_len = SD_EXT_CDB_SIZE;
		memset(SCpnt->cmnd, 0, SCpnt->cmd_len);
		SCpnt->cmnd[0] = VARIABLE_LENGTH_CMD;
		SCpnt->cmnd[7] = 0x18;
		SCpnt->cmnd[9] = is_write ? WRITE_32 : READ_32;
		SCpnt->cmnd[10] = protect;
		if (unlikely(rq->cmd_flags & REQ_FUA))
			SCpnt->cmnd[10] |= 0x8;
		put_unaligned_be64(lba, SCpnt->cmnd + 12);

		/* Expected initial LB reference tag: lower 4 bytes of LBA */
		put_unaligned_be32(lba, SCpnt->cmnd + 20);
		/* Leave Expected LB application tag and LB application tag mask zeroed */

		/* Transfer length */
		put_unaligned_be32(num_blks, SCpnt->cmnd + 28);
	} else if (sdp->use_16_for_rw || unlikely(num_blks > 0xffff)) {
		SCpnt->cmnd[0] = is_write ? WRITE_16 : READ_16;
		SCpnt->cmnd[1] = protect;
	        if (unlikely(rq->cmd_flags & REQ_FUA))
			SCpnt->cmnd[1] |= 0x8;
		put_unaligned_be64(lba, SCpnt->cmnd + 2);
		put_unaligned_be32(num_blks, SCpnt->cmnd + 10);
		SCpnt->cmnd[14] = 0;
	       	SCpnt->cmnd[15] = 0;
	} else if (SCpnt->device->use_10_for_rw || (num_blks > 0xff) ||
	       	   (lba > 0x1fffff) || scsi_device_protection(SCpnt->device)) {
		SCpnt->cmnd[0] = is_write ? WRITE_10 : READ_10;
		SCpnt->cmnd[1] = protect;
	        if (unlikely(rq->cmd_flags & REQ_FUA))
			SCpnt->cmnd[1] |= 0x8;
		put_unaligned_be32(lba, SCpnt->cmnd + 2);
		SCpnt->cmnd[6] = 0;
		put_unaligned_be16(num_blks, SCpnt->cmnd + 7);
		SCpnt->cmnd[9] = 0;
	} else {
		if (unlikely(rq->cmd_flags & REQ_FUA)) {
			/*
			 * This happens only if this drive failed
			 * 10byte rw command with ILLEGAL_REQUEST
			 * during operation and thus turned off
			 * use_10_for_rw.
			 */
			scmd_printk(KERN_ERR, SCpnt,
				    "FUA write on READ/WRITE(6) drive\n");
			goto out;
		}
		/* rely on lba <= 0x1fffff */
		put_unaligned_be32(lba, SCpnt->cmnd + 0);
		SCpnt->cmnd[0] = is_write ? WRITE_6 : READ_6;
		SCpnt->cmnd[4] = (unsigned char) num_blks;
		SCpnt->cmnd[5] = 0;
	}
	SCpnt->sdb.length = num_blks * sect_sz;

	/*
	 * We shouldn't disconnect in the middle of a sector, so with a dumb
	 * host adapter, it's safe to assume that we can at least transfer
	 * this many bytes between each connect / disconnect.
	 */
	SCpnt->transfersize = sect_sz;
	SCpnt->underflow = num_blks << 9;
	SCpnt->allowed = SD_MAX_RETRIES;

	/*
	 * This indicates that the command is ready from our end to be
	 * queued.
	 */
	ret = BLKPREP_OK;
 out:
	if (zoned_write && ret != BLKPREP_OK)
		sd_zbc_write_unlock_zone(SCpnt);

	return ret;
}

Re: [PATCH] sd: Micro-optimize READ / WRITE CDB encoding

[Douglas Gilbert]

[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]

On 2017-10-17 06:41 PM, Bart Van Assche wrote:
> On Tue, 2017-10-17 at 18:17 -0400, Douglas Gilbert wrote:
>> On 2017-10-17 05:03 PM, Bart Van Assche wrote:
>>> @@ -1025,7 +1025,6 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>>>    	struct gendisk *disk = rq->rq_disk;
>>>    	struct scsi_disk *sdkp = scsi_disk(disk);
>>>    	sector_t block = blk_rq_pos(rq);
>>
>> s/block/lba/		# use the well understood SCSI abbreviation
> 
> Since blk_rq_pos() returns the offset in units of 512 bytes, how about renaming
> 'block' into 'sector' and using the name 'lba' for the number obtained after the
> shift operation?
> 
>>
>>> -	sector_t threshold;
>>>    	unsigned int this_count = blk_rq_sectors(rq);
>>>    	unsigned int dif, dix;
>>>    	bool zoned_write = sd_is_zoned(sdkp) && rq_data_dir(rq) == WRITE;
>>> @@ -1071,20 +1070,22 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>>>    		goto out;
>>>    	}
>>>    
>>> -	/*
>>> -	 * Some SD card readers can't handle multi-sector accesses which touch
>>> -	 * the last one or two hardware sectors.  Split accesses as needed.
>>> -	 */
>>> -	threshold = get_capacity(disk) - SD_LAST_BUGGY_SECTORS *
>>> -		(sdp->sector_size / 512);
>>> +	if (unlikely(sdp->last_sector_bug)) {
>>> +		sector_t threshold;
>>
>> s/threshold/threshold_lba/	# a bit long but more precise
> 
> A similar comment applies here - shouldn't this be called 'threshold_sector'?
> 
>>>    		}
>>>    	}
>>>    	if (rq_data_dir(rq) == WRITE) {
>>> @@ -1173,56 +1157,26 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>>>    		SCpnt->cmnd[7] = 0x18;
>>>    		SCpnt->cmnd[9] = (rq_data_dir(rq) == READ) ? READ_32 : WRITE_32;
>>
>> Perhaps rq_data_dir(rq) could be placed in a local variable
> 
> I will keep that for a separate patch.

Hi,
This thread is going a bit cold so I have attached my rewrite of
sd_setup_read_write_cmnd(). It incorporates Bart's speed improvements
(e.g. using put_unaligned_be*() and improving the scaling algorithm)
plus the naming improvements discussed above. I plan to send it as
a freestanding post shortly.

One thing that caught my eye in the rewrite was this line near the end:
	SCpnt->underflow = num_blks << 9;

The underflow field is defined in scsi_cmnd.h as:
         unsigned underflow;     /* Return error if less than
                                    this amount is transferred */

IMO the calculation (i.e. multiplying by 512) is the correct number
of bytes only if sector_size is 512. To make it more generally
correct it should read:
	SCpnt->underflow = num_sects << 9;

Comments, anyone ...

Doug Gilbert

[-- Attachment #2: sd_setup_rw_v2.c --]
[-- Type: text/x-csrc, Size: 6623 bytes --]


static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
{
	struct request *rq = SCpnt->request;
	struct scsi_device *sdp = SCpnt->device;
	struct gendisk *disk = rq->rq_disk;
	struct scsi_disk *sdkp = scsi_disk(disk);
	unsigned int num_sects = blk_rq_sectors(rq);
	unsigned int num_blks;
	unsigned int dif, dix;
	unsigned int sect_sz;
	sector_t sect_addr = blk_rq_pos(rq);
	sector_t sect_after = sect_addr + num_sects;
	sector_t total_sects = get_capacity(disk);
	sector_t threshold_sect;
	sector_t lba;
	bool is_write = (rq_data_dir(rq) == WRITE);
	bool have_fua = !!(rq->cmd_flags & REQ_FUA);
	bool zoned_write = sd_is_zoned(sdkp) && is_write;
	int ret;
	unsigned char protect;

	if (zoned_write) {
		ret = sd_zbc_write_lock_zone(SCpnt);
		if (ret != BLKPREP_OK)
			return ret;
	}

	ret = scsi_init_io(SCpnt);
	if (ret != BLKPREP_OK)
		goto out;
	WARN_ON_ONCE(SCpnt != rq->special);

	/* from here on until we're complete, any goto out
	 * is used for a killable error condition */
	ret = BLKPREP_KILL;

	SCSI_LOG_HLQUEUE(1,
		scmd_printk(KERN_INFO, SCpnt,
			"%s: sector=%llu, num_sects=%d\n",
			__func__, (unsigned long long)sect_addr, num_sects));

	if (likely(sdp && scsi_device_online(sdp) &&
		   (sect_after <= total_sects)))
		; /* ok: have device, its online and access fits on medium */
	else {
		SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
						"Finishing %u sectors\n",
						num_sects));
		SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
						"Retry with 0x%p\n", SCpnt));
		goto out;
	}
	sect_sz = sdp->sector_size;

	if (unlikely(sdp->changed)) {
		/*
		 * quietly refuse to do anything to a changed disc until 
		 * the changed bit has been reset
		 */
		/* printk("SCSI disk has been changed or is not present. Prohibiting further I/O.\n"); */
		goto out;
	}

	/*
	 * Some SD card readers can't handle multi-sector accesses which touch
	 * the last one or two hardware sectors.  Split accesses as needed.
	 */
	if (unlikely(sdp->last_sector_bug)) {
		threshold_sect = total_sects -
			    (SD_LAST_BUGGY_SECTORS * (sect_sz / 512));

		if (unlikely(sect_after > threshold_sect))
			num_sects = (sect_addr < threshold_sect) ?
						(threshold_sect - sect_addr) :
						(sect_sz / 512);
			/* If LBA less than threshold then access up to the
			 * threshold but not beyond; otherwise access only
			 * a single hardware sector.
			 */
	}

	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt, "num_sects=%llu\n",
					(unsigned long long)num_sects));

	/*
	 * If we have a 1K hardware sectorsize, prevent access to single
	 * 512 byte sectors.  In theory we could handle this - in fact
	 * the scsi cdrom driver must be able to handle this because
	 * we typically use 1K blocksizes, and cdroms typically have
	 * 2K hardware sectorsizes.  Of course, things are simpler
	 * with the cdrom, since it is read-only.  For performance
	 * reasons, the filesystems should be able to handle this
	 * and not force the scsi disk driver to use bounce buffers
	 * for this. Assume sect_sz >= 512 bytes.
	 */
	if (sect_sz > 512) {
		if ((sect_addr | num_sects) & (sect_sz / 512 - 1)) {
			scmd_printk(KERN_ERR, SCpnt,
				    "Bad sector requested: address = %llu, num_sects = %u, sector size = %u bytes\n",
				    sect_addr + 0ULL, num_sects, sect_sz);
			goto out;
		} else {
			int shift = ilog2(sect_sz / 512);

			lba = sect_addr >> shift;
			num_blks = num_sects >> shift;
		}
	} else {	/* So sector size is 512 bytes */
		lba = sect_addr;
		num_blks = num_sects;
	}

	if (is_write) {
		if (blk_integrity_rq(rq))
			sd_dif_prepare(SCpnt);
	} else if (unlikely(rq_data_dir(rq) != READ)) {
		scmd_printk(KERN_ERR, SCpnt, "Unknown command %d\n", req_op(rq));
		goto out;
	}

	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
					"%s %d/%u 512 byte blocks.\n",
					is_write ?  "writing" : "reading",
					num_blks, num_sects));

	dix = scsi_prot_sg_count(SCpnt);
	dif = scsi_host_dif_capable(sdp->host, sdkp->protection_type);

	if (unlikely(dif || dix))
		protect = sd_setup_protect_cmnd(SCpnt, dix, dif);
	else
		protect = 0;

	if (unlikely(protect &&
		     sdkp->protection_type == T10_PI_TYPE2_PROTECTION)) {
		SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);

		if (unlikely(SCpnt->cmnd == NULL)) {
			ret = BLKPREP_DEFER;
			goto out;
		}

		SCpnt->cmd_len = SD_EXT_CDB_SIZE;
		memset(SCpnt->cmnd, 0, SCpnt->cmd_len);
		SCpnt->cmnd[0] = VARIABLE_LENGTH_CMD;
		SCpnt->cmnd[7] = 0x18;
		SCpnt->cmnd[9] = is_write ? WRITE_32 : READ_32;
		SCpnt->cmnd[10] = protect;
		if (unlikely(have_fua))
			SCpnt->cmnd[10] |= 0x8;
		put_unaligned_be64(lba, SCpnt->cmnd + 12);

		/* Expected initial LB reference tag: lower 4 bytes of LBA */
		put_unaligned_be32(lba, SCpnt->cmnd + 20);
		/* Leave Expected LB application tag and LB application tag
		 * mask zeroed
		 */

		put_unaligned_be32(num_blks, SCpnt->cmnd + 28);
	} else if (sdp->use_16_for_rw || unlikely(num_blks > 0xffff)) {
		SCpnt->cmnd[0] = is_write ? WRITE_16 : READ_16;
		SCpnt->cmnd[1] = protect;
		if (unlikely(have_fua))
			SCpnt->cmnd[1] |= 0x8;
		put_unaligned_be64(lba, SCpnt->cmnd + 2);
		put_unaligned_be32(num_blks, SCpnt->cmnd + 10);
		SCpnt->cmnd[14] = 0;
		SCpnt->cmnd[15] = 0;
	} else if (sdp->use_10_for_rw || (num_blks > 0xff) ||
		   (lba > 0x1fffff) || scsi_device_protection(sdp)) {
		SCpnt->cmnd[0] = is_write ? WRITE_10 : READ_10;
		SCpnt->cmnd[1] = protect;
		if (unlikely(have_fua))
			SCpnt->cmnd[1] |= 0x8;
		put_unaligned_be32(lba, SCpnt->cmnd + 2);
		SCpnt->cmnd[6] = 0;
		put_unaligned_be16(num_blks, SCpnt->cmnd + 7);
		SCpnt->cmnd[9] = 0;
	} else {
		if (unlikely(have_fua)) {
			/*
			 * This happens only if this drive failed
			 * 10byte rw command with ILLEGAL_REQUEST
			 * during operation and thus turned off
			 * use_10_for_rw.
			 */
			scmd_printk(KERN_ERR, SCpnt,
				    "FUA write on READ/WRITE(6) drive\n");
			goto out;
		}
		/* rely on lba <= 0x1fffff so cmnd[1] will be zeroed */
		put_unaligned_be32(lba, SCpnt->cmnd + 0);
		SCpnt->cmnd[0] = is_write ? WRITE_6 : READ_6;
		SCpnt->cmnd[4] = (unsigned char) num_blks;
		SCpnt->cmnd[5] = 0;
	}
	SCpnt->sdb.length = num_blks * sect_sz;

	/*
	 * We shouldn't disconnect in the middle of a sector, so with a dumb
	 * host adapter, it's safe to assume that we can at least transfer
	 * this many bytes between each connect / disconnect.
	 */
	SCpnt->transfersize = sect_sz;
	SCpnt->underflow = num_blks << 9;
	SCpnt->allowed = SD_MAX_RETRIES;

	/*
	 * This indicates that the command is ready from our end to be
	 * queued.
	 */
	ret = BLKPREP_OK;
 out:
	if (zoned_write && ret != BLKPREP_OK)
		sd_zbc_write_unlock_zone(SCpnt);

	return ret;
}