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

[Jens Axboe <axboe@kernel.dk>]

On 10/12/21 10:58 PM, Christoph Hellwig wrote:
> 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.

Perfect, I'll drop it.

>>  	cmnd->rw.nsid = cpu_to_le32(ns->head->ns_id);
>> +	cmnd->rw.rsvd2 = 0;
> 
> There should be no need to clear this reserved space.

Actually wasn't sure, sometimes hardware specs required reserved fields
to get cleared. nvme does not? I'd be happy to drop it if not the case.

>> +	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.

OK, will move.

>>  
>> +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

OK I'll do that and resend.

-- 
Jens Axboe