diff mbox series

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

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

Commit Message

Sergey Samoylenko June 24, 2021, 11:19 a.m. UTC 
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(-)

Comments

kernel test robot June 24, 2021, 3:49 p.m. UTC | #1 
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
Bart Van Assche June 24, 2021, 3:57 p.m. UTC | #2 
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.
Sergey Samoylenko June 24, 2021, 5:27 p.m. UTC | #3 
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.
diff mbox series

Patch

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);
 }
 
 /*