by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v8 6/9] nvmet: add copy command support for bdev and file ns

Hi Anuj,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on axboe-block/for-next]
[also build test WARNING on linus/master v6.3-rc4 next-20230329]
[cannot apply to device-mapper-dm/for-next]
[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#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Anuj-Gupta/block-Add-copy-offload-support-infrastructure/20230329-162018
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link: https://lore.kernel.org/r/20230327084103.21601-7-anuj20.g%40samsung.com
patch subject: [PATCH v8 6/9] nvmet: add copy command support for bdev and file ns
config: arm64-randconfig-s041-20230329 (https://download.01.org/0day-ci/archive/20230330/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/f846a8ac40882d9d42532e9e2b43560650ef8510
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Anuj-Gupta/block-Add-copy-offload-support-infrastructure/20230329-162018
git checkout f846a8ac40882d9d42532e9e2b43560650ef8510
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/nvme/target/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

sparse warnings: (new ones prefixed by >>)
>> drivers/nvme/target/admin-cmd.c:539:29: sparse: sparse: cast from restricted __le16

vim +539 drivers/nvme/target/admin-cmd.c

490
491 static void nvmet_execute_identify_ns(struct nvmet_req *req)
492 {
493 struct nvme_id_ns *id;
494 u16 status;
495
496 if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) {
497 req->error_loc = offsetof(struct nvme_identify, nsid);
498 status = NVME_SC_INVALID_NS | NVME_SC_DNR;
499 goto out;
500 }
501
502 id = kzalloc(sizeof(*id), GFP_KERNEL);
503 if (!id) {
504 status = NVME_SC_INTERNAL;
505 goto out;
506 }
507
508 /* return an all zeroed buffer if we can't find an active namespace */
509 status = nvmet_req_find_ns(req);
510 if (status) {
511 status = 0;
512 goto done;
513 }
514
515 if (nvmet_ns_revalidate(req->ns)) {
516 mutex_lock(&req->ns->subsys->lock);
517 nvmet_ns_changed(req->ns->subsys, req->ns->nsid);
518 mutex_unlock(&req->ns->subsys->lock);
519 }
520
521 /*
522 * nuse = ncap = nsze isn't always true, but we have no way to find
523 * that out from the underlying device.
524 */
525 id->ncap = id->nsze =
526 cpu_to_le64(req->ns->size >> req->ns->blksize_shift);
527 switch (req->port->ana_state[req->ns->anagrpid]) {
528 case NVME_ANA_INACCESSIBLE:
529 case NVME_ANA_PERSISTENT_LOSS:
530 break;
531 default:
532 id->nuse = id->nsze;
533 break;
534 }
535
536 if (req->ns->bdev)
537 nvmet_bdev_set_limits(req->ns->bdev, id);
538 else {
> 539 id->msrc = (u8)to0based(BIO_MAX_VECS - 1);
540 id->mssrl = cpu_to_le16(BIO_MAX_VECS <<
541 (PAGE_SHIFT - SECTOR_SHIFT));
542 id->mcl = cpu_to_le32(le16_to_cpu(id->mssrl));
543 }
544
545 /*
546 * We just provide a single LBA format that matches what the
547 * underlying device reports.
548 */
549 id->nlbaf = 0;
550 id->flbas = 0;
551
552 /*
553 * Our namespace might always be shared. Not just with other
554 * controllers, but also with any other user of the block device.
555 */
556 id->nmic = NVME_NS_NMIC_SHARED;
557 id->anagrpid = cpu_to_le32(req->ns->anagrpid);
558
559 memcpy(&id->nguid, &req->ns->nguid, sizeof(id->nguid));
560
561 id->lbaf[0].ds = req->ns->blksize_shift;
562
563 if (req->sq->ctrl->pi_support && nvmet_ns_has_pi(req->ns)) {
564 id->dpc = NVME_NS_DPC_PI_FIRST | NVME_NS_DPC_PI_LAST |
565 NVME_NS_DPC_PI_TYPE1 | NVME_NS_DPC_PI_TYPE2 |
566 NVME_NS_DPC_PI_TYPE3;
567 id->mc = NVME_MC_EXTENDED_LBA;
568 id->dps = req->ns->pi_type;
569 id->flbas = NVME_NS_FLBAS_META_EXT;
570 id->lbaf[0].ms = cpu_to_le16(req->ns->metadata_size);
571 }
572
573 if (req->ns->readonly)
574 id->nsattr |= NVME_NS_ATTR_RO;
575 done:
576 if (!status)
577 status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
578
579 kfree(id);
580 out:
581 nvmet_req_complete(req, status);
582 }
583

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

2023-03-30 05:58:45

by Christian Brauner

[permalink] [raw]

Subject: Re: [PATCH v8 4/9] fs, block: copy_file_range for def_blk_ops for direct block device.

On Wed, Mar 29, 2023 at 06:12:36PM +0530, Nitesh Shetty wrote:
> On Wed, Mar 29, 2023 at 02:14:40PM +0200, Christian Brauner wrote:
> > On Mon, Mar 27, 2023 at 02:10:52PM +0530, Anuj Gupta wrote:
> > > From: Nitesh Shetty <[email protected]>
> > >
> > > For direct block device opened with O_DIRECT, use copy_file_range to
> > > issue device copy offload, and fallback to generic_copy_file_range incase
> > > device copy offload capability is absent.
> > > Modify checks to allow bdevs to use copy_file_range.
> > >
> > > Suggested-by: Ming Lei <[email protected]>
> > > Signed-off-by: Anuj Gupta <[email protected]>
> > > Signed-off-by: Nitesh Shetty <[email protected]>
> > > ---
> > > block/blk-lib.c | 22 ++++++++++++++++++++++
> > > block/fops.c | 20 ++++++++++++++++++++
> > > fs/read_write.c | 11 +++++++++--
> > > include/linux/blkdev.h | 3 +++
> > > 4 files changed, 54 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/block/blk-lib.c b/block/blk-lib.c
> > > index a21819e59b29..c288573c7e77 100644
> > > --- a/block/blk-lib.c
> > > +++ b/block/blk-lib.c
> > > @@ -475,6 +475,28 @@ static inline bool blk_check_copy_offload(struct request_queue *q_in,
> > > return blk_queue_copy(q_in) && blk_queue_copy(q_out);
> > > }
> > >
> > > +int blkdev_copy_offload(struct block_device *bdev_in, loff_t pos_in,
> > > + struct block_device *bdev_out, loff_t pos_out, size_t len,
> > > + cio_iodone_t end_io, void *private, gfp_t gfp_mask)
> > > +{
> > > + struct request_queue *in_q = bdev_get_queue(bdev_in);
> > > + struct request_queue *out_q = bdev_get_queue(bdev_out);
> > > + int ret = -EINVAL;
> >
> > Why initialize to -EINVAL if blk_copy_sanity_check() initializes it
> > right away anyway?
> >
>
> acked.
>
> > > + bool offload = false;
> >
> > Same thing with initializing offload.
> >
> acked
>
> > > +
> > > + ret = blk_copy_sanity_check(bdev_in, pos_in, bdev_out, pos_out, len);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + offload = blk_check_copy_offload(in_q, out_q);
> > > + if (offload)
> > > + ret = __blk_copy_offload(bdev_in, pos_in, bdev_out, pos_out,
> > > + len, end_io, private, gfp_mask);
> > > +
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(blkdev_copy_offload);
> > > +
> > > /*
> > > * @bdev_in: source block device
> > > * @pos_in: source offset
> > > diff --git a/block/fops.c b/block/fops.c
> > > index d2e6be4e3d1c..3b7c05831d5c 100644
> > > --- a/block/fops.c
> > > +++ b/block/fops.c
> > > @@ -611,6 +611,25 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
> > > return ret;
> > > }
> > >
> > > +static ssize_t blkdev_copy_file_range(struct file *file_in, loff_t pos_in,
> > > + struct file *file_out, loff_t pos_out,
> > > + size_t len, unsigned int flags)
> > > +{
> > > + struct block_device *in_bdev = I_BDEV(bdev_file_inode(file_in));
> > > + struct block_device *out_bdev = I_BDEV(bdev_file_inode(file_out));
> > > + int comp_len = 0;
> > > +
> > > + if ((file_in->f_iocb_flags & IOCB_DIRECT) &&
> > > + (file_out->f_iocb_flags & IOCB_DIRECT))
> > > + comp_len = blkdev_copy_offload(in_bdev, pos_in, out_bdev,
> > > + pos_out, len, NULL, NULL, GFP_KERNEL);
> > > + if (comp_len != len)
> > > + comp_len = generic_copy_file_range(file_in, pos_in + comp_len,
> > > + file_out, pos_out + comp_len, len - comp_len, flags);
> >
> > I'm not deeply familiar with this code but this looks odd. It at least
> > seems possible that comp_len could be -EINVAL and len 20 at which point
> > you'd be doing len - comp_len aka 20 - 22 = -2 in generic_copy_file_range().

20 - -22 = 44 ofc

>
> comp_len should be 0 incase of error. We do agree, some function

I mean, not to hammer on this point too much but just to be clear
blk_copy_sanity_check(), which is introduced in the second patch, can
return both -EPERM and -EINVAL and is first called in
blkdev_copy_offload() so it's definitely possible for comp_len to be
negative.

2023-03-30 15:25:29

by Nitesh Shetty

[permalink] [raw]

Subject: Re: [PATCH v8 4/9] fs, block: copy_file_range for def_blk_ops for direct block device.

On Thu, Mar 30, 2023 at 11:18 AM Christian Brauner <[email protected]> wrote:
>
> On Wed, Mar 29, 2023 at 06:12:36PM +0530, Nitesh Shetty wrote:
> > On Wed, Mar 29, 2023 at 02:14:40PM +0200, Christian Brauner wrote:
> > > On Mon, Mar 27, 2023 at 02:10:52PM +0530, Anuj Gupta wrote:
> > > > From: Nitesh Shetty <[email protected]>
> > > >
> > > > For direct block device opened with O_DIRECT, use copy_file_range to
> > > > issue device copy offload, and fallback to generic_copy_file_range incase
> > > > device copy offload capability is absent.
> > > > Modify checks to allow bdevs to use copy_file_range.
> > > >
> > > > Suggested-by: Ming Lei <[email protected]>
> > > > Signed-off-by: Anuj Gupta <[email protected]>
> > > > Signed-off-by: Nitesh Shetty <[email protected]>
> > > > ---
> > > > block/blk-lib.c | 22 ++++++++++++++++++++++
> > > > block/fops.c | 20 ++++++++++++++++++++
> > > > fs/read_write.c | 11 +++++++++--
> > > > include/linux/blkdev.h | 3 +++
> > > > 4 files changed, 54 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/block/blk-lib.c b/block/blk-lib.c
> > > > index a21819e59b29..c288573c7e77 100644
> > > > --- a/block/blk-lib.c
> > > > +++ b/block/blk-lib.c
> > > > @@ -475,6 +475,28 @@ static inline bool blk_check_copy_offload(struct request_queue *q_in,
> > > > return blk_queue_copy(q_in) && blk_queue_copy(q_out);
> > > > }
> > > >
> > > > +int blkdev_copy_offload(struct block_device *bdev_in, loff_t pos_in,
> > > > + struct block_device *bdev_out, loff_t pos_out, size_t len,
> > > > + cio_iodone_t end_io, void *private, gfp_t gfp_mask)
> > > > +{
> > > > + struct request_queue *in_q = bdev_get_queue(bdev_in);
> > > > + struct request_queue *out_q = bdev_get_queue(bdev_out);
> > > > + int ret = -EINVAL;
> > >
> > > Why initialize to -EINVAL if blk_copy_sanity_check() initializes it
> > > right away anyway?
> > >
> >
> > acked.
> >
> > > > + bool offload = false;
> > >
> > > Same thing with initializing offload.
> > >
> > acked
> >
> > > > +
> > > > + ret = blk_copy_sanity_check(bdev_in, pos_in, bdev_out, pos_out, len);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + offload = blk_check_copy_offload(in_q, out_q);
> > > > + if (offload)
> > > > + ret = __blk_copy_offload(bdev_in, pos_in, bdev_out, pos_out,
> > > > + len, end_io, private, gfp_mask);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(blkdev_copy_offload);
> > > > +
> > > > /*
> > > > * @bdev_in: source block device
> > > > * @pos_in: source offset
> > > > diff --git a/block/fops.c b/block/fops.c
> > > > index d2e6be4e3d1c..3b7c05831d5c 100644
> > > > --- a/block/fops.c
> > > > +++ b/block/fops.c
> > > > @@ -611,6 +611,25 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
> > > > return ret;
> > > > }
> > > >
> > > > +static ssize_t blkdev_copy_file_range(struct file *file_in, loff_t pos_in,
> > > > + struct file *file_out, loff_t pos_out,
> > > > + size_t len, unsigned int flags)
> > > > +{
> > > > + struct block_device *in_bdev = I_BDEV(bdev_file_inode(file_in));
> > > > + struct block_device *out_bdev = I_BDEV(bdev_file_inode(file_out));
> > > > + int comp_len = 0;
> > > > +
> > > > + if ((file_in->f_iocb_flags & IOCB_DIRECT) &&
> > > > + (file_out->f_iocb_flags & IOCB_DIRECT))
> > > > + comp_len = blkdev_copy_offload(in_bdev, pos_in, out_bdev,
> > > > + pos_out, len, NULL, NULL, GFP_KERNEL);
> > > > + if (comp_len != len)
> > > > + comp_len = generic_copy_file_range(file_in, pos_in + comp_len,
> > > > + file_out, pos_out + comp_len, len - comp_len, flags);
> > >
> > > I'm not deeply familiar with this code but this looks odd. It at least
> > > seems possible that comp_len could be -EINVAL and len 20 at which point
> > > you'd be doing len - comp_len aka 20 - 22 = -2 in generic_copy_file_range().
>
> 20 - -22 = 44 ofc
>
> >
> > comp_len should be 0 incase of error. We do agree, some function
>
> I mean, not to hammer on this point too much but just to be clear
> blk_copy_sanity_check(), which is introduced in the second patch, can
> return both -EPERM and -EINVAL and is first called in
> blkdev_copy_offload() so it's definitely possible for comp_len to be
> negative.

Acked. Will be updated in the next version.

Thank you,
Nitesh Shetty