storvsc: add more logging for error and warning messages
Subsystems (2)
PATCH 1/1 storvsc: add more logging for error and warning messages
Introduce a logging level for storvsc to log certain error/warning messages. Those messages are helpful in some environments, e.g. Microsoft Azure, for customer support and troubleshooting purposes.
--- drivers/scsi/storvsc_drv.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)
drivers/scsi/storvsc_drv.c +29 -1
@@ -164,6 +164,21 @@ static int sense_buffer_size = PRE_WIN8_STORVSC_SENSE_BUFFER_SIZE;*/ static int vmstor_proto_version; +#define STORVSC_LOGGING_NONE 0 +#define STORVSC_LOGGING_ERROR 1 +#define STORVSC_LOGGING_WARN 2 + +static int logging_level =STORVSC_LOGGING_ERROR ; +module_param (logging_level, int,S_IRUGO |S_IWUSR ); +MODULE_PARM_DESC (logging_level, +"Logging level, 0 - None, 1 - Error (default), 2 - Warning." ); + +static inline bool do_logging (int level) +{ +return (logging_level >= level) ? true : false; +} + +struct vmscsi_win8_extension {/* * The following were added in Windows 8
@@ -1183,7 +1198,7 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request) scmnd->result = vm_srb->scsi_status; -if (scmnd->result) { +if (scmnd->result &&do_logging (STORVSC_LOGGING_ERROR )) {if (scsi_normalize_sense (scmnd->sense_buffer,SCSI_SENSE_BUFFERSIZE , &sense_hdr)) scsi_print_sense_hdr(scmnd->device, "storvsc",
@@ -1239,12 +1254,25 @@ static void storvsc_on_io_completion(struct hv_device *device, stor_pkt->vm_srb.sense_info_length = vstor_packet->vm_srb.sense_info_length; +if (vstor_packet->vm_srb.scsi_status !=0 || + vstor_packet->vm_srb.srb_status !=SRB_STATUS_SUCCESS ) +if (do_logging (STORVSC_LOGGING_WARN )) +dev_warn (&device->device, +"cmd 0x%x scsi status 0x%x srb status 0x%x\n" , + stor_pkt->vm_srb.cdb[0 ], + vstor_packet->vm_srb.scsi_status, + vstor_packet->vm_srb.srb_status);if ((vstor_packet->vm_srb.scsi_status &0xFF ) ==0x02 ) {/* CHECK_CONDITION */ if (vstor_packet->vm_srb.srb_status &SRB_STATUS_AUTOSENSE_VALID ) {/* autosense data available */ +if (do_logging (STORVSC_LOGGING_WARN )) +dev_warn (&device->device, +"stor pkt %p autosense data valid - len %d\n" , + request, + vstor_packet->vm_srb.sense_info_length); memcpy(request->cmd->sense_buffer, vstor_packet->vm_srb.sense_data,
Signature · 1 line
1.8.5.6
Quoted 3 lines · Long Li
Introduce a logging level for storvsc to log certain error/warning messages. Those messages are helpful in some environments, e.g. Microsoft Azure, for customer support and troubleshooting purposes.
[]
drivers/scsi/storvsc_drv.c
Quoted 1 line
drivers/scsi/storvsc_drv.c +0
-0
[]
drivers/scsi/storvsc_drv.c static int sense_buffer_size = PRE_WIN8_STORVSC_SENSE_BUFFER_SIZE; · line 178
Quoted 3 lines
drivers/scsi/storvsc_drv.c +3
-0+static inline bool do_logging (int level) +{ +return (logging_level >= level) ? true : false;
The ternary is not necessary
return logging_level >= level;
is enough
Quoted 12 lines
drivers/scsi/storvsc_drv.c +3
-0+} + +struct
vmscsi_win8_extension{
/*
* The following were added in Windows 8drivers/scsi/storvsc_drv.c +0
-0@@ -1183,7 +1198,7 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request)
scmnd->result=vm_srb->scsi_status;drivers/scsi/storvsc_drv.c +1
-1-if (scmnd->result) { +if (scmnd->result &&do_logging (STORVSC_LOGGING_ERROR )) {
Quoted 3 lines
if (
scsi_normalize_sense(scmnd->sense_buffer, SCSI_SENSE_BUFFERSIZE, &sense_hdr)) scsi_print_sense_hdr(scmnd->device, "storvsc",
Is it appropriate to make this scsi_normalize_sense call conditional on do_logging here?
drivers/scsi/storvsc_drv.c static void storvsc_on_io_completion(struct hv_device *device, · line 1264
Quoted 12 lines
drivers/scsi/storvsc_drv.c +0
-0@@ -1239,12 +1254,25 @@ static void storvsc_on_io_completion(struct hv_device *device,
stor_pkt->vm_srb.sense_info_length=
vstor_packet->vm_srb.sense_info_length;drivers/scsi/storvsc_drv.c +8
-0+if (vstor_packet->vm_srb.scsi_status !=0 || + vstor_packet->vm_srb.srb_status !=SRB_STATUS_SUCCESS ) +if (do_logging (STORVSC_LOGGING_WARN )) +dev_warn (&device->device, +"cmd 0x%x scsi status 0x%x srb status 0x%x\n" , + stor_pkt->vm_srb.cdb[0 ], + vstor_packet->vm_srb.scsi_status, + vstor_packet->vm_srb.srb_status);
It might make some sense to use another macro indirection like
#define svc_log_warn(dev, level, fmt, ...) \
do { \
if (do_logging(STORSVC_LOGGING_##level) \
dev_warn(&(dev)->device, fmt, ##__VA_ARGS__); \
} while (0)So a use could be:
if (vstore_packet...) svc_log_warn(device, WARN, ...);
Quoted 1 line
Thanks Joe.
I'll send out another patch.
Quoted 76 lines
-----Original Message-----
From: Joe Perches [mailto:[email protected]]
Sent: Thursday, December 3, 2015 6:28 PM
To: Long Li <[email protected]>; KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; James E.J. Bottomley <[email protected]>
Cc: [email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH] storvsc: add more logging for error and warning messagesQuoted 3 lines · Long Li
Introduce a logging level for storvsc to log certain error/warning messages. Those messages are helpful in some environments, e.g. Microsoft Azure, for customer support and troubleshooting purposes.
[]
drivers/scsi/storvsc_drv.c
Quoted 1 line
drivers/scsi/storvsc_drv.c +0
-0[]
drivers/scsi/storvsc_drv.c
Quoted 2 lines
drivers/scsi/storvsc_drv.c +2
-0+static inline bool do_logging (int level) { +return (logging_level >= level) ? true : false;The ternary is not necessary
return logging_level >= level;is enough
Quoted 13 lines
drivers/scsi/storvsc_drv.c +3
-0+} + +struct
vmscsi_win8_extension{
/*
* The following were added in Windows 8 @@ -1183,7 +1198,7 @@ static voidstorvsc_command_completion(struct storvsc_cmd_request *cmd_request)
scmnd->result=vm_srb->scsi_status;drivers/scsi/storvsc_drv.c +1
-1-if (scmnd->result) { +if (scmnd->result &&do_logging (STORVSC_LOGGING_ERROR )) {Quoted 3 lines
if (
scsi_normalize_sense(scmnd->sense_buffer, SCSI_SENSE_BUFFERSIZE, &sense_hdr)) scsi_print_sense_hdr(scmnd->device, "storvsc",Is it appropriate to make this
scsi_normalize_sensecall conditional ondo_logginghere?drivers/scsi/storvsc_drv.c static void storvsc_on_io_completion(struct
Quoted 1 line
drivers/scsi/storvsc_drv.c +0
-0@@ -1239,12 +1254,25 @@ static void storvsc_on_io_completion(struct
hv_device*device,drivers/scsi/storvsc_drv.c static void storvsc_on_io_completion(struct hv_device *device,
Quoted 8 lines
stor_pkt->vm_srb.sense_info_length=
vstor_packet->vm_srb.sense_info_length;drivers/scsi/storvsc_drv.c +5
-0+if (vstor_packet->vm_srb.scsi_status !=0 || + vstor_packet->vm_srb.srb_status !=SRB_STATUS_SUCCESS ) +if (do_logging (STORVSC_LOGGING_WARN )) + dev_warn(&device->device, + "cmd 0x%x scsi status 0x%x srb status0x%x\n",
drivers/scsi/storvsc_drv.c static void storvsc_on_io_completion(struct hv_device *device, · line 1264
Quoted 3 lines
drivers/scsi/storvsc_drv.c +3
-0+ stor_pkt->vm_srb.cdb[0 ], + vstor_packet->vm_srb.scsi_status, + vstor_packet->vm_srb.srb_status);It might make some sense to use another macro indirection like
#define
svc_log_warn(dev, level, fmt, ...)\do { \ if (do_logging(STORSVC_LOGGING_##level) \ dev_warn(&(dev)->device, fmt, ##__VA_ARGS__); \ } while (0)So a use could be:
if (vstore_packet...) svc_log_warn(device, WARN, ...);Quoted 1 line
Quoted 4 lines · Long Li
Introduce a logging level for storvsc to log certain error/warning messages. Those messages are helpful in some environments, e.g. Microsoft Azure, for customer support and troubleshooting purposes.
I have an alternative suggestion: let's use dynamic debug! Basically, we need to convert all non-error logging to using dev_dbg() and this can be enabled dynamically when needed, even reboot won't be required.
Quoted 67 lines
Signed-off-by Long Li <[email protected]>--- drivers/scsi/storvsc_drv.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)drivers/scsi/storvsc_drv.c +29
-1@@ -164,6 +164,21 @@ static int sense_buffer_size = PRE_WIN8_STORVSC_SENSE_BUFFER_SIZE;*/ static int vmstor_proto_version; +#define STORVSC_LOGGING_NONE 0 +#define STORVSC_LOGGING_ERROR 1 +#define STORVSC_LOGGING_WARN 2 + +static int logging_level =STORVSC_LOGGING_ERROR ; +module_param (logging_level, int,S_IRUGO |S_IWUSR ); +MODULE_PARM_DESC (logging_level, +"Logging level, 0 - None, 1 - Error (default), 2 - Warning." ); + +static inline bool do_logging (int level) +{ +return (logging_level >= level) ? true : false; +} + +struct vmscsi_win8_extension {/* * The following were added in Windows 8 @@ -1183,7 +1198,7 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request) scmnd->result = vm_srb->scsi_status; -if (scmnd->result) { +if (scmnd->result &&do_logging (STORVSC_LOGGING_ERROR )) {if (scsi_normalize_sense (scmnd->sense_buffer,SCSI_SENSE_BUFFERSIZE , &sense_hdr)) scsi_print_sense_hdr(scmnd->device, "storvsc",@@ -1239,12 +1254,25 @@ static void storvsc_on_io_completion(struct hv_device *device, stor_pkt->vm_srb.sense_info_length = vstor_packet->vm_srb.sense_info_length; +if (vstor_packet->vm_srb.scsi_status !=0 || + vstor_packet->vm_srb.srb_status !=SRB_STATUS_SUCCESS ) +if (do_logging (STORVSC_LOGGING_WARN )) +dev_warn (&device->device, +"cmd 0x%x scsi status 0x%x srb status 0x%x\n" , + stor_pkt->vm_srb.cdb[0 ], + vstor_packet->vm_srb.scsi_status, + vstor_packet->vm_srb.srb_status);if ((vstor_packet->vm_srb.scsi_status &0xFF ) ==0x02 ) {/* CHECK_CONDITION */ if (vstor_packet->vm_srb.srb_status &SRB_STATUS_AUTOSENSE_VALID ) {/* autosense data available */ +if (do_logging (STORVSC_LOGGING_WARN )) +dev_warn (&device->device, +"stor pkt %p autosense data valid - len %d\n" , + request, + vstor_packet->vm_srb.sense_info_length); memcpy(request->cmd->sense_buffer, vstor_packet->vm_srb.sense_data,
Signature · 1 line
Vitaly
Quoted 13 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Friday, December 4, 2015 1:53 AM
To: Long Li <[email protected]>
Cc: KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; James E.J. Bottomley <[email protected]>; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH] storvsc: add more logging for error and warning messagesLong Li <[email protected]> writes:
Quoted 7 lines
Quoted 3 lines
Introduce a logging level for storvsc to log certain error/warning messages. Those messages are helpful in some environments, e.g. Microsoft Azure, for customer support and troubleshooting purposes.
I have an alternative suggestion: let's use dynamic debug! Basically, we need to convert all non-error logging to using
dev_dbg()and this can be enabled dynamically when needed, even reboot won't be required.
This is great idea for debugging!
I think the messages (srb errors) we want to log in this patch are real errors. They are not for debugging, but for customer support in production environment.
Those errors can be ignored in certain specific storage configurations due to some quirks. They are real errors on Azure, so we want to always log them.
Quoted 76 lines
Quoted 35 lines
Signed-off-by Long Li <[email protected]>--- drivers/scsi/storvsc_drv.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)drivers/scsi/storvsc_drv.c +13
-0@@ -164,6 +164,21 @@ static int sense_buffer_size =PRE_WIN8_STORVSC_SENSE_BUFFER_SIZE; */ static int vmstor_proto_version; +#define STORVSC_LOGGING_NONE 0 +#define STORVSC_LOGGING_ERROR 1 +#define STORVSC_LOGGING_WARN 2 + +static int logging_level =STORVSC_LOGGING_ERROR ; +module_param (logging_level, int,S_IRUGO |S_IWUSR ); +MODULE_PARM_DESC (logging_level, +"Logging level, 0 - None, 1 - Error (default), 2 - Warning." ); + +static inline bool do_logging (int level) { +return (logging_level >= level) ? true : false; } + +struct vmscsi_win8_extension {/* * The following were added in Windows 8 @@ -1183,7 +1198,7 @@ static void
storvsc_command_completion(struct storvsc_cmd_request *cmd_request)drivers/scsi/storvsc_drv.c static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request)
Quoted 7 lines
drivers/scsi/storvsc_drv.c +1
-1scmnd->result = vm_srb->scsi_status; -if (scmnd->result) { +if (scmnd->result &&do_logging (STORVSC_LOGGING_ERROR )) {if (scsi_normalize_sense (scmnd->sense_buffer,SCSI_SENSE_BUFFERSIZE , &sense_hdr)) scsi_print_sense_hdr(scmnd->device, "storvsc", @@-1239,12
Quoted 1 line
+1254,25 @@ static void storvsc_on_io_completion(struct
hv_device*device,
drivers/scsi/storvsc_drv.c static void storvsc_on_io_completion(struct hv_device *device,
Quoted 8 lines
drivers/scsi/storvsc_drv.c +5
-0stor_pkt->vm_srb.sense_info_length = vstor_packet->vm_srb.sense_info_length; +if (vstor_packet->vm_srb.scsi_status !=0 || + vstor_packet->vm_srb.srb_status !=SRB_STATUS_SUCCESS ) +if (do_logging (STORVSC_LOGGING_WARN )) + dev_warn(&device->device, + "cmd 0x%x scsi status 0x%x srb status0x%x\n",
drivers/scsi/storvsc_drv.c static void storvsc_on_io_completion(struct hv_device *device,
Quoted 12 lines
drivers/scsi/storvsc_drv.c +6
-0+ stor_pkt->vm_srb.cdb[0 ], + vstor_packet->vm_srb.scsi_status, + vstor_packet->vm_srb.srb_status);if ((vstor_packet->vm_srb.scsi_status &0xFF ) ==0x02 ) {/* CHECK_CONDITION */ if (vstor_packet->vm_srb.srb_status &SRB_STATUS_AUTOSENSE_VALID ) {/* autosense data available */ +if (do_logging (STORVSC_LOGGING_WARN )) + dev_warn(&device->device, + "stor pkt %p autosense data valid -len %d\n",
drivers/scsi/storvsc_drv.c static void storvsc_on_io_completion(struct hv_device *device, · line 1278
Quoted 6 lines
drivers/scsi/storvsc_drv.c +2
-0+ request, + vstor_packet-vm_srb.sense_info_length);
drivers/scsi/storvsc_drv.c +0
-0memcpy(request->cmd->sense_buffer, vstor_packet->vm_srb.sense_data,--
Vitaly