Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
On Mon, Aug 26, 2019 at 01:20:28PM +0200, Marta Rybczynska wrote: > Do you mean something like this? Yes.
Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
- On 22 Aug, 2019, at 02:06, Christoph Hellwig h...@lst.de wrote:
> On Fri, Aug 16, 2019 at 11:47:21AM +0200, Marta Rybczynska wrote:
>> It is not possible to get 64-bit results from the passthru commands,
>> what prevents from getting for the Capabilities (CAP) property value.
>>
>> As a result, it is not possible to implement IOL's NVMe Conformance
>> test 4.3 Case 1 for Fabrics targets [1] (page 123).
>>
>> This issue has been already discussed [2], but without a solution.
>>
>> This patch solves the problem by adding new ioctls with a new
>> passthru structure, including 64-bit results. The older ioctls stay
>> unchanged.
>
> Ok, with my idea not being suitable I think I'm fine with this approach, a
> little nitpick below:
>
>> +static bool is_admin_cmd(unsigned int cmd)
>> +{
>> +if ((cmd == NVME_IOCTL_ADMIN_CMD) || (cmd == NVME_IOCTL_ADMIN64_CMD))
>> +return true;
>> +return false;
>> +}
>
> No need for the inner braces. But I'm actually not sure the current
> code structure is very suitable for extending it.
>
>> +
>> static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
>> unsigned int cmd, unsigned long arg)
>> {
>> @@ -1418,13 +1473,13 @@ static int nvme_ioctl(struct block_device *bdev,
>> fmode_t
>> mode,
>> * seperately and drop the ns SRCU reference early. This avoids a
>> * deadlock when deleting namespaces using the passthrough interface.
>> */
>> -if (cmd == NVME_IOCTL_ADMIN_CMD || is_sed_ioctl(cmd)) {
>> +if (is_admin_cmd(cmd) || is_sed_ioctl(cmd)) {
>
> So maybe for this check we should have a is_ctrl_iocl() helper instead
> that includes the is_sed_ioctl check.
>
>> struct nvme_ctrl *ctrl = ns->ctrl;
>>
>> nvme_get_ctrl(ns->ctrl);
>> nvme_put_ns_from_disk(head, srcu_idx);
>>
>> -if (cmd == NVME_IOCTL_ADMIN_CMD)
>> +if (is_admin_cmd(cmd))
>> ret = nvme_user_cmd(ctrl, NULL, argp);
>> else
>> ret = sed_ioctl(ctrl->opal_dev, cmd, argp);
>
> And then we can move this whole branch into a helper function,
> which then switches on the ioctl cmd, with sed_ioctl as the fallback.
Do you mean something like this?
+static bool is_ctrl_ioctl(unsigned int cmd)
+{
+ if (cmd == NVME_IOCTL_ADMIN_CMD || cmd == NVME_IOCTL_ADMIN64_CMD)
+ return true;
+ if (is_sed_ioctl(cmd))
+ return true;
+ return false;
+}
+
+static int nvme_handle_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
+ void __user *argp,
+ struct nvme_ns_head *head,
+ int srcu_idx)
+{
+ struct nvme_ctrl *ctrl = ns->ctrl;
+ int ret;
+
+ nvme_get_ctrl(ns->ctrl);
+ nvme_put_ns_from_disk(head, srcu_idx);
+
+ switch (cmd) {
+ case NVME_IOCTL_ADMIN_CMD:
+ ret = nvme_user_cmd(ctrl, NULL, argp);
+ break;
+ case NVME_IOCTL_ADMIN64_CMD:
+ ret = nvme_user_cmd64(ctrl, NULL, argp);
+ break;
+ default:
+ ret = sed_ioctl(ctrl->opal_dev, cmd, argp);
+ break;
+ }
+ nvme_put_ctrl(ctrl);
+ return ret;
+}
+
static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
unsigned int cmd, unsigned long arg)
{
@@ -1418,20 +1501,8 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t
mode,
* seperately and drop the ns SRCU reference early. This avoids a
* deadlock when deleting namespaces using the passthrough interface.
*/
- if (cmd == NVME_IOCTL_ADMIN_CMD || is_sed_ioctl(cmd)) {
- struct nvme_ctrl *ctrl = ns->ctrl;
-
- nvme_get_ctrl(ns->ctrl);
- nvme_put_ns_from_disk(head, srcu_idx);
-
- if (cmd == NVME_IOCTL_ADMIN_CMD)
- ret = nvme_user_cmd(ctrl, NULL, argp);
- else
- ret = sed_ioctl(ctrl->opal_dev, cmd, argp);
-
- nvme_put_ctrl(ctrl);
- return ret;
- }
+ if (is_ctrl_ioctl(cmd))
+ return nvme_handle_ctrl_ioctl(ns, cmd, argp, head, srcu_idx);
switch (cmd) {
case NVME_IOCTL_ID:
Regards,
Marta
Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
On Fri, Aug 16, 2019 at 11:47:21AM +0200, Marta Rybczynska wrote:
> It is not possible to get 64-bit results from the passthru commands,
> what prevents from getting for the Capabilities (CAP) property value.
>
> As a result, it is not possible to implement IOL's NVMe Conformance
> test 4.3 Case 1 for Fabrics targets [1] (page 123).
>
> This issue has been already discussed [2], but without a solution.
>
> This patch solves the problem by adding new ioctls with a new
> passthru structure, including 64-bit results. The older ioctls stay
> unchanged.
Ok, with my idea not being suitable I think I'm fine with this approach, a
little nitpick below:
> +static bool is_admin_cmd(unsigned int cmd)
> +{
> + if ((cmd == NVME_IOCTL_ADMIN_CMD) || (cmd == NVME_IOCTL_ADMIN64_CMD))
> + return true;
> + return false;
> +}
No need for the inner braces. But I'm actually not sure the current
code structure is very suitable for extending it.
> +
> static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
> unsigned int cmd, unsigned long arg)
> {
> @@ -1418,13 +1473,13 @@ static int nvme_ioctl(struct block_device *bdev,
> fmode_t mode,
>* seperately and drop the ns SRCU reference early. This avoids a
>* deadlock when deleting namespaces using the passthrough interface.
>*/
> - if (cmd == NVME_IOCTL_ADMIN_CMD || is_sed_ioctl(cmd)) {
> + if (is_admin_cmd(cmd) || is_sed_ioctl(cmd)) {
So maybe for this check we should have a is_ctrl_iocl() helper instead
that includes the is_sed_ioctl check.
> struct nvme_ctrl *ctrl = ns->ctrl;
>
> nvme_get_ctrl(ns->ctrl);
> nvme_put_ns_from_disk(head, srcu_idx);
>
> - if (cmd == NVME_IOCTL_ADMIN_CMD)
> + if (is_admin_cmd(cmd))
> ret = nvme_user_cmd(ctrl, NULL, argp);
> else
> ret = sed_ioctl(ctrl->opal_dev, cmd, argp);
And then we can move this whole branch into a helper function,
which then switches on the ioctl cmd, with sed_ioctl as the fallback.
Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
On Mon, Aug 19, 2019 at 08:49:22AM -0600, Keith Busch wrote: > On Mon, Aug 19, 2019 at 12:06:23AM -0700, Marta Rybczynska wrote: > > - On 16 Aug, 2019, at 15:16, Christoph Hellwig h...@lst.de wrote: > > > Sorry for not replying to the earlier version, and thanks for doing > > > this work. > > > > > > I wonder if instead of using our own structure we'd just use > > > a full nvme SQE for the input and CQE for that output. Even if we > > > reserve a few fields that means we are ready for any newly used > > > field (at least until the SQE/CQE sizes are expanded..). > > > > We could do that, nvme_command and nvme_completion are already UAPI. > > On the other hand that would mean not filling out certain fields like > > command_id. Can do an approach like this. > > Well, we need to pass user space addresses and lengths, which isn't > captured in struct nvme_command. Well, the address would fit into the data pointer. But yes, the lack of a command length concept in nvme makes this idea a mess and not really workable.
Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
On Mon, Aug 19, 2019 at 02:17:44PM -0700, Sagi Grimberg wrote: > > - On 16 Aug, 2019, at 15:16, Christoph Hellwig h...@lst.de wrote: > > Sorry for not replying to the earlier version, and thanks for doing > > this work. > > > > I wonder if instead of using our own structure we'd just use > > a full nvme SQE for the input and CQE for that output. Even if we > > reserve a few fields that means we are ready for any newly used > > field (at least until the SQE/CQE sizes are expanded..). > > We could do that, nvme_command and nvme_completion are already UAPI. > On the other hand that would mean not filling out certain fields like > command_id. Can do an approach like this. > >>> > >>> Well, we need to pass user space addresses and lengths, which isn't > >>> captured in struct nvme_command. > >> > >> Isn't simply having a 64 variant simpler? > > > > Could you provide more details on what you mean by this? > > Why would we need to pass addresses and lengths if userspace is > sending the 64 variant when it is expecting a 64 result? > > Or maybe I'm missing something... The recommendation was to have user space provide an SQE, i.e. 'struct nvme_command', as input to the driver and receive 'struct nvme_completion' in response. I am only pointing out that 'struct nvme_command' is inappropriate for user space.
Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
- On 16 Aug, 2019, at 15:16, Christoph Hellwig h...@lst.de wrote: Sorry for not replying to the earlier version, and thanks for doing this work. I wonder if instead of using our own structure we'd just use a full nvme SQE for the input and CQE for that output. Even if we reserve a few fields that means we are ready for any newly used field (at least until the SQE/CQE sizes are expanded..). We could do that, nvme_command and nvme_completion are already UAPI. On the other hand that would mean not filling out certain fields like command_id. Can do an approach like this. Well, we need to pass user space addresses and lengths, which isn't captured in struct nvme_command. Isn't simply having a 64 variant simpler? Could you provide more details on what you mean by this? Why would we need to pass addresses and lengths if userspace is sending the 64 variant when it is expecting a 64 result? Or maybe I'm missing something...
Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
On Mon, Aug 19, 2019 at 11:56:28AM -0700, Sagi Grimberg wrote: > > >> - On 16 Aug, 2019, at 15:16, Christoph Hellwig h...@lst.de wrote: > >>> Sorry for not replying to the earlier version, and thanks for doing > >>> this work. > >>> > >>> I wonder if instead of using our own structure we'd just use > >>> a full nvme SQE for the input and CQE for that output. Even if we > >>> reserve a few fields that means we are ready for any newly used > >>> field (at least until the SQE/CQE sizes are expanded..). > >> > >> We could do that, nvme_command and nvme_completion are already UAPI. > >> On the other hand that would mean not filling out certain fields like > >> command_id. Can do an approach like this. > > > > Well, we need to pass user space addresses and lengths, which isn't > > captured in struct nvme_command. > > Isn't simply having a 64 variant simpler? Could you provide more details on what you mean by this?
Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
- On 16 Aug, 2019, at 15:16, Christoph Hellwig h...@lst.de wrote: Sorry for not replying to the earlier version, and thanks for doing this work. I wonder if instead of using our own structure we'd just use a full nvme SQE for the input and CQE for that output. Even if we reserve a few fields that means we are ready for any newly used field (at least until the SQE/CQE sizes are expanded..). We could do that, nvme_command and nvme_completion are already UAPI. On the other hand that would mean not filling out certain fields like command_id. Can do an approach like this. Well, we need to pass user space addresses and lengths, which isn't captured in struct nvme_command. Isn't simply having a 64 variant simpler?
Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
On 8/19/2019 7:49 AM, Keith Busch wrote: On Mon, Aug 19, 2019 at 12:06:23AM -0700, Marta Rybczynska wrote: - On 16 Aug, 2019, at 15:16, Christoph Hellwig h...@lst.de wrote: Sorry for not replying to the earlier version, and thanks for doing this work. I wonder if instead of using our own structure we'd just use a full nvme SQE for the input and CQE for that output. Even if we reserve a few fields that means we are ready for any newly used field (at least until the SQE/CQE sizes are expanded..). We could do that, nvme_command and nvme_completion are already UAPI. On the other hand that would mean not filling out certain fields like command_id. Can do an approach like this. Well, we need to pass user space addresses and lengths, which isn't captured in struct nvme_command. This is going to be fun. It's going to have to be a cooperative effort between app and transport. There will always need to be some parts of the SQE filled out by the transport like SGL, command type/subtype bits, as well as dealing with buffers as Keith states. Command ID is another of those fields. -- james
Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
On Mon, Aug 19, 2019 at 12:06:23AM -0700, Marta Rybczynska wrote: > - On 16 Aug, 2019, at 15:16, Christoph Hellwig h...@lst.de wrote: > > Sorry for not replying to the earlier version, and thanks for doing > > this work. > > > > I wonder if instead of using our own structure we'd just use > > a full nvme SQE for the input and CQE for that output. Even if we > > reserve a few fields that means we are ready for any newly used > > field (at least until the SQE/CQE sizes are expanded..). > > We could do that, nvme_command and nvme_completion are already UAPI. > On the other hand that would mean not filling out certain fields like > command_id. Can do an approach like this. Well, we need to pass user space addresses and lengths, which isn't captured in struct nvme_command.
Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
- On 16 Aug, 2019, at 15:16, Christoph Hellwig h...@lst.de wrote: > Sorry for not replying to the earlier version, and thanks for doing > this work. > > I wonder if instead of using our own structure we'd just use > a full nvme SQE for the input and CQE for that output. Even if we > reserve a few fields that means we are ready for any newly used > field (at least until the SQE/CQE sizes are expanded..). We could do that, nvme_command and nvme_completion are already UAPI. On the other hand that would mean not filling out certain fields like command_id. Can do an approach like this. > > On Fri, Aug 16, 2019 at 11:47:21AM +0200, Marta Rybczynska wrote: >> It is not possible to get 64-bit results from the passthru commands, >> what prevents from getting for the Capabilities (CAP) property value. >> >> As a result, it is not possible to implement IOL's NVMe Conformance >> test 4.3 Case 1 for Fabrics targets [1] (page 123). > > Not that I'm not sure passing through fabrics commands is an all that > good idea. But we have pending NVMe TPs that use 64-bit result > values as well, so this seems like a good idea in general. I'm not sure how those tests could be implemented otherwise today. The goal is to send an arbitrary command to the target and the passthru command is AFAIK the only solution. If we have something better I'm sure they will be ready to use it. Regards, Marta
Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
Sorry for not replying to the earlier version, and thanks for doing this work. I wonder if instead of using our own structure we'd just use a full nvme SQE for the input and CQE for that output. Even if we reserve a few fields that means we are ready for any newly used field (at least until the SQE/CQE sizes are expanded..). On Fri, Aug 16, 2019 at 11:47:21AM +0200, Marta Rybczynska wrote: > It is not possible to get 64-bit results from the passthru commands, > what prevents from getting for the Capabilities (CAP) property value. > > As a result, it is not possible to implement IOL's NVMe Conformance > test 4.3 Case 1 for Fabrics targets [1] (page 123). Not that I'm not sure passing through fabrics commands is an all that good idea. But we have pending NVMe TPs that use 64-bit result values as well, so this seems like a good idea in general.
