* [v2 0/2] target: core: Fix sense key for invalid XCOPY request @ 2021-07-26 15:16 Sergey Samoylenko 2021-07-26 15:16 ` [v2 1/2] target: allows backend drivers to fail with specific sense codes Sergey Samoylenko 2021-07-26 15:16 ` [v2 2/2] target: core: Fix sense key for invalid XCOPY request Sergey Samoylenko 0 siblings, 2 replies; 6+ messages in thread From: Sergey Samoylenko @ 2021-07-26 15:16 UTC (permalink / raw) To: martin.petersen, michael.christie, ddiss, bvanassche, target-devel Cc: linux-scsi, linux, Sergey Samoylenko EXTENDED COPY tests in libiscsi [1] show that TCM doesn't follow SPC4 when detects invalid parameters in a XCOPY command or IO errors. The replies from TCM contain wrong sense key or ASCQ for incorrect request. The series fixes the following tests from libiscsi: SCSI.ExtendedCopy.DescrType SCSI.ExtendedCopy.DescrLimits SCSI.ExtendedCopy.ParamHdr SCSI.ExtendedCopy.ValidSegDescr SCSI.ExtendedCopy.ValidTgtDescr 1. https://github.com/sahlberg/libiscsi Sergey Samoylenko (2): target: allows backend drivers to fail with specific sense codes target: core: Fix sense key for invalid XCOPY request drivers/target/target_core_transport.c | 21 ++++++++++++++++++--- drivers/target/target_core_xcopy.c | 26 +++++++++++++++----------- include/target/target_core_backend.h | 1 + include/target/target_core_base.h | 2 ++ 4 files changed, 36 insertions(+), 14 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [v2 1/2] target: allows backend drivers to fail with specific sense codes 2021-07-26 15:16 [v2 0/2] target: core: Fix sense key for invalid XCOPY request Sergey Samoylenko @ 2021-07-26 15:16 ` Sergey Samoylenko 2021-07-30 16:37 ` David Disseldorp 2021-07-26 15:16 ` [v2 2/2] target: core: Fix sense key for invalid XCOPY request Sergey Samoylenko 1 sibling, 1 reply; 6+ messages in thread From: Sergey Samoylenko @ 2021-07-26 15:16 UTC (permalink / raw) To: martin.petersen, michael.christie, ddiss, bvanassche, target-devel Cc: linux-scsi, linux, Sergey Samoylenko Currently, backend drivers can fail IO with SAM_STAT_CHECK_CONDITION which gets us TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE. The patch adds a new helper that allows backend drivers to fail with specific sense codes. This is based on a patch from Mike Christie <michael.christie@oracle.com>. Cc: Mike Christie <michael.christie@oracle.com> Cc: David Disseldorp <ddiss@suse.de> [ Moved target_complete_cmd_with_sense() helper to mainline ] Signed-off-by: Sergey Samoylenko <s.samoylenko@yadro.com> --- drivers/target/target_core_transport.c | 21 ++++++++++++++++++--- include/target/target_core_backend.h | 1 + include/target/target_core_base.h | 2 ++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 26ceabe34de5..d2a2892bda9c 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -736,8 +736,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); } /* @@ -855,7 +854,8 @@ static bool target_cmd_interrupted(struct se_cmd *cmd) } /* May be called from interrupt context so must not sleep. */ -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_wwn *wwn = cmd->se_sess->se_tpg->se_tpg_wwn; int success, cpu; @@ -865,6 +865,7 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) return; cmd->scsi_status = scsi_status; + cmd->sense_reason = sense_reason; spin_lock_irqsave(&cmd->t_state_lock, flags); switch (cmd->scsi_status) { @@ -893,8 +894,22 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) queue_work_on(cpu, 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_set_cmd_data_length(struct se_cmd *cmd, int length) { if (length < cmd->data_length) { diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h index 1f78b09bba55..3cc50d30231a 100644 --- a/include/target/target_core_backend.h +++ b/include/target/target_core_backend.h @@ -75,6 +75,7 @@ void target_backend_unregister(const struct target_backend_ops *); void target_complete_cmd(struct se_cmd *, u8); void target_set_cmd_data_length(struct se_cmd *, int); +void target_complete_cmd_with_sense(struct se_cmd *, sense_reason_t); void target_complete_cmd_with_length(struct se_cmd *, u8, int); void transport_copy_sense_to_cmd(struct se_cmd *, unsigned char *); diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index 85c16c266eac..8c85d3b83a70 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -453,6 +453,8 @@ enum target_core_dif_check { #define TCM_ACA_TAG 0x24 struct se_cmd { + /* Used for fail with specific sense codes */ + sense_reason_t sense_reason; /* SAM response code being sent to initiator */ u8 scsi_status; u8 scsi_asc; -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [v2 1/2] target: allows backend drivers to fail with specific sense codes 2021-07-26 15:16 ` [v2 1/2] target: allows backend drivers to fail with specific sense codes Sergey Samoylenko @ 2021-07-30 16:37 ` David Disseldorp 2021-08-02 18:31 ` Sergey Samoylenko 0 siblings, 1 reply; 6+ messages in thread From: David Disseldorp @ 2021-07-30 16:37 UTC (permalink / raw) To: Sergey Samoylenko Cc: martin.petersen, michael.christie, bvanassche, target-devel, linux-scsi, linux Hi Sergey, On Mon, 26 Jul 2021 18:16:45 +0300, Sergey Samoylenko wrote: > Currently, backend drivers can fail IO with > SAM_STAT_CHECK_CONDITION which gets us > TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE. The patch adds > a new helper that allows backend drivers to fail with > specific sense codes. > > This is based on a patch from Mike Christie <michael.christie@oracle.com>. This looks good and works for me, but I have one comment... > Cc: Mike Christie <michael.christie@oracle.com> > Cc: David Disseldorp <ddiss@suse.de> > [ Moved target_complete_cmd_with_sense() helper to mainline ] > Signed-off-by: Sergey Samoylenko <s.samoylenko@yadro.com> > --- > drivers/target/target_core_transport.c | 21 ++++++++++++++++++--- > include/target/target_core_backend.h | 1 + > include/target/target_core_base.h | 2 ++ > 3 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 26ceabe34de5..d2a2892bda9c 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -736,8 +736,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); > } > > /* > @@ -855,7 +854,8 @@ static bool target_cmd_interrupted(struct se_cmd *cmd) > } > > /* May be called from interrupt context so must not sleep. */ > -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_wwn *wwn = cmd->se_sess->se_tpg->se_tpg_wwn; > int success, cpu; > @@ -865,6 +865,7 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) > return; > > cmd->scsi_status = scsi_status; > + cmd->sense_reason = sense_reason; > > spin_lock_irqsave(&cmd->t_state_lock, flags); > switch (cmd->scsi_status) { > @@ -893,8 +894,22 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) > > queue_work_on(cpu, 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); > + It's a little unclear from the function prototype that this actually fails the command with SAM_STAT_CHECK_CONDITION. I could imagine people erroneously calling target_complete_cmd_with_sense(cmd, TCM_NO_SENSE) and expecting success. I think it might be a bit clearer if you just export __target_complete_cmd() as target_complete_cmd_with_sense() with all three parameters and leave it up to the caller to flag CHECK_CONDITION. Cheers, David ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [v2 1/2] target: allows backend drivers to fail with specific sense codes 2021-07-30 16:37 ` David Disseldorp @ 2021-08-02 18:31 ` Sergey Samoylenko 2021-08-03 12:33 ` David Disseldorp 0 siblings, 1 reply; 6+ messages in thread From: Sergey Samoylenko @ 2021-08-02 18:31 UTC (permalink / raw) To: David Disseldorp Cc: martin.petersen, michael.christie, bvanassche, target-devel, linux-scsi, linux Hi David, Sorry for the long answer. > Hi Sergey, > > On Mon, 26 Jul 2021 18:16:45 +0300, Sergey Samoylenko wrote: > >> Currently, backend drivers can fail IO with >> SAM_STAT_CHECK_CONDITION which gets us >> TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE. The patch adds >> a new helper that allows backend drivers to fail with >> specific sense codes. >> >> This is based on a patch from Mike Christie <michael.christie@oracle.com>. > > This looks good and works for me, but I have one comment... > > It's a little unclear from the function prototype that this actually > fails the command with SAM_STAT_CHECK_CONDITION. I could imagine people > erroneously calling target_complete_cmd_with_sense(cmd, TCM_NO_SENSE) > and expecting success. > I think it might be a bit clearer if you just export > __target_complete_cmd() as target_complete_cmd_with_sense() with all > three parameters and leave it up to the caller to flag > CHECK_CONDITION. > > Cheers, David David, am I getting the idea right? We want to get something like this: ----------------------------------------------------------------------------------- diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 7e35eddd9eb7..6dbfba7f16a6 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -736,8 +736,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); } /* @@ -855,7 +854,8 @@ static bool target_cmd_interrupted(struct se_cmd *cmd) } /* May be called from interrupt context so must not sleep. */ -void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) +void target_complete_cmd_with_sense(struct se_cmd *cmd, u8 scsi_status, + sense_reason_t sense_reason) { struct se_wwn *wwn = cmd->se_sess->se_tpg->se_tpg_wwn; int success, cpu; @@ -865,6 +865,7 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) return; cmd->scsi_status = scsi_status; + cmd->sense_reason = sense_reason; spin_lock_irqsave(&cmd->t_state_lock, flags); switch (cmd->scsi_status) { @@ -893,6 +894,14 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) queue_work_on(cpu, target_completion_wq, &cmd->work); } +EXPORT_SYMBOL(target_complete_cmd_with_sense); + +void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) +{ + target_complete_cmd_with_sense(cmd, scsi_status, scsi_status ? + TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE : + TCM_NO_SENSE); +} EXPORT_SYMBOL(target_complete_cmd); void target_set_cmd_data_length(struct se_cmd *cmd, int length) ----------------------------------------------------------------------------------- Then we use it as follows: ----------------------------------------------------------------------------------- diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c index 0f1319336f3e..1b4faafedb1a 100644 --- a/drivers/target/target_core_xcopy.c +++ b/drivers/target/target_core_xcopy.c @@ -674,12 +674,16 @@ static void target_xcopy_do_work(struct work_struct *work) ... err_free: kfree(xop); - /* - * Don't override an error scsi status if it has already been set - */ - if (ec_cmd->scsi_status == SAM_STAT_GOOD) { - pr_warn_ratelimited("target_xcopy_do_work: rc: %d, Setting X-COPY" - " CHECK_CONDITION -> sending response\n", rc); - ec_cmd->scsi_status = SAM_STAT_CHECK_CONDITION; - } - target_complete_cmd(ec_cmd, ec_cmd->scsi_status); + pr_warn_ratelimited("target_xcopy_do_work: rc: %d, sense: %u," + " XCOPY operation failed\n", rc, sense_rc); + target_complete_cmd_with_sense(ec_cmd, SAM_STAT_CHECK_CONDITION, sense_rc); } ----------------------------------------------------------------------------------- Best regards, Sergey ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [v2 1/2] target: allows backend drivers to fail with specific sense codes 2021-08-02 18:31 ` Sergey Samoylenko @ 2021-08-03 12:33 ` David Disseldorp 0 siblings, 0 replies; 6+ messages in thread From: David Disseldorp @ 2021-08-03 12:33 UTC (permalink / raw) To: Sergey Samoylenko Cc: martin.petersen, michael.christie, bvanassche, target-devel, linux-scsi, linux Hi Sergey, On Mon, 2 Aug 2021 18:31:06 +0000, Sergey Samoylenko wrote: > David, am I getting the idea right? > > We want to get something like this: [trimmed] Yes, this is my preference (with the corresponding .h changes). Please send it as a git-send-email patchset and feel free to add my reviewed-by tag. Cheers, David ^ permalink raw reply [flat|nested] 6+ messages in thread
* [v2 2/2] target: core: Fix sense key for invalid XCOPY request 2021-07-26 15:16 [v2 0/2] target: core: Fix sense key for invalid XCOPY request Sergey Samoylenko 2021-07-26 15:16 ` [v2 1/2] target: allows backend drivers to fail with specific sense codes Sergey Samoylenko @ 2021-07-26 15:16 ` Sergey Samoylenko 1 sibling, 0 replies; 6+ messages in thread From: Sergey Samoylenko @ 2021-07-26 15:16 UTC (permalink / raw) To: martin.petersen, michael.christie, ddiss, bvanassche, target-devel Cc: linux-scsi, linux, Sergey Samoylenko, Roman Bolshakov, Konstantin Shelekhin TCM fails to pass the following tests in libiscsi: SCSI.ExtendedCopy.DescrType SCSI.ExtendedCopy.DescrLimits SCSI.ExtendedCopy.ParamHdr SCSI.ExtendedCopy.ValidSegDescr SCSI.ExtendedCopy.ValidTgtDescr XCOPY always returns the same NOT READY sense key for all detected errors. It changes a sense key for invalid requests to ILLEGAL REQUEST sense key, for aborted transferring data (IO error and etc.) to COPY ABORTED. Fixes: d877d7275be34ad ("target: Fix a deadlock between the XCOPY code and iSCSI session shutdown") Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com> Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com> Signed-off-by: Sergey Samoylenko <s.samoylenko@yadro.com> --- drivers/target/target_core_xcopy.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c index 0f1319336f3e..64baf3e8c079 100644 --- a/drivers/target/target_core_xcopy.c +++ b/drivers/target/target_core_xcopy.c @@ -674,12 +674,16 @@ static void target_xcopy_do_work(struct work_struct *work) unsigned int max_sectors; int rc = 0; unsigned short nolb, max_nolb, copied_nolb = 0; + sense_reason_t sense_rc; - if (target_parse_xcopy_cmd(xop) != TCM_NO_SENSE) + sense_rc = target_parse_xcopy_cmd(xop); + if (sense_rc != TCM_NO_SENSE) goto err_free; - if (WARN_ON_ONCE(!xop->src_dev) || WARN_ON_ONCE(!xop->dst_dev)) + if (WARN_ON_ONCE(!xop->src_dev) || WARN_ON_ONCE(!xop->dst_dev)) { + sense_rc = TCM_INVALID_PARAMETER_LIST; goto err_free; + } src_dev = xop->src_dev; dst_dev = xop->dst_dev; @@ -762,20 +766,20 @@ static void target_xcopy_do_work(struct work_struct *work) return; out: + /* + * The XCOPY command was aborted after some data was transferred. + * Terminate command with CHECK CONDITION status, with the sense key + * set to COPY ABORTED. + */ + sense_rc = TCM_COPY_TARGET_DEVICE_NOT_REACHABLE; xcopy_pt_undepend_remotedev(xop); target_free_sgl(xop->xop_data_sg, xop->xop_data_nents); err_free: kfree(xop); - /* - * Don't override an error scsi status if it has already been set - */ - if (ec_cmd->scsi_status == SAM_STAT_GOOD) { - pr_warn_ratelimited("target_xcopy_do_work: rc: %d, Setting X-COPY" - " CHECK_CONDITION -> sending response\n", rc); - ec_cmd->scsi_status = SAM_STAT_CHECK_CONDITION; - } - target_complete_cmd(ec_cmd, ec_cmd->scsi_status); + pr_warn_ratelimited("target_xcopy_do_work: rc: %d, sense: %u," + " XCOPY operation failed\n", rc, sense_rc); + target_complete_cmd_with_sense(ec_cmd, sense_rc); } /* -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-03 12:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-26 15:16 [v2 0/2] target: core: Fix sense key for invalid XCOPY request Sergey Samoylenko 2021-07-26 15:16 ` [v2 1/2] target: allows backend drivers to fail with specific sense codes Sergey Samoylenko 2021-07-30 16:37 ` David Disseldorp 2021-08-02 18:31 ` Sergey Samoylenko 2021-08-03 12:33 ` David Disseldorp 2021-07-26 15:16 ` [v2 2/2] target: core: Fix sense key for invalid XCOPY request Sergey Samoylenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.