diff mbox

[RESEND] TCMUser: add read length support

Message ID 5b06ed25.3Xi5BduJFP9OUO6V%bstroesser@ts.fujitsu.com (mailing list archive)
State Accepted
Headers show

Commit Message

Bodo Stroesser May 24, 2018, 4:49 p.m. UTC 
This is patch version 3.

Generally target core and TCMUser seem to work fine for tape devices and
media changers.
But there is at least one situation, where TCMUser is not able to support
sequential access device emulation correctly.

The situation is when an initiator sends a SCSI READ CDB with a length that is
greater than the length of the tape block to read. We can distinguish two
subcases:

A) The initiator sent the READ CDB with the SILI bit being set.
   In this case the sequential access device has to transfer the data from the
   tape block (only the length of the tape block) and transmit a good status.
   The current interface between TCMUser and the userspace does not support
   reduction of the read data size by the userspace program.

   The patch below fixes this subcase by allowing the userspace program to
   specify a reduced data size in read direction.

B) The initiator sent the READ CDB with the SILI bit not being set.
   In this case the sequential access device has to transfer the data from the
   tape block as in A), but additionally has to transmit CHECK CONDITION with
   the ILI bit set and NO SENSE in the sensebytes. The information field in
   the sensebytes must contain the residual count.

   With the below patch a user space program can specify the real read data
   length and appropriate sensebytes.
   TCMUser then uses the se_cmd flag SCF_TREAT_READ_AS_NORMAL, to force target
   core to transmit the real data size and the sensebytes.
   Note: the flag SCF_TREAT_READ_AS_NORMAL is introduced by Lee Duncan's
   patch "[PATCH v4] target: transport should handle st FM/EOM/ILI reads" from
   Tue, 15 May 2018 18:25:24 -0700.

This patch was created for kernel 4.15.9.


Changes from RFC:
- patch now uses SCF_TREAT_READ_AS_NORMAL to fix B) also.
- comment changed accordingly

Changes from V2:
- Cleaned up code according to review
- Userspace can set read_len for data in only, not for bidi.
- read_len from userspace no longer is checked implicitly by
  gather_data_area(), but the check is done explicitly
  in tcmu_handle_completion(). Now code is easier to understand.
  

Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
Acked-by: Mike Christie <mchristi@redhat.com>

---
 include/uapi/linux/target_core_user.h |    4 ++-
 drivers/target/target_core_user.c     |   44 +++++++++++++++++++++++++++-------
 2 files changed, 39 insertions(+), 9 deletions(-)

Comments

kernel test robot May 26, 2018, 5:14 p.m. UTC | #1 
Hi Bodo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.17-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Bodo-Stroesser/TCMUser-add-read-length-support/20180526-231412
config: x86_64-randconfig-x003-201820 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers//target/target_core_user.c: In function 'tcmu_handle_completion':
>> drivers//target/target_core_user.c:1077:28: error: 'SCF_TREAT_READ_AS_NORMAL' undeclared (first use in this function)
       se_cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
                               ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers//target/target_core_user.c:1077:28: note: each undeclared identifier is reported only once for each function it appears in

vim +/SCF_TREAT_READ_AS_NORMAL +1077 drivers//target/target_core_user.c

  1041	
  1042	static void tcmu_handle_completion(struct tcmu_cmd *cmd, struct tcmu_cmd_entry *entry)
  1043	{
  1044		struct se_cmd *se_cmd = cmd->se_cmd;
  1045		struct tcmu_dev *udev = cmd->tcmu_dev;
  1046		bool read_len_valid = false;
  1047		uint32_t read_len = se_cmd->data_length;
  1048	
  1049		/*
  1050		 * cmd has been completed already from timeout, just reclaim
  1051		 * data area space and free cmd
  1052		 */
  1053		if (test_bit(TCMU_CMD_BIT_EXPIRED, &cmd->flags))
  1054			goto out;
  1055	
  1056		tcmu_cmd_reset_dbi_cur(cmd);
  1057	
  1058		if (entry->hdr.uflags & TCMU_UFLAG_UNKNOWN_OP) {
  1059			pr_warn("TCMU: Userspace set UNKNOWN_OP flag on se_cmd %p\n",
  1060				cmd->se_cmd);
  1061			entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
  1062			goto done;
  1063		}
  1064	
  1065		if (se_cmd->data_direction == DMA_FROM_DEVICE &&
  1066		    (entry->hdr.uflags & TCMU_UFLAG_READ_LEN) && entry->rsp.read_len) {
  1067			read_len_valid = true;
  1068			if (entry->rsp.read_len < read_len)
  1069				read_len = entry->rsp.read_len;
  1070		}
  1071	
  1072		if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
  1073			transport_copy_sense_to_cmd(se_cmd, entry->rsp.sense_buffer);
  1074			if (!read_len_valid )
  1075				goto done;
  1076			else
> 1077				se_cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
  1078		}
  1079		if (se_cmd->se_cmd_flags & SCF_BIDI) {
  1080			/* Get Data-In buffer before clean up */
  1081			gather_data_area(udev, cmd, true, read_len);
  1082		} else if (se_cmd->data_direction == DMA_FROM_DEVICE) {
  1083			gather_data_area(udev, cmd, false, read_len);
  1084		} else if (se_cmd->data_direction == DMA_TO_DEVICE) {
  1085			/* TODO: */
  1086		} else if (se_cmd->data_direction != DMA_NONE) {
  1087			pr_warn("TCMU: data direction was %d!\n",
  1088				se_cmd->data_direction);
  1089		}
  1090	
  1091	done:
  1092		if (read_len_valid) {
  1093			pr_debug("read_len = %d\n", read_len);
  1094			target_complete_cmd_with_length(cmd->se_cmd,
  1095						entry->rsp.scsi_status, read_len);
  1096		} else
  1097			target_complete_cmd(cmd->se_cmd, entry->rsp.scsi_status);
  1098	
  1099	out:
  1100		cmd->se_cmd = NULL;
  1101		tcmu_cmd_free_data(cmd, cmd->dbi_cnt);
  1102		tcmu_free_cmd(cmd);
  1103	}
  1104	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Lee Duncan May 27, 2018, 1:54 p.m. UTC | #2 
On 05/24/2018 09:49 AM, Bodo Stroesser wrote:
> This is patch version 3.
> 
> Generally target core and TCMUser seem to work fine for tape devices and
> media changers.
> But there is at least one situation, where TCMUser is not able to support
> sequential access device emulation correctly.
> 
> ...

Looks good now.

Reviewed-by: Lee Duncan <lduncan@suse.com>
Bodo Stroesser May 30, 2018, 10:04 a.m. UTC | #3 
On 05/26/18 19:14, kbuild test robot wrote:

> Hi Bodo,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.17-rc6]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
Sorry for the late response.

The missing 'SCF_TREAT_READ_AS_NORMAL'is defined in another patch from 
Lee Duncan.
The title of the patch is '[PATCH v4] target: transport should handle st 
FM/EOM/ILI reads'
Link: https://www.spinics.net/lists/target-devel/msg16738.html

According to an email from Martin K. Petersen from 05/18/18 18:26 he 
applied this patch to 4.18/scsi-queue.
Link: https://www.spinics.net/lists/target-devel/msg16743.html

HTH

Bodo

> url:    https://github.com/0day-ci/linux/commits/Bodo-Stroesser/TCMUser-add-read-length-support/20180526-231412
> config: x86_64-randconfig-x003-201820 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> reproduce:
>          # save the attached .config to linux build tree
>          make ARCH=x86_64
>
> All errors (new ones prefixed by >>):
>
>     drivers//target/target_core_user.c: In function 'tcmu_handle_completion':
>>> drivers//target/target_core_user.c:1077:28: error: 'SCF_TREAT_READ_AS_NORMAL' undeclared (first use in this function)
>         se_cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
>                                 ^~~~~~~~~~~~~~~~~~~~~~~~
>     drivers//target/target_core_user.c:1077:28: note: each undeclared identifier is reported only once for each function it appears in
>
diff mbox

Patch

--- a/include/uapi/linux/target_core_user.h
+++ b/include/uapi/linux/target_core_user.h
@@ -43,6 +43,7 @@ 
 #define TCMU_MAILBOX_VERSION 2
 #define ALIGN_SIZE 64 /* Should be enough for most CPUs */
 #define TCMU_MAILBOX_FLAG_CAP_OOOC (1 << 0) /* Out-of-order completions */
+#define TCMU_MAILBOX_FLAG_CAP_READ_LEN (1 << 1) /* Read data length */
 
 struct tcmu_mailbox {
 	__u16 version;
@@ -70,6 +71,7 @@  struct tcmu_cmd_entry_hdr {
 	__u16 cmd_id;
 	__u8 kflags;
 #define TCMU_UFLAG_UNKNOWN_OP 0x1
+#define TCMU_UFLAG_READ_LEN   0x2
 	__u8 uflags;
 
 } __packed;
@@ -118,7 +120,7 @@  struct tcmu_cmd_entry {
 			__u8 scsi_status;
 			__u8 __pad1;
 			__u16 __pad2;
-			__u32 __pad3;
+			__u32 read_len;
 			char sense_buffer[TCMU_SENSE_BUFFERSIZE];
 		} rsp;
 	};
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -576,7 +576,7 @@  static int scatter_data_area(struct tcmu
 }
 
 static void gather_data_area(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
-			     bool bidi)
+			     bool bidi, uint32_t read_len)
 {
 	struct se_cmd *se_cmd = cmd->se_cmd;
 	int i, dbi;
@@ -609,7 +609,7 @@  static void gather_data_area(struct tcmu
 	for_each_sg(data_sg, sg, data_nents, i) {
 		int sg_remaining = sg->length;
 		to = kmap_atomic(sg_page(sg)) + sg->offset;
-		while (sg_remaining > 0) {
+		while (sg_remaining > 0 && read_len > 0) {
 			if (block_remaining == 0) {
 				if (from)
 					kunmap_atomic(from);
@@ -621,6 +621,8 @@  static void gather_data_area(struct tcmu
 			}
 			copy_bytes = min_t(size_t, sg_remaining,
 					block_remaining);
+			if (read_len < copy_bytes)
+				copy_bytes = read_len;
 			offset = DATA_BLOCK_SIZE - block_remaining;
 			tcmu_flush_dcache_range(from, copy_bytes);
 			memcpy(to + sg->length - sg_remaining, from + offset,
@@ -628,8 +630,11 @@  static void gather_data_area(struct tcmu
 
 			sg_remaining -= copy_bytes;
 			block_remaining -= copy_bytes;
+			read_len -= copy_bytes;
 		}
 		kunmap_atomic(to - sg->offset);
+		if (read_len == 0)
+			break;
 	}
 	if (from)
 		kunmap_atomic(from);
@@ -947,6 +952,8 @@  static void tcmu_handle_completion(struc
 {
 	struct se_cmd *se_cmd = cmd->se_cmd;
 	struct tcmu_dev *udev = cmd->tcmu_dev;
+	bool read_len_valid = false;
+	uint32_t read_len = se_cmd->data_length;
 
 	/*
 	 * cmd has been completed already from timeout, just reclaim
@@ -961,13 +968,28 @@  static void tcmu_handle_completion(struc
 		pr_warn("TCMU: Userspace set UNKNOWN_OP flag on se_cmd %p\n",
 			cmd->se_cmd);
 		entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
-	} else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
+		goto done;
+	}
+
+	if (se_cmd->data_direction == DMA_FROM_DEVICE &&
+	    (entry->hdr.uflags & TCMU_UFLAG_READ_LEN) && entry->rsp.read_len) {
+		read_len_valid = true;
+		if (entry->rsp.read_len < read_len)
+			read_len = entry->rsp.read_len;
+	}
+
+	if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
 		transport_copy_sense_to_cmd(se_cmd, entry->rsp.sense_buffer);
-	} else if (se_cmd->se_cmd_flags & SCF_BIDI) {
+		if (!read_len_valid )
+			goto done;
+		else
+			se_cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
+	}
+	if (se_cmd->se_cmd_flags & SCF_BIDI) {
 		/* Get Data-In buffer before clean up */
-		gather_data_area(udev, cmd, true);
+		gather_data_area(udev, cmd, true, read_len);
 	} else if (se_cmd->data_direction == DMA_FROM_DEVICE) {
-		gather_data_area(udev, cmd, false);
+		gather_data_area(udev, cmd, false, read_len);
 	} else if (se_cmd->data_direction == DMA_TO_DEVICE) {
 		/* TODO: */
 	} else if (se_cmd->data_direction != DMA_NONE) {
@@ -975,7 +997,13 @@  static void tcmu_handle_completion(struc
 			se_cmd->data_direction);
 	}
 
-	target_complete_cmd(cmd->se_cmd, entry->rsp.scsi_status);
+done:
+	if (read_len_valid) {
+		pr_debug("read_len = %d\n", read_len);
+		target_complete_cmd_with_length(cmd->se_cmd,
+					entry->rsp.scsi_status, read_len);
+	} else
+		target_complete_cmd(cmd->se_cmd, entry->rsp.scsi_status);
 
 out:
 	cmd->se_cmd = NULL;
@@ -1532,7 +1560,7 @@  static int tcmu_configure_device(struct
 	/* Initialise the mailbox of the ring buffer */
 	mb = udev->mb_addr;
 	mb->version = TCMU_MAILBOX_VERSION;
-	mb->flags = TCMU_MAILBOX_FLAG_CAP_OOOC;
+	mb->flags = TCMU_MAILBOX_FLAG_CAP_OOOC | TCMU_MAILBOX_FLAG_CAP_READ_LEN;
 	mb->cmdr_off = CMDR_OFF;
 	mb->cmdr_size = udev->cmdr_size;