mbox series

[0/1] scsi: target: core: Fix sense key for invalid XCOPY request

Message ID 20210624111926.63176-1-s.samoylenko@yadro.com (mailing list archive)
Headers show
Series scsi: target: core: Fix sense key for invalid XCOPY request | expand

Message

Sergey Samoylenko June 24, 2021, 11:19 a.m. UTC 
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 (1):
  scsi: target: core: Fix sense key for invalid XCOPY request

 drivers/target/target_core_xcopy.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

Comments

David Disseldorp July 21, 2021, 9:45 p.m. UTC | #1 
Hi Sergey,

On Thu, 24 Jun 2021 14:19:25 +0300, Sergey Samoylenko wrote:

> 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:

We've hit this too. The incorrect sense reporting appears to also affect
VMware XCOPY fallback to initiator driven READ/WRITE. I'm pretty sure
this is a regression from d877d7275be34ad70ce92bcbb4bb36cec77ed004, so
should probably be marked as such via a Fixes tag.

Cheers, David
Sergey Samoylenko July 22, 2021, 11:03 a.m. UTC | #2 
Hi David,

> Hi Sergey,
>
> On Thu, 24 Jun 2021 14:19:25 +0300, Sergey Samoylenko wrote:
>
>> 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:
>
> We've hit this too. The incorrect sense reporting appears to also affect VMware XCOPY fallback to initiator driven READ/WRITE. I'm pretty sure this is a regression from
> d877d7275be34ad70ce92bcbb4bb36cec77ed004, so should probably be marked as such via a Fixes tag.
>
> Cheers, David

The d877d7275be34ad70ce92bcbb4bb36cec77ed004 was added for v4.10.x kernel and it was necessary
for to avoid LUN removal race conditions. Later you excluded using configfs in the XCOPY workqueue.
It was the 2896c93811e39d63a4d9b63ccf12a8fbc226e5e4.

If we remove the d877d7275be34ad70ce92bcbb4bb36cec77ed004, will it break anything?
We have accumulated many changes between v4.10 and v5.14.

David, maybe can we move the helper 'target_complete_cmd_with_sense' from your path to mainline kernel?
I think it will be useful in the future.

Best regards,
Sergey



From 2e96d8ac2695a13edf71976bd099003dda52056d Mon Sep 17 00:00:00 2001
From: Mike Christie <michaelc@cs.wisc.edu>
Date: Wed, 29 Jul 2015 04:23:49 -0500
Subject: [PATCH] target: compare and write backend driver sense handling
References: fate#318836
Patch-mainline: Not yet, SES2 clustered LIO/RBD

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>
Acked-by: David Disseldorp <ddiss@suse.de>
[ddiss@suse.de rebase against ab78fef4d5 and 9ec1e1ce3a]
---
 drivers/target/target_core_transport.c |   34 ++++++++++++++++++++++++++++++---
 include/target/target_core_backend.h   |    1
 include/target/target_core_base.h      |    5 +++-
 3 files changed, 36 insertions(+), 4 deletions(-)

--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -718,8 +718,7 @@ static void target_complete_failure_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);
 }

 /*
@@ -837,7 +836,8 @@ static bool target_cmd_interrupted(struc
 }

 /* 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)
 {
  int success;
  unsigned long flags;
@@ -846,6 +846,7 @@ void target_complete_cmd(struct se_cmd *
    return;

  cmd->scsi_status = scsi_status;
+ cmd->sense_reason = sense_reason;

  spin_lock_irqsave(&cmd->t_state_lock, flags);
  switch (cmd->scsi_status) {
@@ -871,8 +872,22 @@ void target_complete_cmd(struct se_cmd *
  else
    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_set_cmd_data_length(struct se_cmd *cmd, int length)
 {
  if (length < cmd->data_length) {
@@ -1917,6 +1932,7 @@ void transport_generic_request_failure(s
  case TCM_UNSUPPORTED_TARGET_DESC_TYPE_CODE:
  case TCM_TOO_MANY_SEGMENT_DESCS:
  case TCM_UNSUPPORTED_SEGMENT_DESC_TYPE_CODE:
+ case TCM_MISCOMPARE_VERIFY:
    break;
  case TCM_OUT_OF_RESOURCES:
    cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
@@ -3101,11 +3117,13 @@ bool transport_wait_for_tasks(struct se_
 }
 EXPORT_SYMBOL(transport_wait_for_tasks);

+
 struct sense_info {
  u8 key;
  u8 asc;
  u8 ascq;
  bool add_sector_info;
+ bool add_sense_info;
 };

 static const struct sense_info sense_info_table[] = {
@@ -3203,6 +3221,7 @@ static const struct sense_info sense_inf
    .key = MISCOMPARE,
    .asc = 0x1d, /* MISCOMPARE DURING VERIFY OPERATION */
    .ascq = 0x00,
+   .add_sense_info = true,
  },
  [TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED] = {
    .key = ABORTED_COMMAND,
@@ -3255,6 +3274,13 @@ static const struct sense_info sense_inf
  },
 };

+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]);
+}
+
 /**
  * translate_sense_reason - translate a sense reason into T10 key, asc and ascq
  * @cmd: SCSI command in which the resulting sense buffer or SCSI status will
@@ -3304,6 +3330,8 @@ static void translate_sense_reason(struc
    WARN_ON_ONCE(scsi_set_sense_information(buffer,
              cmd->scsi_sense_length,
              cmd->bad_sector) < 0);
+ if (si->add_sense_info)
+   transport_err_sense_info(buffer, cmd->sense_info);
 }

 int
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -74,6 +74,7 @@ void  target_backend_unregister(const str

 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 *);
--- 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
@@ -452,6 +453,8 @@ enum target_core_dif_check {
 #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;
David Disseldorp July 22, 2021, 1:27 p.m. UTC | #3 
On Thu, 22 Jul 2021 11:03:02 +0000, Sergey Samoylenko wrote:

> Hi David,
> 
> > Hi Sergey,
> >
> > On Thu, 24 Jun 2021 14:19:25 +0300, Sergey Samoylenko wrote:
> >  
> >> 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:  
> >
> > We've hit this too. The incorrect sense reporting appears to also affect VMware XCOPY fallback to initiator driven READ/WRITE. I'm pretty sure this is a regression from
> > d877d7275be34ad70ce92bcbb4bb36cec77ed004, so should probably be marked as such via a Fixes tag.
> >
> > Cheers, David  
> 
> The d877d7275be34ad70ce92bcbb4bb36cec77ed004 was added for v4.10.x kernel and it was necessary
> for to avoid LUN removal race conditions. Later you excluded using configfs in the XCOPY workqueue.
> It was the 2896c93811e39d63a4d9b63ccf12a8fbc226e5e4.
> 
> If we remove the d877d7275be34ad70ce92bcbb4bb36cec77ed004, will it break anything?
> We have accumulated many changes between v4.10 and v5.14.
> 
> David, maybe can we move the helper 'target_complete_cmd_with_sense' from your path to mainline kernel?
> I think it will be useful in the future.

I don't think it makes sense to revert d877d7275be34. I agree that
Mike's target_complete_cmd_with_sense() patch should be helpful for
proper sense propagation here.

Cheers, David