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

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

[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 (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(-)

-- 
2.25.1

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

[Sergey Samoylenko]

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.

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

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6969 bytes --]

Hi Sergey,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on v5.13-rc7 next-20210624]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sergey-Samoylenko/scsi-target-core-Fix-sense-key-for-invalid-XCOPY-request/20210624-192229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-a001-20210622 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 7c8a507272587f181ec29401453949ebcd8fec65)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/2af81ab452a5bda2c33f25a230cda9f97ebb0431
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sergey-Samoylenko/scsi-target-core-Fix-sense-key-for-invalid-XCOPY-request/20210624-192229
        git checkout 2af81ab452a5bda2c33f25a230cda9f97ebb0431
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/target/target_core_xcopy.c:782:2: error: implicit declaration of function 'target_complete_cmd_with_sense' [-Werror,-Wimplicit-function-declaration]
           target_complete_cmd_with_sense(ec_cmd, sense_rc);
           ^
   drivers/target/target_core_xcopy.c:782:2: note: did you mean 'target_complete_cmd_with_length'?
   include/target/target_core_backend.h:78:6: note: 'target_complete_cmd_with_length' declared here
   void    target_complete_cmd_with_length(struct se_cmd *, u8, int);
           ^
   1 error generated.


vim +/target_complete_cmd_with_sense +782 drivers/target/target_core_xcopy.c

   667	
   668	static void target_xcopy_do_work(struct work_struct *work)
   669	{
   670		struct xcopy_op *xop = container_of(work, struct xcopy_op, xop_work);
   671		struct se_cmd *ec_cmd = xop->xop_se_cmd;
   672		struct se_device *src_dev, *dst_dev;
   673		sector_t src_lba, dst_lba, end_lba;
   674		unsigned int max_sectors;
   675		int rc = 0;
   676		unsigned short nolb, max_nolb, copied_nolb = 0;
   677		sense_reason_t sense_rc;
   678	
   679		sense_rc = target_parse_xcopy_cmd(xop);
   680		if (sense_rc != TCM_NO_SENSE)
   681			goto err_free;
   682	
   683		if (WARN_ON_ONCE(!xop->src_dev) || WARN_ON_ONCE(!xop->dst_dev)) {
   684			sense_rc = TCM_INVALID_PARAMETER_LIST;
   685			goto err_free;
   686		}
   687	
   688		src_dev = xop->src_dev;
   689		dst_dev = xop->dst_dev;
   690		src_lba = xop->src_lba;
   691		dst_lba = xop->dst_lba;
   692		nolb = xop->nolb;
   693		end_lba = src_lba + nolb;
   694		/*
   695		 * Break up XCOPY I/O into hw_max_sectors sized I/O based on the
   696		 * smallest max_sectors between src_dev + dev_dev, or
   697		 */
   698		max_sectors = min(src_dev->dev_attrib.hw_max_sectors,
   699				  dst_dev->dev_attrib.hw_max_sectors);
   700		max_sectors = min_t(u32, max_sectors, XCOPY_MAX_SECTORS);
   701	
   702		max_nolb = min_t(u16, max_sectors, ((u16)(~0U)));
   703	
   704		pr_debug("target_xcopy_do_work: nolb: %hu, max_nolb: %hu end_lba: %llu\n",
   705				nolb, max_nolb, (unsigned long long)end_lba);
   706		pr_debug("target_xcopy_do_work: Starting src_lba: %llu, dst_lba: %llu\n",
   707				(unsigned long long)src_lba, (unsigned long long)dst_lba);
   708	
   709		while (src_lba < end_lba) {
   710			unsigned short cur_nolb = min(nolb, max_nolb);
   711			u32 cur_bytes = cur_nolb * src_dev->dev_attrib.block_size;
   712	
   713			if (cur_bytes != xop->xop_data_bytes) {
   714				/*
   715				 * (Re)allocate a buffer large enough to hold the XCOPY
   716				 * I/O size, which can be reused each read / write loop.
   717				 */
   718				target_free_sgl(xop->xop_data_sg, xop->xop_data_nents);
   719				rc = target_alloc_sgl(&xop->xop_data_sg,
   720						      &xop->xop_data_nents,
   721						      cur_bytes,
   722						      false, false);
   723				if (rc < 0)
   724					goto out;
   725				xop->xop_data_bytes = cur_bytes;
   726			}
   727	
   728			pr_debug("target_xcopy_do_work: Calling read src_dev: %p src_lba: %llu,"
   729				" cur_nolb: %hu\n", src_dev, (unsigned long long)src_lba, cur_nolb);
   730	
   731			rc = target_xcopy_read_source(ec_cmd, xop, src_dev, src_lba, cur_nolb);
   732			if (rc < 0)
   733				goto out;
   734	
   735			src_lba += cur_nolb;
   736			pr_debug("target_xcopy_do_work: Incremented READ src_lba to %llu\n",
   737					(unsigned long long)src_lba);
   738	
   739			pr_debug("target_xcopy_do_work: Calling write dst_dev: %p dst_lba: %llu,"
   740				" cur_nolb: %hu\n", dst_dev, (unsigned long long)dst_lba, cur_nolb);
   741	
   742			rc = target_xcopy_write_destination(ec_cmd, xop, dst_dev,
   743							dst_lba, cur_nolb);
   744			if (rc < 0)
   745				goto out;
   746	
   747			dst_lba += cur_nolb;
   748			pr_debug("target_xcopy_do_work: Incremented WRITE dst_lba to %llu\n",
   749					(unsigned long long)dst_lba);
   750	
   751			copied_nolb += cur_nolb;
   752			nolb -= cur_nolb;
   753		}
   754	
   755		xcopy_pt_undepend_remotedev(xop);
   756		target_free_sgl(xop->xop_data_sg, xop->xop_data_nents);
   757		kfree(xop);
   758	
   759		pr_debug("target_xcopy_do_work: Final src_lba: %llu, dst_lba: %llu\n",
   760			(unsigned long long)src_lba, (unsigned long long)dst_lba);
   761		pr_debug("target_xcopy_do_work: Blocks copied: %hu, Bytes Copied: %u\n",
   762			copied_nolb, copied_nolb * dst_dev->dev_attrib.block_size);
   763	
   764		pr_debug("target_xcopy_do_work: Setting X-COPY GOOD status -> sending response\n");
   765		target_complete_cmd(ec_cmd, SAM_STAT_GOOD);
   766		return;
   767	
   768	out:
   769		/*
   770		 * The XCOPY command was aborted after some data was transferred.
   771		 * Terminate command with CHECK CONDITION status, with the sense key
   772		 * set to COPY ABORTED.
   773		 */
   774		sense_rc = TCM_COPY_TARGET_DEVICE_NOT_REACHABLE;
   775		xcopy_pt_undepend_remotedev(xop);
   776		target_free_sgl(xop->xop_data_sg, xop->xop_data_nents);
   777	
   778	err_free:
   779		kfree(xop);
   780		pr_warn_ratelimited("target_xcopy_do_work: rc: %d, sense: %u,"
   781			" XCOPY operation failed\n", rc, sense_rc);
 > 782		target_complete_cmd_with_sense(ec_cmd, sense_rc);
   783	}
   784	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45142 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7136 bytes --]

Hi Sergey,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on v5.13-rc7 next-20210624]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sergey-Samoylenko/scsi-target-core-Fix-sense-key-for-invalid-XCOPY-request/20210624-192229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-a001-20210622 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 7c8a507272587f181ec29401453949ebcd8fec65)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/2af81ab452a5bda2c33f25a230cda9f97ebb0431
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sergey-Samoylenko/scsi-target-core-Fix-sense-key-for-invalid-XCOPY-request/20210624-192229
        git checkout 2af81ab452a5bda2c33f25a230cda9f97ebb0431
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/target/target_core_xcopy.c:782:2: error: implicit declaration of function 'target_complete_cmd_with_sense' [-Werror,-Wimplicit-function-declaration]
           target_complete_cmd_with_sense(ec_cmd, sense_rc);
           ^
   drivers/target/target_core_xcopy.c:782:2: note: did you mean 'target_complete_cmd_with_length'?
   include/target/target_core_backend.h:78:6: note: 'target_complete_cmd_with_length' declared here
   void    target_complete_cmd_with_length(struct se_cmd *, u8, int);
           ^
   1 error generated.


vim +/target_complete_cmd_with_sense +782 drivers/target/target_core_xcopy.c

   667	
   668	static void target_xcopy_do_work(struct work_struct *work)
   669	{
   670		struct xcopy_op *xop = container_of(work, struct xcopy_op, xop_work);
   671		struct se_cmd *ec_cmd = xop->xop_se_cmd;
   672		struct se_device *src_dev, *dst_dev;
   673		sector_t src_lba, dst_lba, end_lba;
   674		unsigned int max_sectors;
   675		int rc = 0;
   676		unsigned short nolb, max_nolb, copied_nolb = 0;
   677		sense_reason_t sense_rc;
   678	
   679		sense_rc = target_parse_xcopy_cmd(xop);
   680		if (sense_rc != TCM_NO_SENSE)
   681			goto err_free;
   682	
   683		if (WARN_ON_ONCE(!xop->src_dev) || WARN_ON_ONCE(!xop->dst_dev)) {
   684			sense_rc = TCM_INVALID_PARAMETER_LIST;
   685			goto err_free;
   686		}
   687	
   688		src_dev = xop->src_dev;
   689		dst_dev = xop->dst_dev;
   690		src_lba = xop->src_lba;
   691		dst_lba = xop->dst_lba;
   692		nolb = xop->nolb;
   693		end_lba = src_lba + nolb;
   694		/*
   695		 * Break up XCOPY I/O into hw_max_sectors sized I/O based on the
   696		 * smallest max_sectors between src_dev + dev_dev, or
   697		 */
   698		max_sectors = min(src_dev->dev_attrib.hw_max_sectors,
   699				  dst_dev->dev_attrib.hw_max_sectors);
   700		max_sectors = min_t(u32, max_sectors, XCOPY_MAX_SECTORS);
   701	
   702		max_nolb = min_t(u16, max_sectors, ((u16)(~0U)));
   703	
   704		pr_debug("target_xcopy_do_work: nolb: %hu, max_nolb: %hu end_lba: %llu\n",
   705				nolb, max_nolb, (unsigned long long)end_lba);
   706		pr_debug("target_xcopy_do_work: Starting src_lba: %llu, dst_lba: %llu\n",
   707				(unsigned long long)src_lba, (unsigned long long)dst_lba);
   708	
   709		while (src_lba < end_lba) {
   710			unsigned short cur_nolb = min(nolb, max_nolb);
   711			u32 cur_bytes = cur_nolb * src_dev->dev_attrib.block_size;
   712	
   713			if (cur_bytes != xop->xop_data_bytes) {
   714				/*
   715				 * (Re)allocate a buffer large enough to hold the XCOPY
   716				 * I/O size, which can be reused each read / write loop.
   717				 */
   718				target_free_sgl(xop->xop_data_sg, xop->xop_data_nents);
   719				rc = target_alloc_sgl(&xop->xop_data_sg,
   720						      &xop->xop_data_nents,
   721						      cur_bytes,
   722						      false, false);
   723				if (rc < 0)
   724					goto out;
   725				xop->xop_data_bytes = cur_bytes;
   726			}
   727	
   728			pr_debug("target_xcopy_do_work: Calling read src_dev: %p src_lba: %llu,"
   729				" cur_nolb: %hu\n", src_dev, (unsigned long long)src_lba, cur_nolb);
   730	
   731			rc = target_xcopy_read_source(ec_cmd, xop, src_dev, src_lba, cur_nolb);
   732			if (rc < 0)
   733				goto out;
   734	
   735			src_lba += cur_nolb;
   736			pr_debug("target_xcopy_do_work: Incremented READ src_lba to %llu\n",
   737					(unsigned long long)src_lba);
   738	
   739			pr_debug("target_xcopy_do_work: Calling write dst_dev: %p dst_lba: %llu,"
   740				" cur_nolb: %hu\n", dst_dev, (unsigned long long)dst_lba, cur_nolb);
   741	
   742			rc = target_xcopy_write_destination(ec_cmd, xop, dst_dev,
   743							dst_lba, cur_nolb);
   744			if (rc < 0)
   745				goto out;
   746	
   747			dst_lba += cur_nolb;
   748			pr_debug("target_xcopy_do_work: Incremented WRITE dst_lba to %llu\n",
   749					(unsigned long long)dst_lba);
   750	
   751			copied_nolb += cur_nolb;
   752			nolb -= cur_nolb;
   753		}
   754	
   755		xcopy_pt_undepend_remotedev(xop);
   756		target_free_sgl(xop->xop_data_sg, xop->xop_data_nents);
   757		kfree(xop);
   758	
   759		pr_debug("target_xcopy_do_work: Final src_lba: %llu, dst_lba: %llu\n",
   760			(unsigned long long)src_lba, (unsigned long long)dst_lba);
   761		pr_debug("target_xcopy_do_work: Blocks copied: %hu, Bytes Copied: %u\n",
   762			copied_nolb, copied_nolb * dst_dev->dev_attrib.block_size);
   763	
   764		pr_debug("target_xcopy_do_work: Setting X-COPY GOOD status -> sending response\n");
   765		target_complete_cmd(ec_cmd, SAM_STAT_GOOD);
   766		return;
   767	
   768	out:
   769		/*
   770		 * The XCOPY command was aborted after some data was transferred.
   771		 * Terminate command with CHECK CONDITION status, with the sense key
   772		 * set to COPY ABORTED.
   773		 */
   774		sense_rc = TCM_COPY_TARGET_DEVICE_NOT_REACHABLE;
   775		xcopy_pt_undepend_remotedev(xop);
   776		target_free_sgl(xop->xop_data_sg, xop->xop_data_nents);
   777	
   778	err_free:
   779		kfree(xop);
   780		pr_warn_ratelimited("target_xcopy_do_work: rc: %d, sense: %u,"
   781			" XCOPY operation failed\n", rc, sense_rc);
 > 782		target_complete_cmd_with_sense(ec_cmd, sense_rc);
   783	}
   784	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 45142 bytes --]

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

[Bart Van Assche]

On 6/24/21 4:19 AM, Sergey Samoylenko wrote:
> +	pr_warn_ratelimited("target_xcopy_do_work: rc: %d, sense: %u,"
> +		" XCOPY operation failed\n", rc, sense_rc);

Please do not split format strings across multiple lines. Checkpatch
should have complained about this.

Thanks,

Bart.

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

[Sergey Samoylenko]

Thank you Bart,

I used API (target_complete_cmd_with_sense) which is not in
mainline kernel. I need to change the patch.
Sorry, this is my mistake.

Sergey.

>On 6/24/21 4:19 AM, Sergey Samoylenko wrote:
>> +	pr_warn_ratelimited("target_xcopy_do_work: rc: %d, sense: %u,"
>> +		" XCOPY operation failed\n", rc, sense_rc);
>
>Please do not split format strings across multiple lines. Checkpatch
>should have complained about this.
>
>Thanks,
>
>Bart.

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

[David Disseldorp]

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

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

[Sergey Samoylenko]

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;

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

[David Disseldorp]

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