Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: re/BAI1wD63Z0x3nLQqwKKcj9Wo441tN9Uze14I905U+O4rm+kqx3rvYpwc3Siw4e5cijMZESH
 RVF8y4fRG3X5qIwMXCNiB3IOB75HfP8mVOeU9mE6XqtjNFn4T55gd8yGitC2oSwTWsqmfUjq3A
 Pe7FtXaZIuRjUQshTaG1FG3HSpr5Gme9YwBwT3S3VXRWTjEX74e0JOl0c03QLRXWs0PRx38Jqx
 sSEkLPJhEJMW/zX17+T2JceoO7UyFm99fSRGE7OkSBop/RKVEnmi2FYf20U7Clozg55oOzJQIR
 RuA=
From:   Johannes Thumshirn <Johannes.Thumshirn@wdc.com>
To:     SelvaKumar S <selvakuma.s1@samsung.com>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>
CC:     "kbusch@kernel.org" <kbusch@kernel.org>,
        "axboe@kernel.dk" <axboe@kernel.dk>,
        Damien Le Moal <Damien.LeMoal@wdc.com>,
        "hch@lst.de" <hch@lst.de>, "sagi@grimberg.me" <sagi@grimberg.me>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "selvajove@gmail.com" <selvajove@gmail.com>,
        "nj.shetty@samsung.com" <nj.shetty@samsung.com>,
        "joshi.k@samsung.com" <joshi.k@samsung.com>,
        "javier.gonz@samsung.com" <javier.gonz@samsung.com>
Subject: Re: [RFC PATCH 1/2] block: add simple copy support
Thread-Topic: [RFC PATCH 1/2] block: add simple copy support
Thread-Index: AQHWx7Gmd3TRGW/jckGt+BDcIzWOoQ==
Date:   Tue, 1 Dec 2020 09:57:24 +0000
Message-ID: <SN4PR0401MB3598163C941976D90F745D8B9BF40@SN4PR0401MB3598.namprd04.prod.outlook.com>
References: <20201201053949.143175-1-selvakuma.s1@samsung.com>
 <CGME20201201054057epcas5p1d5bd2813146d2cb57eb66b7cedce1f63@epcas5p1.samsung.com>
 <20201201053949.143175-2-selvakuma.s1@samsung.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 0355fd70-e1fc-4fe9-0715-08d895df8109
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Dec 2020 09:57:24.6883
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: h0TOKtYgUjPD3wnvmNz2+JxWlThsypwBikWl2pjrR48FQiDgDLDJI098HroJRPbssC/GCWHwnUmiF9zkb+HPp/B7eRror0gLxJ36S0JbpYA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR04MB7306
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 01/12/2020 08:14, SelvaKumar S wrote:=0A=
> +static inline int bio_check_copy_eod(struct bio *bio, sector_t start,=0A=
> +		sector_t nr_sectors, sector_t maxsector)=0A=
> +{=0A=
> +	if (nr_sectors && maxsector && (nr_sectors > maxsector ||=0A=
> +				start > maxsector - nr_sectors)) {=0A=
=0A=
Nit: I don't like the line break here, maybe:=0A=
=0A=
	if (nr_sectors && maxsector &&=0A=
	    (nr_sectors > maxsector || start > maxsector - nr_sectors)) {=0A=
=0A=
> +		handle_bad_sector(bio, maxsector);=0A=
> +		return -EIO;=0A=
> +	}=0A=
> +	return 0;=0A=
> +}=0A=
> +=0A=
>  /*=0A=
>   * Check whether this bio extends beyond the end of the device or partit=
ion.=0A=
>   * This may well happen - the kernel calls bread() without checking the =
size of=0A=
> @@ -737,6 +748,75 @@ static inline int bio_check_eod(struct bio *bio, sec=
tor_t maxsector)=0A=
>  	return 0;=0A=
>  }=0A=
>  =0A=
> +/*=0A=
> + * Check for copy limits and remap source ranges if needed.=0A=
> + */=0A=
> +static inline int blk_check_copy(struct bio *bio)=0A=
=0A=
That function is a bit big to be marked as inline, isn't it?=0A=
=0A=
=0A=
> +{=0A=
> +	struct hd_struct *p =3D NULL;=0A=
> +	struct request_queue *q =3D bio->bi_disk->queue;=0A=
> +	struct blk_copy_payload *payload;=0A=
> +	unsigned short nr_range;=0A=
> +	int ret =3D -EIO;=0A=
> +	int i, copy_len =3D 0;=0A=
> +=0A=
> +	rcu_read_lock();=0A=
> +=0A=
> +	if (bio->bi_partno) {=0A=
> +		p =3D __disk_get_part(bio->bi_disk, bio->bi_partno);=0A=
> +		if (unlikely(!p))=0A=
> +			goto out;=0A=
> +		if (unlikely(bio_check_ro(bio, p)))=0A=
> +			goto out;=0A=
> +	} else {=0A=
> +		if (unlikely(bio_check_ro(bio, &bio->bi_disk->part0)))=0A=
> +			goto out;=0A=
> +	}=0A=
> +=0A=
> +	payload =3D bio_data(bio);=0A=
> +	nr_range =3D payload->copy_range;=0A=
> +=0A=
> +	/* cannot handle copy crossing nr_ranges limit */=0A=
> +	if (payload->copy_range > q->limits.max_copy_nr_ranges)=0A=
> +		goto out;=0A=
> +=0A=
> +	for (i =3D 0; i < nr_range; i++) {=0A=
> +		copy_len +=3D payload->range[i].len;=0A=
> +		if (p) {=0A=
> +			if (bio_check_copy_eod(bio, payload->range[i].src,=0A=
> +					payload->range[i].len, part_nr_sects_read(p)))=0A=
> +				goto out;=0A=
> +			payload->range[i].src +=3D p->start_sect;=0A=
> +		} else {=0A=
> +			if (unlikely(bio_check_copy_eod(bio, payload->range[i].src,=0A=
> +					payload->range[i].len,=0A=
> +					get_capacity(bio->bi_disk))))=0A=
> +				goto out;=0A=
> +		}=0A=
> +	}=0A=
> +=0A=
> +	/* cannot handle copy more than copy limits */=0A=
> +	if (copy_len > q->limits.max_copy_sectors)=0A=
> +		goto out;=0A=
> +=0A=
> +	if (p) {=0A=
> +		if (unlikely(bio_check_copy_eod(bio, bio->bi_iter.bi_sector, copy_len,=
=0A=
> +					part_nr_sects_read(p))))=0A=
> +			goto out;=0A=
> +	} else {=0A=
> +		if (unlikely(bio_check_copy_eod(bio, bio->bi_iter.bi_sector, copy_len,=
=0A=
> +					get_capacity(bio->bi_disk))))=0A=
> +			goto out;=0A=
> +=0A=
> +	}=0A=
> +=0A=
=0A=
=0A=
All these if (p) {} else {} branches make this function a bit hard to follo=
w.=0A=
=0A=
> +	if (p)=0A=
> +		bio->bi_partno =3D 0;=0A=
> +	ret =3D 0;=0A=
> +out:=0A=
> +	rcu_read_unlock();=0A=
> +	return ret;=0A=
> +}=0A=
>  /*=0A=
>   * Remap block n of partition p to block n+start(p) of the disk.=0A=
>   */=0A=
=0A=
=0A=
> diff --git a/block/blk-lib.c b/block/blk-lib.c=0A=
> index e90614fd8d6a..db4947f7014d 100644=0A=
> --- a/block/blk-lib.c=0A=
> +++ b/block/blk-lib.c=0A=
> @@ -150,6 +150,122 @@ int blkdev_issue_discard(struct block_device *bdev,=
 sector_t sector,=0A=
>  }=0A=
>  EXPORT_SYMBOL(blkdev_issue_discard);=0A=
>  =0A=
> +int __blkdev_issue_copy(struct block_device *bdev, sector_t dest,=0A=
> +		sector_t nr_srcs, struct range_entry *rlist, gfp_t gfp_mask,=0A=
> +		int flags, struct bio **biop)=0A=
> +{=0A=
> +	struct request_queue *q =3D bdev_get_queue(bdev);=0A=
> +	struct bio *bio;=0A=
> +	struct blk_copy_payload *payload;=0A=
> +	unsigned int op;=0A=
=0A=
I don't think op is needed.=0A=
=0A=
> +	sector_t bs_mask;=0A=
> +	sector_t src_sects, len =3D 0, total_len =3D 0;=0A=
> +	int i, ret, total_size;=0A=
> +=0A=
> +	if (!q)=0A=
> +		return -ENXIO;=0A=
> +=0A=
> +	if (!nr_srcs)=0A=
> +		return -EINVAL;=0A=
> +=0A=
> +	if (bdev_read_only(bdev))=0A=
> +		return -EPERM;=0A=
> +=0A=
> +	if (!blk_queue_copy(q))=0A=
> +		return -EOPNOTSUPP;=0A=
> +	op =3D REQ_OP_COPY;=0A=
> +=0A=
> +	bs_mask =3D (bdev_logical_block_size(bdev) >> 9) - 1;=0A=
> +	if (dest & bs_mask)=0A=
> +		return -EINVAL;=0A=
> +=0A=
> +	payload =3D kmalloc(sizeof(struct blk_copy_payload) +=0A=
> +			nr_srcs * sizeof(struct range_entry),=0A=
> +				GFP_ATOMIC | __GFP_NOWARN);=0A=
=0A=
Please check if the use of struct_size() is possible. Probably even assign=
=0A=
total_size here so you don't need to do the size calculation twice.=0A=
=0A=
> +	if (!payload)=0A=
> +		return -ENOMEM;=0A=
> +=0A=
> +	bio =3D bio_alloc(gfp_mask, 1);=0A=
> +	bio->bi_iter.bi_sector =3D dest;=0A=
> +	bio_set_dev(bio, bdev);=0A=
> +	bio_set_op_attrs(bio, op, REQ_NOMERGE);=0A=
=0A=
bio_set_op_attrs() is deprecated, please don't use it.=0A=
	bio->bi_opf =3D REQ_OP_COPY | REQ_NOMERGE;=0A=
=0A=
> +=0A=
> +	payload->dest =3D dest;=0A=
> +=0A=
> +	for (i =3D 0; i < nr_srcs; i++) {=0A=
> +		/*  copy payload provided are in bytes */=0A=
> +		src_sects =3D rlist[i].src;=0A=
> +		if (src_sects & bs_mask)=0A=
> +			return -EINVAL;=0A=
> +		src_sects =3D src_sects >> SECTOR_SHIFT;=0A=
> +=0A=
> +		if (len & bs_mask)=0A=
> +			return -EINVAL;=0A=
> +		len =3D rlist[i].len >> SECTOR_SHIFT;=0A=
> +		if (len > q->limits.max_copy_range_sectors)=0A=
> +			return -EINVAL;=0A=
> +=0A=
> +		total_len +=3D len;=0A=
> +=0A=
> +		WARN_ON_ONCE((src_sects << 9) > UINT_MAX);=0A=
> +=0A=
> +		payload->range[i].src =3D src_sects;=0A=
> +		payload->range[i].len =3D len;=0A=
> +	}=0A=
> +=0A=
> +	/* storing # of source ranges */=0A=
> +	payload->copy_range =3D i;=0A=
> +	/* storing copy len so far */=0A=
> +	payload->copy_size =3D total_len;=0A=
> +=0A=
> +	total_size =3D sizeof(struct blk_copy_payload) + nr_srcs * sizeof(struc=
t range_entry);=0A=
=0A=
See above.=0A=
=0A=
> +	ret =3D bio_add_page(bio, virt_to_page(payload), total_size,=0A=
> +					   offset_in_page(payload));=0A=
> +	if (ret !=3D total_size) {=0A=
> +		kfree(payload);=0A=
> +		return -ENOMEM;=0A=
> +	}=0A=
> +=0A=
> +	*biop =3D bio;=0A=
> +	return 0;=0A=
> +}=0A=
> +EXPORT_SYMBOL(__blkdev_issue_copy);=0A=
> +=0A=
> +/**=0A=
> + * blkdev_issue_copy - queue a copy=0A=
> + * @bdev:       blockdev to issue copy for=0A=
> + * @dest:	dest sector=0A=
> + * @nr_srcs:	number of source ranges to copy=0A=
> + * @rlist:	list of range entries=0A=
> + * @gfp_mask:   memory allocation flags (for bio_alloc)=0A=
> + * @flags:      BLKDEV_COPY_* flags to control behaviour	//TODO=0A=
> + *=0A=
> + * Description:=0A=
> + *    Issue a copy request for dest sector with source in rlist=0A=
> + */=0A=
> +int blkdev_issue_copy(struct block_device *bdev, sector_t dest,=0A=
> +		int nr_srcs, struct range_entry *rlist,=0A=
> +		gfp_t gfp_mask, unsigned long flags)=0A=
> +{=0A=
> +	struct bio *bio =3D NULL;=0A=
> +	int ret;=0A=
> +=0A=
> +	ret =3D __blkdev_issue_copy(bdev, dest, nr_srcs, rlist, gfp_mask, flags=
,=0A=
> +			&bio);=0A=
> +	if (!ret && bio) {=0A=
> +		ret =3D submit_bio_wait(bio);=0A=
> +		if (ret =3D=3D -EOPNOTSUPP)=0A=
> +			ret =3D 0;=0A=
> +=0A=
> +		kfree(page_address(bio_first_bvec_all(bio)->bv_page) +=0A=
> +				bio_first_bvec_all(bio)->bv_offset);=0A=
> +		bio_put(bio);=0A=
> +	}=0A=
> +=0A=
> +	return ret;=0A=
=0A=
What happens with bio here if __blkdev_issue_copy() returns say -ENOMEM bec=
ause=0A=
bio_add_page() fails?=0A=
=0A=
Also please handle failure not success. Sth along the lines of=0A=
=0A=
	ret =3D __blkdev_issue_copy(bdev, dest, nr_srcs, rlist, gfp_mask, flags,=
=0A=
				  &bio);=0A=
	if (ret)=0A=
		...=0A=
=0A=
	ret =3D submit_bio_wait();=0A=
	if (ret)=0A=
		...=0A=
=0A=
	...=0A=
=0A=
	return ret;=0A=
}=0A=
=0A=
> +}=0A=
> +EXPORT_SYMBOL(blkdev_issue_copy);=0A=
=0A=
=0A=
=0A=