lkml.org 
[lkml]   [2020]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v11 7/9] nvmet-passthru: Add passthru code to process commands
Thanks for the Review!

On 2020-02-26 4:28 p.m., Sagi Grimberg wrote:
> This looks questionable... There are tons of features that doesn't
> make sense here like hmb, temperature stuff, irq stuff, timestamps,
> reservations etc... passing-through these will have confusing
> semantics.. Maybe white-list what actually makes sense to passthru?

Yes, I agree a white-list here probably makes sense. I'll try to come up
with a list of features to start that whitelist, though my list might be
a bit different from yours: I don't see why temperature or timestamps
can't be passed through.

Also note: Christoph was advocating against the whitelist for the
commands, though, I agree with you that it is the most sensible approach.

>> +        break;
>> +    case nvme_admin_identify:
>> +        switch (req->cmd->identify.cns) {
>> +        case NVME_ID_CNS_CTRL:
>> +            req->execute = nvmet_passthru_execute_cmd;
>> +            req->p.end_req = nvmet_passthru_override_id_ctrl;
>> +            return NVME_SC_SUCCESS;
>> +        case NVME_ID_CNS_NS:
>> +            req->execute = nvmet_passthru_execute_cmd;
>> +            req->p.end_req = nvmet_passthru_override_id_ns;
>> +            return NVME_SC_SUCCESS;
>
> Aren't you missing NVME_ID_CNS_NS_DESC_LIST? and
> NVME_ID_CNS_NS_ACTIVE_LIST?

Well no, seeing they can be passed through the default path.... But in
the light of the comment below, yes.

>> +        default:
>> +            return nvmet_setup_passthru_command(req);
>> +        }
>
> Also here, all the namespace management stuff has questionable
> semantics in my mind...

Yes, I agree with that. I'll make the change in the next revision.

Logan

\
 
 \ /
  Last update: 2020-02-27 18:34    [W:0.097 / U:2.668 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site