Re: [PATCH V11 2/4] nvmet: add ZBD over ZNS backend support

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2021/03/11 14:29, Chaitanya Kulkarni wrote:
>>> +
>>> +bool nvmet_bdev_zns_enable(struct nvmet_ns *ns)
>>> +{
>>> +	if (nvmet_bdev_has_conv_zones(ns->bdev))
>>> +		return false;
>>> +
>>> +	ns->blksize_shift = blksize_bits(bdev_physical_block_size(ns->bdev));
>> Shouldn't this be using logical block size ? Otherwise, zsze calculation in
>> nvmet_execute_identify_cns_cs_ns() could be wrong.
> 
> Okay, will send out V12 with that fix.
> 
>>> +
>>> +	if (!nvmet_zns_update_zasl(ns))
>>> +		return false;
>>> +
>>> +	return !(get_capacity(ns->bdev->bd_disk) &
>>> +			(bdev_zone_sectors(ns->bdev) - 1));
>>> +}
>>> +
>>> +void nvmet_execute_identify_cns_cs_ctrl(struct nvmet_req *req)
>>>
>>> +
>>> +void nvmet_bdev_execute_zone_mgmt_recv(struct nvmet_req *req)
>>> +{
>>> +	sector_t sect = nvmet_lba_to_sect(req->ns, req->cmd->zmr.slba);
>>> +	u32 bufsize = (le32_to_cpu(req->cmd->zmr.numd) + 1) << 2;
>>> +	struct nvmet_report_zone_data data = { .ns = req->ns };
>>> +	unsigned int nr_zones;
>>> +	int reported_zones;
>>> +	u16 status;
>>> +
>>> +	status = nvmet_bdev_zns_checks(req);
>>> +	if (status)
>>> +		goto out;
>>> +
>>> +	data.rz = __vmalloc(bufsize, GFP_KERNEL | __GFP_NORETRY | __GFP_ZERO);
>>> +	if (!data.rz) {
>>> +		status = NVME_SC_INTERNAL;
>>> +		goto out;
>>> +	}
>>> +
>>> +	nr_zones = (bufsize - sizeof(struct nvme_zone_report)) /
>>> +			sizeof(struct nvme_zone_descriptor);
>> What if nr_zones is 0 ? This should be failed.
> 
> blkdev_report_zones() already handles that check. I thinkthe error condition
> below blkdev_report_zones() should include<= 0 case instead of just <.

Reporting 0 zones with a valid buffer size (nr_zones > 0) is a valid reply, not
an error. This can happen depending on reporting options. Even though
blkdev_report_zones() does not allow reporting options, it would be strange to
fail empty reports.

> 
> Will send V12 with the <= fix.
> 
>>
>>> +	reported_zones = blkdev_report_zones(req->ns->bdev, sect, nr_zones,
>>> +					     nvmet_bdev_report_zone_cb, &data);
>>> +	if (reported_zones < 0) {
>>> +		status = NVME_SC_INTERNAL;
>>> +		goto out_free_report_zones;
>>> +	}
>>> +
>>> +	data.rz->nr_zones = cpu_to_le64(reported_zones);
>>> +
>>> +	status = nvmet_copy_to_sgl(req, 0, data.rz, bufsize);
>>> +
>>> +out_free_report_zones:
>>> +	kvfree(data.rz);
>>> +out:
>>> +	nvmet_req_complete(req, status);
>>> +}
>>> +
>>>
> 
> 


-- 
Damien Le Moal
Western Digital Research

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme