[PATCH V13 2/4] nvmet: add ZBD over ZNS backend support
Christoph Hellwig
hch at lst.de
Thu Apr 8 08:23:37 BST 2021
> +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.
More information about the Linux-nvme
mailing list