From: Ewan Milne <emilne@redhat.com>
To: Hannes Reinecke <hare@suse.de>
Cc: James Bottomley <jbottomley@parallels.com>,
Christoph Hellwig <hch@infradead.org>,
linux-scsi@vger.kernel.org, Robert Elliott <elliott@hp.com>
Subject: Re: [PATCH 21/26] scsi: simplify scsi_log_(send|completion)
Date: Mon, 06 Oct 2014 15:53:03 -0400 [thread overview]
Message-ID: <1412625183.5241.148.camel@localhost.localdomain> (raw)
In-Reply-To: <1412586086-129821-22-git-send-email-hare@suse.de>
Rats. I found another case of "(null)" appearing in the log messages
where I think you really just wanted no text at all to appear.
Sorry I didn't find it last time along with the other one...
-Ewan
On Mon, 2014-10-06 at 11:01 +0200, Hannes Reinecke wrote:
> Simplify scsi_log_(send|completion) by externalizing
> scsi_mlreturn_string() and always print the command address.
>
> Reviewed-by: Robert Elliott <elliott@hp.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> drivers/scsi/constants.c | 39 ++++++++++++++++++++++++++++++++++++---
> drivers/scsi/scsi.c | 43 ++++++-------------------------------------
> drivers/scsi/scsi_lib.c | 13 ++++++++++---
> include/scsi/scsi_dbg.h | 3 ++-
> 4 files changed, 54 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
> index 1ebdc5d..59b7f9e 100644
> --- a/drivers/scsi/constants.c
> +++ b/drivers/scsi/constants.c
> @@ -1439,19 +1439,52 @@ const char *scsi_driverbyte_string(int result)
> }
> EXPORT_SYMBOL(scsi_driverbyte_string);
>
> -void scsi_print_result(struct scsi_cmnd *cmd)
> +#ifdef CONFIG_SCSI_CONSTANTS
> +#define scsi_mlreturn_name(result) { result, #result }
> +static const struct value_name_pair scsi_mlreturn_arr[] = {
> + scsi_mlreturn_name(NEEDS_RETRY),
> + scsi_mlreturn_name(SUCCESS),
> + scsi_mlreturn_name(FAILED),
> + scsi_mlreturn_name(QUEUED),
> + scsi_mlreturn_name(SOFT_ERROR),
> + scsi_mlreturn_name(ADD_TO_MLQUEUE),
> + scsi_mlreturn_name(TIMEOUT_ERROR),
> + scsi_mlreturn_name(SCSI_RETURN_NOT_HANDLED),
> + scsi_mlreturn_name(FAST_IO_FAIL)
> +};
> +#endif
> +
> +const char *scsi_mlreturn_string(int result)
> +{
> +#ifdef CONFIG_SCSI_CONSTANTS
> + const struct value_name_pair *arr = scsi_mlreturn_arr;
> + int k;
> +
> + for (k = 0; k < ARRAY_SIZE(scsi_mlreturn_arr); ++k, ++arr) {
> + if (result == arr->value)
> + return arr->name;
> + }
> +#endif
> + return NULL;
> +}
> +EXPORT_SYMBOL(scsi_mlreturn_string);
> +
> +void scsi_print_result(struct scsi_cmnd *cmd, const char *msg, int disposition)
> {
> + const char *mlret_string = scsi_mlreturn_string(disposition);
> const char *hb_string = scsi_hostbyte_string(cmd->result);
> const char *db_string = scsi_driverbyte_string(cmd->result);
>
> if (hb_string || db_string)
> scmd_printk(KERN_INFO, cmd,
> - "Result: hostbyte=%s driverbyte=%s",
> + "%s%s Result: hostbyte=%s driverbyte=%s",
> + msg, mlret_string ? mlret_string : "UNKNOWN",
> hb_string ? hb_string : "invalid",
> db_string ? db_string : "invalid");
> else
> scmd_printk(KERN_INFO, cmd,
> - "Result: hostbyte=0x%02x driverbyte=0x%02x",
> + "%s%s Result: hostbyte=0x%02x driverbyte=0x%02x",
> + msg, mlret_string ? mlret_string : "UNKNOWN",
> host_byte(cmd->result), driver_byte(cmd->result));
> }
> EXPORT_SYMBOL(scsi_print_result);
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 61aeaf1..3d81a07 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -527,9 +527,9 @@ void scsi_log_send(struct scsi_cmnd *cmd)
> *
> * 1: nothing (match completion)
> *
> - * 2: log opcode + command of all commands
> + * 2: log opcode + command of all commands + cmd address
> *
> - * 3: same as 2 plus dump cmd address
> + * 3: same as 2
> *
> * 4: same as 3 plus dump extra junk
> */
> @@ -537,10 +537,8 @@ void scsi_log_send(struct scsi_cmnd *cmd)
> level = SCSI_LOG_LEVEL(SCSI_LOG_MLQUEUE_SHIFT,
> SCSI_LOG_MLQUEUE_BITS);
> if (level > 1) {
> - scmd_printk(KERN_INFO, cmd, "Send: ");
> - if (level > 2)
> - printk("0x%p ", cmd);
> - printk("\n");
> + scmd_printk(KERN_INFO, cmd,
> + "Send: scmd 0x%p\n", cmd);
> scsi_print_command(cmd);
> if (level > 3) {
> printk(KERN_INFO "buffer = 0x%p, bufflen = %d,"
> @@ -565,7 +563,7 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
> *
> * 2: same as 1 but for all command completions.
> *
> - * 3: same as 2 plus dump cmd address
> + * 3: same as 2
> *
> * 4: same as 3 plus dump extra junk
> */
> @@ -574,36 +572,7 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
> SCSI_LOG_MLCOMPLETE_BITS);
> if (((level > 0) && (cmd->result || disposition != SUCCESS)) ||
> (level > 1)) {
> - scmd_printk(KERN_INFO, cmd, "Done: ");
> - if (level > 2)
> - printk("0x%p ", cmd);
> - /*
> - * Dump truncated values, so we usually fit within
> - * 80 chars.
> - */
> - switch (disposition) {
> - case SUCCESS:
> - printk("SUCCESS\n");
> - break;
> - case NEEDS_RETRY:
> - printk("RETRY\n");
> - break;
> - case ADD_TO_MLQUEUE:
> - printk("MLQUEUE\n");
> - break;
> - case FAILED:
> - printk("FAILED\n");
> - break;
> - case TIMEOUT_ERROR:
> - /*
> - * If called via scsi_times_out.
> - */
> - printk("TIMEOUT\n");
> - break;
> - default:
> - printk("UNKNOWN\n");
> - }
> - scsi_print_result(cmd);
> + scsi_print_result(cmd, "Done: ", disposition);
> scsi_print_command(cmd);
> if (status_byte(cmd->result) & CHECK_CONDITION)
> scsi_print_sense(cmd);
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index b853659..2df485f 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -831,7 +831,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> int error = 0;
> struct scsi_sense_hdr sshdr;
> bool sense_valid = false;
> - int sense_deferred = 0;
> + int sense_deferred = 0, level = 0;
> enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
> ACTION_DELAYED_RETRY} action;
> unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
> @@ -1037,8 +1037,15 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> switch (action) {
> case ACTION_FAIL:
> /* Give up and fail the remainder of the request */
> - if (!(req->cmd_flags & REQ_QUIET)) {
> - scsi_print_result(cmd);
> + if (unlikely(scsi_logging_level))
> + level = SCSI_LOG_LEVEL(SCSI_LOG_MLQUEUE_SHIFT,
> + SCSI_LOG_MLQUEUE_BITS);
> + /*
> + * if logging is enabled the failure will be printed
> + * in scsi_log_completion(), so avoid duplicate messages
> + */
> + if (!level && !(req->cmd_flags & REQ_QUIET)) {
> + scsi_print_result(cmd, NULL, FAILED);
^^^^--
I think this is what is causing other undesirable log messages:
Oct 6 15:30:51 rhel-storage-01 kernel: [ 1257.681129] sd 9:0:0:0: [sde] (null)FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
^^^^^^--- here
because scsi_print_result() is using "%s%s" and the "msg" argument is NULL
scmd_printk(KERN_INFO, cmd,
"%s%s Result: hostbyte=%s driverbyte=%s",
msg, mlret_string ? mlret_string : "UNKNOWN",
hb_string ? hb_string : "invalid",
db_string ? db_string : "invalid");
> if (driver_byte(result) & DRIVER_SENSE)
> scsi_print_sense(cmd);
> scsi_print_command(cmd);
> diff --git a/include/scsi/scsi_dbg.h b/include/scsi/scsi_dbg.h
> index f41a86b..7982795 100644
> --- a/include/scsi/scsi_dbg.h
> +++ b/include/scsi/scsi_dbg.h
> @@ -17,9 +17,10 @@ extern void scsi_print_sense(const struct scsi_cmnd *);
> extern void __scsi_print_sense(const struct scsi_device *, const char *name,
> const unsigned char *sense_buffer,
> int sense_len);
> -extern void scsi_print_result(struct scsi_cmnd *);
> +extern void scsi_print_result(struct scsi_cmnd *, const char *, int);
> extern const char *scsi_hostbyte_string(int);
> extern const char *scsi_driverbyte_string(int);
> +extern const char *scsi_mlreturn_string(int);
> extern const char *scsi_sense_key_string(unsigned char);
> extern const char *scsi_extd_sense_format(unsigned char, unsigned char,
> const char **);
next prev parent reply other threads:[~2014-10-06 19:53 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-06 9:01 [PATCHv6 00/26] scsi logging update (the boring part) Hannes Reinecke
2014-10-06 9:01 ` [PATCH 01/26] Remove scsi_cmd_print_sense_hdr() Hannes Reinecke
2014-10-06 9:01 ` [PATCH 02/26] sd: Remove scsi_print_sense() in sd_done() Hannes Reinecke
2014-10-06 9:01 ` [PATCH 03/26] aha152x: Debug output update and whitespace cleanup Hannes Reinecke
2014-10-06 9:01 ` [PATCH 04/26] scsi: introduce sdev_prefix_printk() Hannes Reinecke
2014-10-06 9:01 ` [PATCH 05/26] scsi: Use sdev as argument for sense code printing Hannes Reinecke
2014-10-06 9:01 ` [PATCH 06/26] acornscsi: use scsi_print_command() Hannes Reinecke
2014-10-06 9:01 ` [PATCH 07/26] fas216: Return DID_ERROR for incomplete data transfer Hannes Reinecke
2014-10-06 9:01 ` [PATCH 08/26] fas216: Update logging messages Hannes Reinecke
2014-10-06 9:01 ` [PATCH 09/26] 53c700: remove scsi_print_sense() usage Hannes Reinecke
2014-10-06 9:01 ` [PATCH 10/26] scsi: stop decoding if scsi_normalize_sense() fails Hannes Reinecke
2014-10-06 9:01 ` [PATCH 11/26] scsi: do not decode sense extras Hannes Reinecke
2014-10-06 9:01 ` [PATCH 12/26] scsi: use 'bool' as return value for scsi_normalize_sense() Hannes Reinecke
2014-10-06 9:01 ` [PATCH 13/26] scsi: remove scsi_print_status() Hannes Reinecke
2014-10-06 9:01 ` [PATCH 14/26] Implement scsi_opcode_sa_name Hannes Reinecke
2014-10-06 9:01 ` [PATCH 15/26] scsi: merge print_opcode_name() Hannes Reinecke
2014-10-06 9:01 ` [PATCH 16/26] scsi: consolidate opcode lookup in scsi_opcode_sa_name() Hannes Reinecke
2014-10-06 9:01 ` [PATCH 17/26] scsi: repurpose the last argument from print_opcode_name() Hannes Reinecke
2014-10-06 9:01 ` [PATCH 18/26] scsi: Remove scsi_print_command when calling abort Hannes Reinecke
2014-10-06 9:01 ` [PATCH 19/26] scsi: separate out scsi_(host|driver)byte_string() Hannes Reinecke
2014-10-06 9:01 ` [PATCH 20/26] sd: Cleanup logging Hannes Reinecke
2014-10-06 9:01 ` [PATCH 21/26] scsi: simplify scsi_log_(send|completion) Hannes Reinecke
2014-10-06 19:53 ` Ewan Milne [this message]
2014-10-07 8:45 ` Hannes Reinecke
2014-10-06 9:01 ` [PATCH 22/26] scsi: fixup logging messages in scsi_error.c Hannes Reinecke
2014-10-06 9:01 ` [PATCH 23/26] scsi: use shost argument in scsi_eh_prt_fail_stats Hannes Reinecke
2014-10-06 9:01 ` [PATCH 24/26] scsi_error: document scsi_try_to_abort_cmd Hannes Reinecke
2014-10-06 9:01 ` [PATCH 25/26] scsi: check for corrent return code in scsi_eh_abort_cmds() Hannes Reinecke
2014-10-06 9:01 ` [PATCH 26/26] scsi: correct return values for .eh_abort_handler implementations Hannes Reinecke
-- strict thread matches above, loose matches on Subject: below --
2014-10-07 9:02 [PATCHv7 00/26] scsi logging update (the boring part) Hannes Reinecke
2014-10-07 9:03 ` [PATCH 21/26] scsi: simplify scsi_log_(send|completion) Hannes Reinecke
2014-10-08 20:41 ` Elliott, Robert (Server Storage)
2014-10-15 13:10 ` Hannes Reinecke
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1412625183.5241.148.camel@localhost.localdomain \
--to=emilne@redhat.com \
--cc=elliott@hp.com \
--cc=hare@suse.de \
--cc=hch@infradead.org \
--cc=jbottomley@parallels.com \
--cc=linux-scsi@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox