On 14-07-13 06:55 PM, Elliott, Robert (Server Storage) wrote:
-----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...)
These are only informational (i.e. only consumed by
'cat /proc/scsi/scsi_debug/<host_id>'). This could be addressed
if more meat is added to the various ".eh*" entry points.
I'd look for help from Hannes (cc-ed) in this area.
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.
Yes. The driver could make the distinction between SCSI
errors found early in device server processing (e.g. UAs
and Illegal Requests) from errors found later such as
Medium Error. But that adds complexity. The simplest
approach would be to skip TSF injection if any error is
being reported. In the rare case where the driver wants
to delay the response and has no space (i.e. an attempt
to exceed CAN_QUEUE/scsi_debug_max_queue) the error could
take precedence by skipping the delay and doing an
in-thread scsi_done() call.
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...)
Yes.
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...)
I consider the PI+LBP parts of this driver to be maintained
by Martin Petersen (cc-ed) and I'm hoping he will look at
this area (and its lock safety) when the dust settles.
I noticed that scsi_debug is reporting SPC-3 compliance and
that probably should be upped to SPC-4.
In summary, I would like to leave this oversized "v2" patch
as is. Then address some of the issues raised here as
a series of small, follow-up patches including some input
from those cc-ed in this reply.
BTW The driver documentation at:
http://sg.danny.cz/sg/sdebug26.html
has been updated reflecting this v2 patch. There was a temporary
version at: http://sg.danny.cz/scsi/sdebug26.html
which will be removed (or made a symlink to the former).
Doug Gilbert
--
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
