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

[Douglas Gilbert]

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

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 

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

[Douglas Gilbert]

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

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

[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

[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

[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 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
---
  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