[12/18] target: compare and write backend driver sense handling
diff mbox

Message ID 1438161835-27960-12-git-send-email-mchristi@redhat.com
State New
Headers show

Commit Message

Mike Christie July 29, 2015, 9:23 a.m. UTC
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(-)

Comments

Mike Christie Sept. 4, 2015, 7:41 p.m. UTC | #1
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
Andy Grover Sept. 4, 2015, 10:34 p.m. UTC | #2
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
Sagi Grimberg Sept. 6, 2015, 6:38 a.m. UTC | #3
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
Christoph Hellwig Sept. 6, 2015, 7:12 a.m. UTC | #4
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

Patch
diff mbox

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;