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

[Christoph Hellwig <hch@lst.de>]

> +static void nvmet_set_csi_zns_effects(struct nvme_effects_log *log)

Same naming nitpick as for the nvm version.

>  	switch (req->ns->csi) {
>  	case NVME_CSI_NVM:
> +		return nvmet_copy_ns_identifier(req, NVME_NIDT_CSI,
> +						NVME_NIDT_CSI_LEN,
> +						&req->ns->csi, o);
> +	case NVME_CSI_ZNS:
> +		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> +			return NVME_SC_INVALID_IO_CMD_SET;
> +

This goes away with my comment on the previous patch.

> @@ -448,6 +456,15 @@ u16 nvmet_bdev_parse_io_cmd(struct nvmet_req *req)
>  	case nvme_cmd_write_zeroes:
>  		req->execute = nvmet_bdev_execute_write_zeroes;
>  		return 0;
> +	case nvme_cmd_zone_append:
> +		req->execute = nvmet_bdev_execute_zone_append;
> +		return 0;
> +	case nvme_cmd_zone_mgmt_recv:
> +		req->execute = nvmet_bdev_execute_zone_mgmt_recv;
> +		return 0;
> +	case nvme_cmd_zone_mgmt_send:
> +		req->execute = nvmet_bdev_execute_zone_mgmt_send;

I think we need a separate _parse for just ZNS.  That way we can
do the ns.csi and IS_ENABLED check in one single place, and we
also don't need stubs for any of these functions as they are all
under the IS_ENABLED check and thus the compiler will never generate
a call to them for !CONFIG_BLK_DEV_ZONED.

> +static u16 nvmet_bdev_validate_zone_mgmt_recv(struct nvmet_req *req)
> +{
> +	sector_t sect = nvmet_lba_to_sect(req->ns, req->cmd->zmr.slba);
> +	u32 out_bufsize = (le32_to_cpu(req->cmd->zmr.numd) + 1) << 2;
> +
> +	if (!bdev_is_zoned(req->ns->bdev))
> +		return NVME_SC_INVALID_NS | NVME_SC_DNR;

I think checking the csi іn the nvmet_ns structure here is a lot
cleaner.  And as mentioned abive I think we should do this once for
all zns-specific commands.

> +void nvmet_execute_identify_cns_cs_ctrl(struct nvmet_req *req)
> +{
> +	u8 zasl = req->sq->ctrl->subsys->zasl;
> +	struct nvmet_ctrl *ctrl = req->sq->ctrl;
> +	struct nvme_id_ctrl_zns *id;
> +	u16 status;
> +
> +	if (req->cmd->identify.csi != NVME_CSI_ZNS) {
> +		req->error_loc = offsetof(struct nvme_common_command, opcode);
> +		status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
> +		goto out;
> +	}

The CSI check or rather a switch on the CSI with a default fail
needs to move into the common code, probably into the main parsing
function.

> +void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
> +{
> +	struct nvme_id_ns_zns *id_zns;
> +	u64 zsze;
> +	u16 status;
> +
> +	if (req->cmd->identify.csi != NVME_CSI_ZNS) {
> +		req->error_loc = offsetof(struct nvme_common_command, opcode);
> +		status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
> +		goto out;
> +	}

Same here.

> +static int nvmet_bdev_report_zone_cb(struct blk_zone *z, unsigned i, void *d)
> +{
> +	struct nvmet_report_zone_data *rz = d;
> +	struct nvme_zone_descriptor *entries = rz->rz->entries;
> +	struct nvmet_ns *ns = rz->ns;
> +	static const unsigned int blk_zcond_to_nvme_zstate[] = {
> +		[BLK_ZONE_COND_EMPTY]	 = NVME_ZRASF_ZONE_STATE_EMPTY,
> +		[BLK_ZONE_COND_IMP_OPEN] = NVME_ZRASF_ZONE_STATE_IMP_OPEN,
> +		[BLK_ZONE_COND_EXP_OPEN] = NVME_ZRASF_ZONE_STATE_EXP_OPEN,
> +		[BLK_ZONE_COND_CLOSED]	 = NVME_ZRASF_ZONE_STATE_CLOSED,
> +		[BLK_ZONE_COND_READONLY] = NVME_ZRASF_ZONE_STATE_READONLY,
> +		[BLK_ZONE_COND_FULL]	 = NVME_ZRASF_ZONE_STATE_FULL,
> +		[BLK_ZONE_COND_OFFLINE]	 = NVME_ZRASF_ZONE_STATE_OFFLINE,
> +	};
> +
> +	if (rz->zrasf == NVME_ZRASF_ZONE_REPORT_ALL)
> +		goto record_zone;
> +
> +	/*
> +	 * Make sure this zone condition's value is mapped to NVMe ZNS zone
> +	 * condition value.
> +	 */
> +	if (z->cond > ARRAY_SIZE(blk_zcond_to_nvme_zstate) ||
> +	    !blk_zcond_to_nvme_zstate[z->cond])
> +		return -EINVAL;
> +
> +	/* filter zone by condition */
> +	if (blk_zcond_to_nvme_zstate[z->cond] != rz->zrasf)
> +		return 0;
> +
> +record_zone:

While not bad per se I ind the structure a little odd.  I'd move the
checks into a level of indentation instead.

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