| Message ID | 20220630122241.1658-4-a.kovaleva@yadro.com |
|---|---|
| State | New |
| Headers | show |
| Series | Make target send correct io limits | expand |
Hi Anastasia,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on linus/master v5.19-rc4 next-20220630]
[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/intel-lab-lkp/linux/commits/Anastasia-Kovaleva/Make-target-send-correct-io-limits/20220630-203342
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: i386-randconfig-a002
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a9119143a2d1f4d0d0bc1fe0d819e5351b4e0deb)
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
# https://github.com/intel-lab-lkp/linux/commit/56a549ae44a7fa719292ad848e1f70fa745d0ece
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Anastasia-Kovaleva/Make-target-send-correct-io-limits/20220630-203342
git checkout 56a549ae44a7fa719292ad848e1f70fa745d0ece
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/target/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/target/target_core_xcopy.c:765:3: warning: format specifies type 'unsigned short' but the argument has type 'unsigned int' [-Wformat]
copied_bytes / dst_dev->dev_attrib.block_size, copied_bytes);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/printk.h:599:26: note: expanded from macro 'pr_debug'
dynamic_pr_debug(fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dynamic_debug.h:163:22: note: expanded from macro 'dynamic_pr_debug'
pr_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dynamic_debug.h:152:56: note: expanded from macro '_dynamic_func_call'
__dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
^~~~~~~~~~~
include/linux/dynamic_debug.h:134:15: note: expanded from macro '__dynamic_func_call'
func(&id, ##__VA_ARGS__); \
^~~~~~~~~~~
1 warning generated.
vim +765 drivers/target/target_core_xcopy.c
666
667 static void target_xcopy_do_work(struct work_struct *work)
668 {
669 struct xcopy_op *xop = container_of(work, struct xcopy_op, xop_work);
670 struct se_cmd *ec_cmd = xop->xop_se_cmd;
671 struct se_device *src_dev, *dst_dev;
672 sector_t src_lba, dst_lba, end_lba;
673 unsigned long long int max_bytes, max_bytes_src, max_bytes_dst, max_blocks;
674 int rc = 0;
675 unsigned short nolb;
676 unsigned int copied_bytes = 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 * hw_block_size sized
696 * I/O based on the smallest max_bytes between src_dev + dst_dev
697 */
698 max_bytes_src = (unsigned long long) src_dev->dev_attrib.hw_max_sectors *
699 src_dev->dev_attrib.hw_block_size;
700 max_bytes_dst = (unsigned long long) dst_dev->dev_attrib.hw_max_sectors *
701 dst_dev->dev_attrib.hw_block_size;
702
703 max_bytes = min_t(u64, max_bytes_src, max_bytes_dst);
704 max_bytes = min_t(u64, max_bytes, XCOPY_MAX_BYTES);
705 max_blocks = max_bytes / src_dev->dev_attrib.block_size;
706
707 pr_debug("target_xcopy_do_work: nolb: %hu, max_blocks: %llu end_lba: %llu\n",
708 nolb, max_blocks, (unsigned long long)end_lba);
709 pr_debug("target_xcopy_do_work: Starting src_lba: %llu, dst_lba: %llu\n",
710 (unsigned long long)src_lba, (unsigned long long)dst_lba);
711
712 while (nolb) {
713 u32 cur_bytes = min_t(u64, max_bytes, nolb * src_dev->dev_attrib.block_size);
714 unsigned short cur_nolb = cur_bytes / src_dev->dev_attrib.block_size;
715
716 if (cur_bytes != xop->xop_data_bytes) {
717 /*
718 * (Re)allocate a buffer large enough to hold the XCOPY
719 * I/O size, which can be reused each read / write loop.
720 */
721 target_free_sgl(xop->xop_data_sg, xop->xop_data_nents);
722 rc = target_alloc_sgl(&xop->xop_data_sg,
723 &xop->xop_data_nents,
724 cur_bytes,
725 false, false);
726 if (rc < 0)
727 goto out;
728 xop->xop_data_bytes = cur_bytes;
729 }
730
731 pr_debug("target_xcopy_do_work: Calling read src_dev: %p src_lba: %llu,"
732 " cur_nolb: %hu\n", src_dev, (unsigned long long)src_lba, cur_nolb);
733
734 rc = target_xcopy_read_source(ec_cmd, xop, src_dev, src_lba, cur_bytes);
735 if (rc < 0)
736 goto out;
737
738 src_lba += cur_bytes / src_dev->dev_attrib.block_size;
739 pr_debug("target_xcopy_do_work: Incremented READ src_lba to %llu\n",
740 (unsigned long long)src_lba);
741
742 pr_debug("target_xcopy_do_work: Calling write dst_dev: %p dst_lba: %llu,"
743 " cur_nolb: %hu\n", dst_dev, (unsigned long long)dst_lba, cur_nolb);
744
745 rc = target_xcopy_write_destination(ec_cmd, xop, dst_dev,
746 dst_lba, cur_bytes);
747 if (rc < 0)
748 goto out;
749
750 dst_lba += cur_bytes / dst_dev->dev_attrib.block_size;
751 pr_debug("target_xcopy_do_work: Incremented WRITE dst_lba to %llu\n",
752 (unsigned long long)dst_lba);
753
754 copied_bytes += cur_bytes;
755 nolb -= cur_bytes / src_dev->dev_attrib.block_size;
756 }
757
758 xcopy_pt_undepend_remotedev(xop);
759 target_free_sgl(xop->xop_data_sg, xop->xop_data_nents);
760 kfree(xop);
761
762 pr_debug("target_xcopy_do_work: Final src_lba: %llu, dst_lba: %llu\n",
763 (unsigned long long)src_lba, (unsigned long long)dst_lba);
764 pr_debug("target_xcopy_do_work: Blocks copied: %hu, Bytes Copied: %u\n",
> 765 copied_bytes / dst_dev->dev_attrib.block_size, copied_bytes);
766
767 pr_debug("target_xcopy_do_work: Setting X-COPY GOOD status -> sending response\n");
768 target_complete_cmd(ec_cmd, SAM_STAT_GOOD);
769 return;
770
771 out:
772 /*
773 * The XCOPY command was aborted after some data was transferred.
774 * Terminate command with CHECK CONDITION status, with the sense key
775 * set to COPY ABORTED.
776 */
777 sense_rc = TCM_COPY_TARGET_DEVICE_NOT_REACHABLE;
778 xcopy_pt_undepend_remotedev(xop);
779 target_free_sgl(xop->xop_data_sg, xop->xop_data_nents);
780
781 err_free:
782 kfree(xop);
783 pr_warn_ratelimited("target_xcopy_do_work: rc: %d, sense: %u, XCOPY operation failed\n",
784 rc, sense_rc);
785 target_complete_cmd_with_sense(ec_cmd, SAM_STAT_CHECK_CONDITION, sense_rc);
786 }
787
Hi Anastasia,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on linus/master v5.19-rc4 next-20220630]
[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/intel-lab-lkp/linux/commits/Anastasia-Kovaleva/Make-target-send-correct-io-limits/20220630-203342
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: i386-randconfig-a005 (https://download.01.org/0day-ci/archive/20220701/202207011237.RwOL3m3V-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/56a549ae44a7fa719292ad848e1f70fa745d0ece
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Anastasia-Kovaleva/Make-target-send-correct-io-limits/20220630-203342
git checkout 56a549ae44a7fa719292ad848e1f70fa745d0ece
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ld: drivers/target/target_core_xcopy.o: in function `target_xcopy_do_work':
>> target_core_xcopy.c:(.text+0x138d): undefined reference to `__udivdi3'
Hi Anastasia,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on linus/master v5.19-rc4 next-20220701]
[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/intel-lab-lkp/linux/commits/Anastasia-Kovaleva/Make-target-send-correct-io-limits/20220630-203342
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20220702/202207021939.V7V97GD3-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
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
# https://github.com/intel-lab-lkp/linux/commit/56a549ae44a7fa719292ad848e1f70fa745d0ece
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Anastasia-Kovaleva/Make-target-send-correct-io-limits/20220630-203342
git checkout 56a549ae44a7fa719292ad848e1f70fa745d0ece
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "__divdi3" [drivers/target/target_core_mod.ko] undefined!
>> ERROR: modpost: "__udivdi3" [drivers/target/target_core_mod.ko] undefined!
On 6/30/22 7:22 AM, Anastasia Kovaleva wrote: > To determine how many blocks sends in one command, the minimum value is > selected from the hw_max_sectors of both devices. In > target_xcopy_do_work, hw_max_sectors are used as blocks, not sectors; it > also ignores the fact that sectors can be of different sizes, for > example 512 and 4096 bytes. Because of this, a number of blocks can be > transmitted that the device will not be able to accept. > > Change the selection of max thransmition size into bytes. I think it's "transmission". Run the scripts/checkpatch.pl on the patch and fix up those warnings. Also don't forget to checkout the kernel test bot warnings. > > Signed-off-by: Anastasia Kovaleva <a.kovaleva@yadro.com> > Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com> > Reviewed-by: Dmitriy Bogdanov <d.bogdanov@yadro.com> > --- > drivers/target/target_core_xcopy.c | 68 ++++++++++++++++-------------- > drivers/target/target_core_xcopy.h | 2 +- > 2 files changed, 37 insertions(+), 33 deletions(-) > > diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c > index 6bb20aa9c5bc..c9341a92b567 100644 > --- a/drivers/target/target_core_xcopy.c > +++ b/drivers/target/target_core_xcopy.c > @@ -582,11 +582,11 @@ static int target_xcopy_read_source( > struct xcopy_op *xop, > struct se_device *src_dev, > sector_t src_lba, > - u32 src_sectors) > + u32 src_bytes) > { > struct xcopy_pt_cmd xpt_cmd; > struct se_cmd *se_cmd = &xpt_cmd.se_cmd; > - u32 length = (src_sectors * src_dev->dev_attrib.block_size); > + u32 transfer_length = src_bytes / src_dev->dev_attrib.block_size; It was nice how in the patches you added a bytes/blocks to the variable names. Maybe do that here and the other transfer_length you added? I wasn't sure if you just forgot or maybe adding a "block" in the name made it sound weird.
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c index 6bb20aa9c5bc..c9341a92b567 100644 --- a/drivers/target/target_core_xcopy.c +++ b/drivers/target/target_core_xcopy.c @@ -582,11 +582,11 @@ static int target_xcopy_read_source( struct xcopy_op *xop, struct se_device *src_dev, sector_t src_lba, - u32 src_sectors) + u32 src_bytes) { struct xcopy_pt_cmd xpt_cmd; struct se_cmd *se_cmd = &xpt_cmd.se_cmd; - u32 length = (src_sectors * src_dev->dev_attrib.block_size); + u32 transfer_length = src_bytes / src_dev->dev_attrib.block_size; int rc; unsigned char cdb[16]; bool remote_port = (xop->op_origin == XCOL_DEST_RECV_OP); @@ -597,11 +597,11 @@ static int target_xcopy_read_source( memset(&cdb[0], 0, 16); cdb[0] = READ_16; put_unaligned_be64(src_lba, &cdb[2]); - put_unaligned_be32(src_sectors, &cdb[10]); - pr_debug("XCOPY: Built READ_16: LBA: %llu Sectors: %u Length: %u\n", - (unsigned long long)src_lba, src_sectors, length); + put_unaligned_be32(transfer_length, &cdb[10]); + pr_debug("XCOPY: Built READ_16: LBA: %llu Blocks: %u Length: %u\n", + (unsigned long long)src_lba, transfer_length, src_bytes); - __target_init_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length, + __target_init_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, src_bytes, DMA_FROM_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0); rc = target_xcopy_setup_pt_cmd(&xpt_cmd, xop, src_dev, &cdb[0], @@ -627,11 +627,11 @@ static int target_xcopy_write_destination( struct xcopy_op *xop, struct se_device *dst_dev, sector_t dst_lba, - u32 dst_sectors) + u32 dst_bytes) { struct xcopy_pt_cmd xpt_cmd; struct se_cmd *se_cmd = &xpt_cmd.se_cmd; - u32 length = (dst_sectors * dst_dev->dev_attrib.block_size); + u32 transfer_length = dst_bytes / dst_dev->dev_attrib.block_size; int rc; unsigned char cdb[16]; bool remote_port = (xop->op_origin == XCOL_SOURCE_RECV_OP); @@ -642,11 +642,11 @@ static int target_xcopy_write_destination( memset(&cdb[0], 0, 16); cdb[0] = WRITE_16; put_unaligned_be64(dst_lba, &cdb[2]); - put_unaligned_be32(dst_sectors, &cdb[10]); - pr_debug("XCOPY: Built WRITE_16: LBA: %llu Sectors: %u Length: %u\n", - (unsigned long long)dst_lba, dst_sectors, length); + put_unaligned_be32(transfer_length, &cdb[10]); + pr_debug("XCOPY: Built WRITE_16: LBA: %llu Blocks: %u Length: %u\n", + (unsigned long long)dst_lba, transfer_length, dst_bytes); - __target_init_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length, + __target_init_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, dst_bytes, DMA_TO_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0); rc = target_xcopy_setup_pt_cmd(&xpt_cmd, xop, dst_dev, &cdb[0], @@ -670,9 +670,10 @@ static void target_xcopy_do_work(struct work_struct *work) struct se_cmd *ec_cmd = xop->xop_se_cmd; struct se_device *src_dev, *dst_dev; sector_t src_lba, dst_lba, end_lba; - unsigned int max_sectors; + unsigned long long int max_bytes, max_bytes_src, max_bytes_dst, max_blocks; int rc = 0; - unsigned short nolb, max_nolb, copied_nolb = 0; + unsigned short nolb; + unsigned int copied_bytes = 0; sense_reason_t sense_rc; sense_rc = target_parse_xcopy_cmd(xop); @@ -691,23 +692,26 @@ static void target_xcopy_do_work(struct work_struct *work) nolb = xop->nolb; end_lba = src_lba + nolb; /* - * Break up XCOPY I/O into hw_max_sectors sized I/O based on the - * smallest max_sectors between src_dev + dev_dev, or + * Break up XCOPY I/O into hw_max_sectors * hw_block_size sized + * I/O based on the smallest max_bytes between src_dev + dst_dev */ - max_sectors = min(src_dev->dev_attrib.hw_max_sectors, - dst_dev->dev_attrib.hw_max_sectors); - max_sectors = min_t(u32, max_sectors, XCOPY_MAX_SECTORS); + max_bytes_src = (unsigned long long) src_dev->dev_attrib.hw_max_sectors * + src_dev->dev_attrib.hw_block_size; + max_bytes_dst = (unsigned long long) dst_dev->dev_attrib.hw_max_sectors * + dst_dev->dev_attrib.hw_block_size; - max_nolb = min_t(u16, max_sectors, ((u16)(~0U))); + max_bytes = min_t(u64, max_bytes_src, max_bytes_dst); + max_bytes = min_t(u64, max_bytes, XCOPY_MAX_BYTES); + max_blocks = max_bytes / src_dev->dev_attrib.block_size; - pr_debug("target_xcopy_do_work: nolb: %hu, max_nolb: %hu end_lba: %llu\n", - nolb, max_nolb, (unsigned long long)end_lba); + pr_debug("target_xcopy_do_work: nolb: %hu, max_blocks: %llu end_lba: %llu\n", + nolb, max_blocks, (unsigned long long)end_lba); pr_debug("target_xcopy_do_work: Starting src_lba: %llu, dst_lba: %llu\n", (unsigned long long)src_lba, (unsigned long long)dst_lba); - while (src_lba < end_lba) { - unsigned short cur_nolb = min(nolb, max_nolb); - u32 cur_bytes = cur_nolb * src_dev->dev_attrib.block_size; + while (nolb) { + u32 cur_bytes = min_t(u64, max_bytes, nolb * src_dev->dev_attrib.block_size); + unsigned short cur_nolb = cur_bytes / src_dev->dev_attrib.block_size; if (cur_bytes != xop->xop_data_bytes) { /* @@ -727,11 +731,11 @@ static void target_xcopy_do_work(struct work_struct *work) pr_debug("target_xcopy_do_work: Calling read src_dev: %p src_lba: %llu," " cur_nolb: %hu\n", src_dev, (unsigned long long)src_lba, cur_nolb); - rc = target_xcopy_read_source(ec_cmd, xop, src_dev, src_lba, cur_nolb); + rc = target_xcopy_read_source(ec_cmd, xop, src_dev, src_lba, cur_bytes); if (rc < 0) goto out; - src_lba += cur_nolb; + src_lba += cur_bytes / src_dev->dev_attrib.block_size; pr_debug("target_xcopy_do_work: Incremented READ src_lba to %llu\n", (unsigned long long)src_lba); @@ -739,16 +743,16 @@ static void target_xcopy_do_work(struct work_struct *work) " cur_nolb: %hu\n", dst_dev, (unsigned long long)dst_lba, cur_nolb); rc = target_xcopy_write_destination(ec_cmd, xop, dst_dev, - dst_lba, cur_nolb); + dst_lba, cur_bytes); if (rc < 0) goto out; - dst_lba += cur_nolb; + dst_lba += cur_bytes / dst_dev->dev_attrib.block_size; pr_debug("target_xcopy_do_work: Incremented WRITE dst_lba to %llu\n", (unsigned long long)dst_lba); - copied_nolb += cur_nolb; - nolb -= cur_nolb; + copied_bytes += cur_bytes; + nolb -= cur_bytes / src_dev->dev_attrib.block_size; } xcopy_pt_undepend_remotedev(xop); @@ -758,7 +762,7 @@ static void target_xcopy_do_work(struct work_struct *work) pr_debug("target_xcopy_do_work: Final src_lba: %llu, dst_lba: %llu\n", (unsigned long long)src_lba, (unsigned long long)dst_lba); pr_debug("target_xcopy_do_work: Blocks copied: %hu, Bytes Copied: %u\n", - copied_nolb, copied_nolb * dst_dev->dev_attrib.block_size); + copied_bytes / dst_dev->dev_attrib.block_size, copied_bytes); pr_debug("target_xcopy_do_work: Setting X-COPY GOOD status -> sending response\n"); target_complete_cmd(ec_cmd, SAM_STAT_GOOD); diff --git a/drivers/target/target_core_xcopy.h b/drivers/target/target_core_xcopy.h index e5f20005179a..51a8e4d85d3e 100644 --- a/drivers/target/target_core_xcopy.h +++ b/drivers/target/target_core_xcopy.h @@ -5,7 +5,7 @@ #define XCOPY_TARGET_DESC_LEN 32 #define XCOPY_SEGMENT_DESC_LEN 28 #define XCOPY_NAA_IEEE_REGEX_LEN 16 -#define XCOPY_MAX_SECTORS 4096 +#define XCOPY_MAX_BYTES 16777216 /* 16 MB */ /* * SPC4r37 6.4.6.1