On Tue, 2013-08-27 at 18:17 -0400, Jörn Engel wrote:
> Here is a fun patch in an early state. Essentially I want to trace
> scsi commands, which has already been done long ago. The problem I
> have is that I care about all the scsi commands for one particular
> device - without knowing in advance which device it will be. Once I
> know the device in question, I want to dump the last X commands.
>
> The existing tracing is covering all commands to all devices. So
> either I have to oversize my trace buffer and dump way too much, or I
> will miss the stuff I am interested in most of the time. Hence my
> per-device trace buffer (called fdr to avoid namespace collisions).
>
Sounds like a useful idea..
However, it adds another kmalloc + spin_lock -> spin_unlock in the fast
path for each struct scsi_cmnd I/O dispatch, which is exactly what the
scsi-mq prototype code is trying to avoid at all costs.
So that said, this logic should be disabled (by default) with some
manner of sysfs attribute in /sys/class/scsi_device (or some other
mechanism) that allows it to be enabled/disabled on a per struct
scsi_device basis.
I imagine that is already on your TODO, but given the gaping hole in
SCSI performance vs. raw block that scsi-mq is attempting to address, I
just wanted to make sure. ;)
Thanks,
--nab
> Steven, is there some magic 2-line patch that would also give me what
> I want without having to reinvent all your infrastructure and doing
> such a bad job?
>
> Jörn
>
> --
> It is better to die of hunger having lived without grief and fear,
> than to live with a troubled spirit amid abundance.
> -- Epictetus
>
>
> And dump the fdr whenever we run into a timeout. If the timeout is
> caused by a crashed target and the target crash is triggered by a weird
> command sent by us, we might want to know which commands we send out.
>
> Signed-off-by: Joern Engel <joern@xxxxxxxxx>
> ---
> drivers/scsi/scsi.c | 31 +++++++++++++++++++++++++++++++
> drivers/scsi/scsi_error.c | 38 ++++++++++++++++++++++++++++++++++++++
> drivers/scsi/scsi_scan.c | 1 +
> include/scsi/scsi_device.h | 13 +++++++++++++
> 4 files changed, 83 insertions(+)
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index a884f13..54168bb 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -647,6 +647,36 @@ void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmnd *cmd)
> }
> EXPORT_SYMBOL(scsi_cmd_get_serial);
>
> +static void fdr_add(struct fdr_ring *ring, struct fdr_entry *entry)
> +{
> + struct fdr_entry *old;
> + unsigned long flags;
> + int index;
> +
> + spin_lock_irqsave(&ring->fdr_lock, flags);
> + index = ring->last_entry + 1;
> + if (index >= ARRAY_SIZE(ring->entry))
> + index = 0;
> + old = ring->entry[index];
> + ring->entry[index] = entry;
> + ring->last_entry = index;
> + spin_unlock_irqrestore(&ring->fdr_lock, flags);
> + kfree(old);
> +}
> +
> +static void fdr_scsi_cmd(struct scsi_cmnd *cmd)
> +{
> + struct fdr_entry *new;
> +
> + new = kmalloc(sizeof(*new) + cmd->cmd_len, GFP_KERNEL);
> + if (!new)
> + return;
> + new->ts_nsec = local_clock();
> + new->size = cmd->cmd_len;
> + memcpy(new->data, cmd->cmnd, new->size);
> + fdr_add(&cmd->device->fdr, new);
> +}
> +
> /**
> * scsi_dispatch_command - Dispatch a command to the low-level driver.
> * @cmd: command block we are dispatching.
> @@ -747,6 +777,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
> scsi_done(cmd);
> } else {
> trace_scsi_dispatch_cmd_start(cmd);
> + fdr_scsi_cmd(cmd);
> cmd->scsi_done = scsi_done;
> rtn = host->hostt->queuecommand(host, cmd);
> }
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 0dfae50..829cb74 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -132,6 +132,41 @@ static void queue_for_eh(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
> schedule_work(&scmd->abort_work);
> }
>
> +static void scsi_fdr_dump(struct fdr_entry *entry)
> +{
> + char buf[64 + 8 + 2];
> + unsigned long ts = entry->ts_nsec;
> + unsigned long rem_nsec;
> +
> + rem_nsec = do_div(ts, 1000000000);
> + hex_dump_to_buffer(entry->data, entry->size, 32, 8, buf, sizeof(buf), 0);
> + pr_info("%6lu.%06lu: %s\n", ts, rem_nsec, buf);
> +}
> +
> +static void fdr_dump(struct fdr_ring *ring, void (*dump) (struct fdr_entry * entry))
> +{
> + struct fdr_entry *entry;
> + unsigned long flags;
> + int i;
> +
> + spin_lock_irqsave(&ring->fdr_lock, flags);
> + for (i = ring->last_entry + 1; i < ARRAY_SIZE(ring->entry); i++) {
> + entry = ring->entry[i];
> + ring->entry[i] = NULL;
> + if (entry)
> + dump(entry);
> + kfree(entry);
> + }
> + for (i = 0; i < ring->last_entry; i++) {
> + entry = ring->entry[i];
> + ring->entry[i] = NULL;
> + if (entry)
> + dump(entry);
> + kfree(entry);
> + }
> + spin_unlock_irqrestore(&ring->fdr_lock, flags);
> +}
> +
> /**
> * scsi_times_out - Timeout function for normal scsi commands.
> * @req: request that is timing out.
> @@ -148,6 +183,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
> enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
> struct Scsi_Host *host = scmd->device->host;
>
> + sdev_printk(KERN_INFO, scmd->device, "dumping cdb recorder\n");
> + fdr_dump(&scmd->device->fdr, scsi_fdr_dump);
> +
> trace_scsi_dispatch_cmd_timeout(scmd);
> scsi_log_completion(scmd, TIMEOUT_ERROR);
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index db89445..85dbae4 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -252,6 +252,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
> INIT_LIST_HEAD(&sdev->starved_entry);
> INIT_LIST_HEAD(&sdev->event_list);
> spin_lock_init(&sdev->list_lock);
> + spin_lock_init(&sdev->fdr.fdr_lock);
> INIT_WORK(&sdev->event_work, scsi_evt_thread);
> INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);
> /* Limit scsi messages per device */
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index bafc41c..00b7e9b 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -97,6 +97,18 @@ enum scsi_err_inj_duration {
> };
> #endif /* CONFIG_SCSI_ERROR_INJECTOR */
>
> +struct fdr_entry {
> + unsigned long ts_nsec;
> + u32 size;
> + u8 data[0];
> +};
> +
> +struct fdr_ring {
> + spinlock_t fdr_lock;
> + int last_entry;
> + struct fdr_entry *entry[1000];
> +};
> +
> struct scsi_device {
> struct Scsi_Host *host;
> struct request_queue *request_queue;
> @@ -222,6 +234,7 @@ struct scsi_device {
> struct scsi_dh_data *scsi_dh_data;
> enum scsi_device_state sdev_state;
> struct ratelimit_state sdev_ratelimit;
> + struct fdr_ring fdr;
> unsigned long sdev_data[0];
> } __attribute__((aligned(sizeof(unsigned long))));
>
--
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
