| Message ID | 1438161835-27960-12-git-send-email-mchristi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hey Nick and Christoph, For the target_core_iblock + rbd/COMPARE_AND_WRITE support approach, I still need a patch like below to be able to allow target_core_iblock.c to be able to specify specific codes like TCM_MISCOMPARE_VERIFY. Is the patch below ok? I made it work similar to how we have other completion functions, target_complete_cmd_with_*, that take in extra amounts of info like length or sense. Instead, I can modify target_complete_cmd and all the callers so they just take/pass the sense and info if needed: void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status, sense_reason_t sense, u32 info) On 07/29/2015 04:23 AM, mchristi@redhat.com wrote: > From: Mike Christie <michaelc@cs.wisc.edu> > > Currently, backend drivers seem to only fail IO with > SAM_STAT_CHECK_CONDITION which gets us > TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE. > For compare and write support we will want to be able to fail with > TCM_MISCOMPARE_VERIFY. This patch adds a new helper that allows backend > drivers to fail with specific sense codes. > > It also allows the backend driver to set the miscompare offset. > > Signed-off-by: Mike Christie <michaelc@cs.wisc.edu> > --- > drivers/target/target_core_transport.c | 33 ++++++++++++++++++++++++++++----- > include/target/target_core_backend.h | 1 + > include/target/target_core_base.h | 5 ++++- > 3 files changed, 33 insertions(+), 6 deletions(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index ce8574b..f9b0527 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -639,8 +639,7 @@ static void target_complete_failure_work(struct work_struct *work) > { > struct se_cmd *cmd = container_of(work, struct se_cmd, work); > > - transport_generic_request_failure(cmd, > - TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE); > + transport_generic_request_failure(cmd, cmd->sense_reason); > } > > /* > @@ -666,14 +665,15 @@ static unsigned char *transport_get_sense_buffer(struct se_cmd *cmd) > return cmd->sense_buffer; > } > > -void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) > +static void __target_complete_cmd(struct se_cmd *cmd, u8 scsi_status, > + sense_reason_t sense_reason) > { > struct se_device *dev = cmd->se_dev; > int success = scsi_status == GOOD; > unsigned long flags; > > cmd->scsi_status = scsi_status; > - > + cmd->sense_reason = sense_reason; > > spin_lock_irqsave(&cmd->t_state_lock, flags); > cmd->transport_state &= ~CMD_T_BUSY; > @@ -716,8 +716,22 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) > > queue_work(target_completion_wq, &cmd->work); > } > + > +void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) > +{ > + __target_complete_cmd(cmd, scsi_status, scsi_status ? > + TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE : > + TCM_NO_SENSE); > +} > EXPORT_SYMBOL(target_complete_cmd); > > +void target_complete_cmd_with_sense(struct se_cmd *cmd, > + sense_reason_t sense_reason) > +{ > + __target_complete_cmd(cmd, SAM_STAT_CHECK_CONDITION, sense_reason); > +} > +EXPORT_SYMBOL(target_complete_cmd_with_sense); > + > void target_complete_cmd_with_length(struct se_cmd *cmd, u8 scsi_status, int length) > { > if (scsi_status == SAM_STAT_GOOD && length < cmd->data_length) { > @@ -1650,6 +1664,7 @@ void transport_generic_request_failure(struct se_cmd *cmd, > case TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED: > case TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED: > case TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED: > + case TCM_MISCOMPARE_VERIFY: > break; > case TCM_OUT_OF_RESOURCES: > sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; > @@ -2633,12 +2648,19 @@ void transport_err_sector_info(unsigned char *buffer, sector_t bad_sector) > buffer[SPC_ADD_SENSE_LEN_OFFSET] = 0xc; > buffer[SPC_DESC_TYPE_OFFSET] = 0; /* Information */ > buffer[SPC_ADDITIONAL_DESC_LEN_OFFSET] = 0xa; > - buffer[SPC_VALIDITY_OFFSET] = 0x80; > + buffer[SPC_CMD_INFO_VALIDITY_OFFSET] = 0x80; > > /* Descriptor Information: failing sector */ > put_unaligned_be64(bad_sector, &buffer[12]); > } > > +static void transport_err_sense_info(unsigned char *buffer, u32 info) > +{ > + buffer[SPC_INFO_VALIDITY_OFFSET] |= 0x80; > + /* Sense Information */ > + put_unaligned_be32(info, &buffer[3]); > +} > + > int > transport_send_check_condition_and_sense(struct se_cmd *cmd, > sense_reason_t reason, int from_transport) > @@ -2831,6 +2853,7 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd, > /* MISCOMPARE DURING VERIFY OPERATION */ > buffer[SPC_ASC_KEY_OFFSET] = 0x1d; > buffer[SPC_ASCQ_KEY_OFFSET] = 0x00; > + transport_err_sense_info(buffer, cmd->sense_info); > break; > case TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED: > /* CURRENT ERROR */ > diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h > index ec5a09f..c98f6e6 100644 > --- a/include/target/target_core_backend.h > +++ b/include/target/target_core_backend.h > @@ -58,6 +58,7 @@ int transport_backend_register(const struct target_backend_ops *); > void target_backend_unregister(const struct target_backend_ops *); > > void target_complete_cmd(struct se_cmd *, u8); > +void target_complete_cmd_with_sense(struct se_cmd *, sense_reason_t); > void target_complete_cmd_with_length(struct se_cmd *, u8, int); > > sense_reason_t spc_parse_cdb(struct se_cmd *cmd, unsigned int *size); > diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h > index 17ae2d6..b83a8ec 100644 > --- a/include/target/target_core_base.h > +++ b/include/target/target_core_base.h > @@ -22,11 +22,12 @@ > */ > #define TRANSPORT_SENSE_BUFFER 96 > /* Used by transport_send_check_condition_and_sense() */ > +#define SPC_INFO_VALIDITY_OFFSET 0 > #define SPC_SENSE_KEY_OFFSET 2 > #define SPC_ADD_SENSE_LEN_OFFSET 7 > #define SPC_DESC_TYPE_OFFSET 8 > #define SPC_ADDITIONAL_DESC_LEN_OFFSET 9 > -#define SPC_VALIDITY_OFFSET 10 > +#define SPC_CMD_INFO_VALIDITY_OFFSET 10 > #define SPC_ASC_KEY_OFFSET 12 > #define SPC_ASCQ_KEY_OFFSET 13 > #define TRANSPORT_IQN_LEN 224 > @@ -439,6 +440,8 @@ struct se_dif_v1_tuple { > #define TCM_ACA_TAG 0x24 > > struct se_cmd { > + sense_reason_t sense_reason; > + u32 sense_info; > /* SAM response code being sent to initiator */ > u8 scsi_status; > u8 scsi_asc; > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/04/2015 12:41 PM, Mike Christie wrote: > Hey Nick and Christoph, > > For the target_core_iblock + rbd/COMPARE_AND_WRITE support approach, I > still need a patch like below to be able to allow target_core_iblock.c > to be able to specify specific codes like TCM_MISCOMPARE_VERIFY. > > Is the patch below ok? I made it work similar to how we have other > completion functions, target_complete_cmd_with_*, that take in extra > amounts of info like length or sense. > > Instead, I can modify target_complete_cmd and all the callers so they > just take/pass the sense and info if needed: > > void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status, > sense_reason_t sense, u32 info) We also want user/pscsi passthrough to be able to return correct sense info, and your patch would let them do this. -- Andy -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9/4/2015 10:41 PM, Mike Christie wrote: > Hey Nick and Christoph, Hi Mike, > > For the target_core_iblock + rbd/COMPARE_AND_WRITE support approach, I > still need a patch like below to be able to allow target_core_iblock.c > to be able to specify specific codes like TCM_MISCOMPARE_VERIFY. I just posted a couple of patches to use scsi helpers to set the sense buffer with/without information (see translate_sense_reason). You can just add TCM_MISCOMPARE_VERIFY entry to sense_info_table with .add_sector_info = true. Moreover, we have a field in the command structure to keep the error sector (used in DIF). So you should go ahead and use that. Sagi. -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 29, 2015 at 04:23:49AM -0500, mchristi@redhat.com wrote: > From: Mike Christie <michaelc@cs.wisc.edu> > > Currently, backend drivers seem to only fail IO with > SAM_STAT_CHECK_CONDITION which gets us > TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE. > For compare and write support we will want to be able to fail with > TCM_MISCOMPARE_VERIFY. This patch adds a new helper that allows backend > drivers to fail with specific sense codes. > > It also allows the backend driver to set the miscompare offset. I agree that we should allwo for better passing of sense data, but I also think we need to redo the sense handling instead of adding more warts. One premise is that with various updates to the standards it will become more common to generate sense data even if we did not fail the whole command, so this might be a good opportunity to preparate for that. > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index ce8574b..f9b0527 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -639,8 +639,7 @@ static void target_complete_failure_work(struct work_struct *work) > { > struct se_cmd *cmd = container_of(work, struct se_cmd, work); > > - transport_generic_request_failure(cmd, > - TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE); > + transport_generic_request_failure(cmd, cmd->sense_reason); > } So I think we should merge target_complete_failure_work and target_complete_ok_work as a first step. Then as a second do away with transport_generic_request_failure and just have single target_complete_cmd that will return success or error based on the scsi_status field an generate sense if cmd->sense_reason is set. Third we should replace SCF_TRANSPORT_TASK_SENSE and SCF_EMULATED_TASK_SENSE with a single driver visible flag and instead have a new TCM_PASSTHROUGH_SENSE sense code to not generate new sense data if pscsi passed on sense data. > struct se_cmd { > + sense_reason_t sense_reason; At this point you should probably also remove the sense_reason from the iscsi_cmd now that it's in the generic CMD. -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index ce8574b..f9b0527 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -639,8 +639,7 @@ static void target_complete_failure_work(struct work_struct *work) { struct se_cmd *cmd = container_of(work, struct se_cmd, work); - transport_generic_request_failure(cmd, - TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE); + transport_generic_request_failure(cmd, cmd->sense_reason); } /* @@ -666,14 +665,15 @@ static unsigned char *transport_get_sense_buffer(struct se_cmd *cmd) return cmd->sense_buffer; } -void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) +static void __target_complete_cmd(struct se_cmd *cmd, u8 scsi_status, + sense_reason_t sense_reason) { struct se_device *dev = cmd->se_dev; int success = scsi_status == GOOD; unsigned long flags; cmd->scsi_status = scsi_status; - + cmd->sense_reason = sense_reason; spin_lock_irqsave(&cmd->t_state_lock, flags); cmd->transport_state &= ~CMD_T_BUSY; @@ -716,8 +716,22 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) queue_work(target_completion_wq, &cmd->work); } + +void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) +{ + __target_complete_cmd(cmd, scsi_status, scsi_status ? + TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE : + TCM_NO_SENSE); +} EXPORT_SYMBOL(target_complete_cmd); +void target_complete_cmd_with_sense(struct se_cmd *cmd, + sense_reason_t sense_reason) +{ + __target_complete_cmd(cmd, SAM_STAT_CHECK_CONDITION, sense_reason); +} +EXPORT_SYMBOL(target_complete_cmd_with_sense); + void target_complete_cmd_with_length(struct se_cmd *cmd, u8 scsi_status, int length) { if (scsi_status == SAM_STAT_GOOD && length < cmd->data_length) { @@ -1650,6 +1664,7 @@ void transport_generic_request_failure(struct se_cmd *cmd, case TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED: case TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED: case TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED: + case TCM_MISCOMPARE_VERIFY: break; case TCM_OUT_OF_RESOURCES: sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; @@ -2633,12 +2648,19 @@ void transport_err_sector_info(unsigned char *buffer, sector_t bad_sector) buffer[SPC_ADD_SENSE_LEN_OFFSET] = 0xc; buffer[SPC_DESC_TYPE_OFFSET] = 0; /* Information */ buffer[SPC_ADDITIONAL_DESC_LEN_OFFSET] = 0xa; - buffer[SPC_VALIDITY_OFFSET] = 0x80; + buffer[SPC_CMD_INFO_VALIDITY_OFFSET] = 0x80; /* Descriptor Information: failing sector */ put_unaligned_be64(bad_sector, &buffer[12]); } +static void transport_err_sense_info(unsigned char *buffer, u32 info) +{ + buffer[SPC_INFO_VALIDITY_OFFSET] |= 0x80; + /* Sense Information */ + put_unaligned_be32(info, &buffer[3]); +} + int transport_send_check_condition_and_sense(struct se_cmd *cmd, sense_reason_t reason, int from_transport) @@ -2831,6 +2853,7 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd, /* MISCOMPARE DURING VERIFY OPERATION */ buffer[SPC_ASC_KEY_OFFSET] = 0x1d; buffer[SPC_ASCQ_KEY_OFFSET] = 0x00; + transport_err_sense_info(buffer, cmd->sense_info); break; case TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED: /* CURRENT ERROR */ diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h index ec5a09f..c98f6e6 100644 --- a/include/target/target_core_backend.h +++ b/include/target/target_core_backend.h @@ -58,6 +58,7 @@ int transport_backend_register(const struct target_backend_ops *); void target_backend_unregister(const struct target_backend_ops *); void target_complete_cmd(struct se_cmd *, u8); +void target_complete_cmd_with_sense(struct se_cmd *, sense_reason_t); void target_complete_cmd_with_length(struct se_cmd *, u8, int); sense_reason_t spc_parse_cdb(struct se_cmd *cmd, unsigned int *size); diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index 17ae2d6..b83a8ec 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -22,11 +22,12 @@ */ #define TRANSPORT_SENSE_BUFFER 96 /* Used by transport_send_check_condition_and_sense() */ +#define SPC_INFO_VALIDITY_OFFSET 0 #define SPC_SENSE_KEY_OFFSET 2 #define SPC_ADD_SENSE_LEN_OFFSET 7 #define SPC_DESC_TYPE_OFFSET 8 #define SPC_ADDITIONAL_DESC_LEN_OFFSET 9 -#define SPC_VALIDITY_OFFSET 10 +#define SPC_CMD_INFO_VALIDITY_OFFSET 10 #define SPC_ASC_KEY_OFFSET 12 #define SPC_ASCQ_KEY_OFFSET 13 #define TRANSPORT_IQN_LEN 224 @@ -439,6 +440,8 @@ struct se_dif_v1_tuple { #define TCM_ACA_TAG 0x24 struct se_cmd { + sense_reason_t sense_reason; + u32 sense_info; /* SAM response code being sent to initiator */ u8 scsi_status; u8 scsi_asc;