| Message ID | 20171017210352.19580-1-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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)) { > /* >
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.
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.
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.
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; }
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
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)) { /*
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(-)