> -----Original Message----- > From: linux-scsi-owner@xxxxxxxxxxxxxxx [mailto:linux-scsi- > owner@xxxxxxxxxxxxxxx] On Behalf Of Douglas Gilbert > Sent: Monday, 07 July, 2014 8:31 AM > To: SCSI development list > Subject: [PATCH v2] scsi_debug: support scsi-mq, queues and locks > > Resend, looks like the list does not like html attachments. > > > This v2 patch is against Christoph's core-for-3.17 branch which > includes scsi-mq V2. Here is a link to a partially updated > version of the scsi_debug html page. > http://sg.danny.cz/scsi/sdebug26.html Reviewed-by: Robert Elliott <elliott@xxxxxx> A few minor concerns: 1. In scsi_debug_abort, does num_aborts needs to be atomic - can the SCSI midlayer have concurrent .eh_abort_handler calls in progress? +static int scsi_debug_abort(struct scsi_cmnd *SCpnt) + ++num_aborts; (I don't think this patch changes that from before...) 2. Same question for: num_dev_resets num_target_resets num_bus_resets num_host_resets (I don't think this patch changes that from before...) 3. schedule_resp includes this comment about the new TASK SET FULL injection code: + /* if (tsf) simulate device reporting SCSI status of TASK SET FULL. + * Might override existing CHECK CONDITION. */ If a TASK SET FULL is injected over a CHECK CONDITION/ UNIT ATTENTION created by check_readiness(): + k = find_first_bit(devip->uas_bm, SDEBUG_NUM_UAS); ... + clear_bit(k, devip->uas_bm); then it looks like that unit attention is lost forever. 4. In scsi_debug_show_info: + "num_tgts=%d, shared (ram) size=%d MB, opts=0x%x, " and the modparam string describing that variable: MODULE_PARM_DESC(dev_size_mb, "size in MB of ram shared by devs(def=8)"); the units are really MiB, not MB. (I don't think this patch changes that from before...) 5. For the UNMAP command, this modparam: MODULE_PARM_DESC(lbprz, "unmapped blocks return 0 on read (def=1)"); always causes unmap_region to zero out the blocks: if (scsi_debug_lbprz) { memset(fake_storep + lba * scsi_debug_sector_size, 0, scsi_debug_sector_size * scsi_debug_unmap_granularity); } That doesn't recognize that unmap requests via UNMAP commands are just hints/suggestions, not mandatory. The same is true in ATA for the DATA SET MANAGEMENT/TRIM command. Zeroing out is fine for when resp_write_same is the caller of unmap_region and either NDOB=1 or the Data-Out Buffer contains all zeros - if WRITE SAME with UNMAP=1 doesn't cause an unmap, it still writes all zeros to the blocks. When resp_unmap is the caller, though, there is no guarantee that the data will change. Maybe another modparam should be included to cause the driver to purposely ignore unmap requests? That might help more people realize the danger in these commands. (e.g., I think mdraid assumes UNMAP will result in zeros for RAID-5/6 volumes, which means parity will be calculated wrong if the drive doesn't really unmap). (I don't think this patch changes that from before...) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
