Re: [PATCH] nvme: don't memset() the normal read/write command

[Christoph Hellwig <hch@lst.de>]

I like this in general, but some comments below:

>  	cmnd->rw.opcode = op;
> +	cmnd->rw.flags = 0;
> +	cmnd->rw.command_id = 0;

The command_id is always set in nvme_setup_cmd, so no need to clear it
here.

>  	cmnd->rw.nsid = cpu_to_le32(ns->head->ns_id);
> +	cmnd->rw.rsvd2 = 0;

There should be no need to clear this reserved space.

> +	cmnd->rw.metadata = 0;
>  	cmnd->rw.slba = cpu_to_le64(nvme_sect_to_lba(ns, blk_rq_pos(req)));
>  	cmnd->rw.length = cpu_to_le16((blk_rq_bytes(req) >> ns->lba_shift) - 1);
> +	cmnd->rw.reftag = 0;
> +	cmnd->rw.apptag = 0;
> +	cmnd->rw.appmask = 0;

All these PI fields are reserved when PI isn't enabled using the control
field.  So I think we can stop clearing reftag here entirely, and move
clearing the apptag and appmask down next to the reftag assignment.

>  
> +static void nvme_clear_cmd(struct request *req)
> +{
> +	if (!(req->rq_flags & RQF_DONTPREP)) {
> +		nvme_clear_nvme_request(req);
> +		memset(nvme_req(req)->cmd, 0, sizeof(struct nvme_command));
> +	}
> +}
> +
>  blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req)
>  {
>  	struct nvme_command *cmd = nvme_req(req)->cmd;
>  	struct nvme_ctrl *ctrl = nvme_req(req)->ctrl;
>  	blk_status_t ret = BLK_STS_OK;
>  
> -	if (!(req->rq_flags & RQF_DONTPREP)) {
> -		nvme_clear_nvme_request(req);
> -		memset(cmd, 0, sizeof(*cmd));
> -	}

The nvme_clear_nvme_request is not done unconditionally for the read
and write commands below, which does the wrong thing.  So I think we want
to keep it here and just move the memset.

I think the best way is to split this patch into two:

  1) remove the memset here and do it unconditionally in the individual
     nvme_setup_* handlers, and document there why the extra memsets don't
     hurt (no partial completions unlikey SCSI)
  2) optimize the read/write case