[PATCH V9 4/9] nvmet: add ZBD over ZNS backend support

[Damien Le Moal]

On 2021/01/12 15:11, Chaitanya Kulkarni wrote:
[...]
>>> +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;
>>> +
>>> +	nr_zones = (bufsize - sizeof(struct nvme_zone_report)) /
>>> +			sizeof(struct nvme_zone_descriptor);
>> I really would prefer this code to be moved down, before the call to
>> blkdev_report_zones().
>>
>> You can also optimize this value a little with a min() of the value above and of
>> DIV_ROUND_UP(dev_capacity - sect, zone size). But not a big deal I think.
> I did that as per your last comment, when I did the code review with
> host side it didn't match, I've a cleanup patch series to fix nits and
> host side css checks for zns I've added this into that series.
>>> +
>>> +	status = nvmet_bdev_zns_checks(req);
>>> +	if (status)
>>> +		goto out;
>>> +
>>> +	data.rz = __vmalloc(bufsize, GFP_KERNEL | __GFP_NORETRY | __GFP_ZERO);
>> Shouldn't this be GFP_NOIO ? Also, is the NORETRY critical ?
> Yes on GFP_NOIO. NORETRY critical means how we areallocating the memory on
> the host side nvme_zns_alloc_report_buffer() ?

By critical, I mean that if __GFP_NORETRY is removed, things break ? Or is it
just an optimization to avoid overtaxing the host resources ? I suspect the
latter case, but wanted to make sure.



-- 
Damien Le Moal
Western Digital Research