On Sat, 2020-12-12 at 21:50 -0800, Chaitanya Kulkarni wrote:
[...]
> +static int nvmet_bdev_validate_zns_zones_cb(struct blk_zone *z,
> + unsigned int idx, void *data)
> +{
> + struct blk_zone *zone = data;
> +
> + memcpy(zone, z, sizeof(struct blk_zone));
See below. This is not necessary.
> +
> + return 0;
> +}
> +
> +static bool nvmet_bdev_has_conv_zones(struct block_device *bdev)
> +{
> + struct blk_zone zone;
> + int reported_zones;
> + unsigned int zno;
> +
> + if (bdev->bd_disk->queue->conv_zones_bitmap)
> + return false;
Bug.
> +
> + for (zno = 0; zno < blk_queue_nr_zones(bdev->bd_disk->queue); zno++) {
Large capacity SMR drives have over 75,000 zones these days. Doing a report
zones one zone at a time will take forever. This needs to be optimized: see
below.
> + reported_zones = blkdev_report_zones(bdev,
> + zno * bdev_zone_sectors(bdev), 1,
> + nvmet_bdev_validate_zns_zones_cb,
> + &zone);
> +
> + if (reported_zones != 1)
> + return true;
> +
> + if (zone.type == BLK_ZONE_TYPE_CONVENTIONAL)
> + return true;
This test should be in the nvmet_bdev_validate_zns_zones_cb() callback. That
callback can return an error if it finds a conventional zone. That will stop
blkdev_report_zones().
> + }
> +
> + return false;
> +}
What about this:
static int nvmet_bdev_validate_zns_zones_cb(struct blk_zone *z,
unsigned int idx, void *data)
{
if (z->type == BLK_ZONE_TYPE_CONVENTIONAL)
return -ENOTSUPP;
return 0;
}
static bool nvmet_bdev_has_conv_zones(struct block_device *bdev)
{
int ret;
if (bdev->bd_disk->queue->conv_zones_bitmap)
return true;
ret = blkdev_report_zones(bdev,
get_capacity(bdev->bd_disk), bdev_nr_zones(bdev),
nvmet_bdev_validate_zns_zones_cb, NULL);
if (ret < 1)
return true;
return false;
}
All zones are checked using the callback with the loop in
blkdev_report_zones().
[...]
> +void nvmet_bdev_execute_zone_append(struct nvmet_req *req)
> +{
> + sector_t sect = nvmet_lba_to_sect(req->ns, req->cmd->rw.slba);
> + struct request_queue *q = req->ns->bdev->bd_disk->queue;
> + unsigned int max_sects = queue_max_zone_append_sectors(q);
> + u16 status = NVME_SC_SUCCESS;
> + unsigned int total_len = 0;
> + struct scatterlist *sg;
> + int ret = 0, sg_cnt;
> + struct bio *bio;
> +
> + if (!nvmet_check_transfer_len(req, nvmet_rw_data_len(req)))
> + return;
> +
> + if (!req->sg_cnt) {
> + nvmet_req_complete(req, 0);
> + return;
> + }
> +
> + if (req->transfer_len <= NVMET_MAX_INLINE_DATA_LEN) {
> + bio = &req->b.inline_bio;
> + bio_init(bio, req->inline_bvec, ARRAY_SIZE(req->inline_bvec));
> + } else {
> + bio = bio_alloc(GFP_KERNEL, req->sg_cnt);
> + }
> +
> + bio_set_dev(bio, req->ns->bdev);
> + bio->bi_iter.bi_sector = sect;
> + bio->bi_opf = REQ_OP_ZONE_APPEND | REQ_SYNC | REQ_IDLE;
> + if (req->cmd->rw.control & cpu_to_le16(NVME_RW_FUA))
> + bio->bi_opf |= REQ_FUA;
> +
> + for_each_sg(req->sg, sg, req->sg_cnt, sg_cnt) {
> + struct page *p = sg_page(sg);
> + unsigned int l = sg->length;
> + unsigned int o = sg->offset;
> + bool same_page = false;
> +
> + ret = bio_add_hw_page(q, bio, p, l, o, max_sects, &same_page);
> + if (ret != sg->length) {
> + status = NVME_SC_INTERNAL;
> + goto out_bio_put;
> + }
> + if (same_page)
> + put_page(p);
> +
> + total_len += sg->length;
> + }
> +
> + if (total_len != nvmet_rw_data_len(req)) {
> + status = NVME_SC_INTERNAL | NVME_SC_DNR;
> + goto out_bio_put;
> + }
> +
> + ret = submit_bio_wait(bio);
> + status = ret < 0 ? NVME_SC_INTERNAL : status;
> +
> + req->cqe->result.u64 = nvmet_sect_to_lba(req->ns,
> + bio->bi_iter.bi_sector);
Why set this if the BIO failed ? There may be no problems doing so, but I do
not see the point either.
> +
> +out_bio_put:
> + if (bio != &req->b.inline_bio)
> + bio_put(bio);
> + nvmet_req_complete(req, status);
> +}
--
Damien Le Moal
Western Digital
